Custom serializers for ChangeKindCache, including protobuf Change-Id: I7f2e56f78fd346170922c366c57465d6dfb136ef
diff --git a/java/com/google/gerrit/server/BUILD b/java/com/google/gerrit/server/BUILD index 70b3d68..ab2c26b 100644 --- a/java/com/google/gerrit/server/BUILD +++ b/java/com/google/gerrit/server/BUILD
@@ -89,6 +89,7 @@ "//lib/ow2:ow2-asm-tree", "//lib/ow2:ow2-asm-util", "//lib/prolog:runtime", + "//proto:cache_java_proto", ], )
diff --git a/java/com/google/gerrit/server/cache/CacheModule.java b/java/com/google/gerrit/server/cache/CacheModule.java index 79c1735..32d604e 100644 --- a/java/com/google/gerrit/server/cache/CacheModule.java +++ b/java/com/google/gerrit/server/cache/CacheModule.java
@@ -26,7 +26,6 @@ import com.google.inject.TypeLiteral; import com.google.inject.name.Names; import com.google.inject.util.Types; -import java.io.Serializable; import java.lang.reflect.Type; /** Miniature DSL to support binding {@link Cache} instances in Guice. */ @@ -120,7 +119,7 @@ * @param <V> type of value stored by the cache. * @return binding to describe the cache. */ - protected <K extends Serializable, V extends Serializable> PersistentCacheBinding<K, V> persist( + protected <K, V> PersistentCacheBinding<K, V> persist( String name, Class<K> keyType, Class<V> valType) { return persist(name, TypeLiteral.get(keyType), TypeLiteral.get(valType)); } @@ -132,7 +131,7 @@ * @param <V> type of value stored by the cache. * @return binding to describe the cache. */ - protected <K extends Serializable, V extends Serializable> PersistentCacheBinding<K, V> persist( + protected <K, V> PersistentCacheBinding<K, V> persist( String name, Class<K> keyType, TypeLiteral<V> valType) { return persist(name, TypeLiteral.get(keyType), valType); } @@ -144,7 +143,7 @@ * @param <V> type of value stored by the cache. * @return binding to describe the cache. */ - protected <K extends Serializable, V extends Serializable> PersistentCacheBinding<K, V> persist( + protected <K, V> PersistentCacheBinding<K, V> persist( String name, TypeLiteral<K> keyType, TypeLiteral<V> valType) { PersistentCacheProvider<K, V> m = new PersistentCacheProvider<>(this, name, keyType, valType); bindCache(m, name, keyType, valType);
diff --git a/java/com/google/gerrit/server/cache/EnumCacheSerializer.java b/java/com/google/gerrit/server/cache/EnumCacheSerializer.java new file mode 100644 index 0000000..6ea61215 --- /dev/null +++ b/java/com/google/gerrit/server/cache/EnumCacheSerializer.java
@@ -0,0 +1,41 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.cache; + +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.google.common.base.Enums; +import java.io.IOException; + +public class EnumCacheSerializer<E extends Enum<E>> implements CacheSerializer<E> { + private final Class<E> clazz; + + public EnumCacheSerializer(Class<E> clazz) { + this.clazz = clazz; + } + + @Override + public byte[] serialize(E object) throws IOException { + return object.name().getBytes(UTF_8); + } + + @Override + public E deserialize(byte[] in) throws IOException { + String name = new String(in, UTF_8); + return Enums.getIfPresent(clazz, name) + .toJavaUtil() + .orElseThrow(() -> new IOException("Invalid " + clazz.getName() + " value: " + name)); + } +}
diff --git a/java/com/google/gerrit/server/cache/JavaCacheSerializer.java b/java/com/google/gerrit/server/cache/JavaCacheSerializer.java index ee46dc6..750c5df 100644 --- a/java/com/google/gerrit/server/cache/JavaCacheSerializer.java +++ b/java/com/google/gerrit/server/cache/JavaCacheSerializer.java
@@ -19,10 +19,14 @@ import java.io.IOException; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; -import java.io.Serializable; -/** Serializer that uses default Java serialization. */ -public class JavaCacheSerializer<T extends Serializable> implements CacheSerializer<T> { +/** + * Serializer that uses default Java serialization. + * + * @param <T> type to serialize. Must implement {@code Serializable}, but due to implementation + * details this is only checked at runtime. + */ +public class JavaCacheSerializer<T> implements CacheSerializer<T> { @Override public byte[] serialize(T object) throws IOException { try (ByteArrayOutputStream bout = new ByteArrayOutputStream();
diff --git a/java/com/google/gerrit/server/cache/PersistentCacheProvider.java b/java/com/google/gerrit/server/cache/PersistentCacheProvider.java index f6a89e4..405de4f 100644 --- a/java/com/google/gerrit/server/cache/PersistentCacheProvider.java +++ b/java/com/google/gerrit/server/cache/PersistentCacheProvider.java
@@ -23,6 +23,7 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.TypeLiteral; +import java.io.Serializable; import java.util.concurrent.TimeUnit; class PersistentCacheProvider<K, V> extends CacheProvider<K, V> @@ -119,12 +120,24 @@ return super.get(); } checkState(version >= 0, "version is required"); - checkState(keySerializer != null, "keySerializer is required"); - checkState(valueSerializer != null, "valueSerializer is required"); + checkSerializer(keyType(), keySerializer, "key"); + checkSerializer(valueType(), valueSerializer, "value"); freeze(); CacheLoader<K, V> ldr = loader(); return ldr != null ? persistentCacheFactory.build(this, ldr) : persistentCacheFactory.build(this); } + + private static <T> void checkSerializer( + TypeLiteral<T> type, CacheSerializer<T> serializer, String name) { + checkState(serializer != null, "%sSerializer is required", name); + if (serializer instanceof JavaCacheSerializer) { + checkState( + Serializable.class.isAssignableFrom(type.getRawType()), + "%s type %s must implement Serializable", + name, + type); + } + } }
diff --git a/java/com/google/gerrit/server/cache/ProtoCacheSerializers.java b/java/com/google/gerrit/server/cache/ProtoCacheSerializers.java new file mode 100644 index 0000000..795df72 --- /dev/null +++ b/java/com/google/gerrit/server/cache/ProtoCacheSerializers.java
@@ -0,0 +1,47 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.cache; + +import com.google.protobuf.CodedOutputStream; +import com.google.protobuf.MessageLite; +import java.io.IOException; + +/** Static utilities for writing protobuf-based {@link CacheSerializer} implementations. */ +public class ProtoCacheSerializers { + /** + * Serializes a proto to a byte array. + * + * <p>Guarantees deterministic serialization and thus is suitable for use as a persistent cache + * key. Should be used in preference to {@link MessageLite#toByteArray()}, which is not guaranteed + * deterministic. + * + * @param message the proto message to serialize. + * @return a byte array with the message contents. + */ + public static byte[] toByteArray(MessageLite message) { + byte[] bytes = new byte[message.getSerializedSize()]; + CodedOutputStream cout = CodedOutputStream.newInstance(bytes); + cout.useDeterministicSerialization(); + try { + message.writeTo(cout); + cout.checkNoSpaceLeft(); + return bytes; + } catch (IOException e) { + throw new IllegalStateException("exception writing to byte array"); + } + } + + private ProtoCacheSerializers() {} +}
diff --git a/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java b/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java index 7b44d8f..6449662 100644 --- a/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java +++ b/java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
@@ -16,8 +16,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; -import static org.eclipse.jgit.lib.ObjectIdSerializer.readWithoutMarker; -import static org.eclipse.jgit.lib.ObjectIdSerializer.writeWithoutMarker; import com.google.common.annotations.VisibleForTesting; import com.google.common.cache.Cache; @@ -30,6 +28,10 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.cache.CacheModule; +import com.google.gerrit.server.cache.CacheSerializer; +import com.google.gerrit.server.cache.EnumCacheSerializer; +import com.google.gerrit.server.cache.ProtoCacheSerializers; +import com.google.gerrit.server.cache.proto.Cache.ChangeKindKeyProto; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.InMemoryInserter; @@ -39,10 +41,8 @@ import com.google.inject.Inject; import com.google.inject.Module; import com.google.inject.name.Named; +import com.google.protobuf.ByteString; import java.io.IOException; -import java.io.ObjectInputStream; -import java.io.ObjectOutputStream; -import java.io.Serializable; import java.util.Arrays; import java.util.Collection; import java.util.Objects; @@ -51,6 +51,7 @@ import java.util.concurrent.ExecutionException; import org.eclipse.jgit.errors.LargeObjectException; import org.eclipse.jgit.lib.Config; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.Repository; @@ -72,7 +73,10 @@ bind(ChangeKindCache.class).to(ChangeKindCacheImpl.class); persist(ID_CACHE, Key.class, ChangeKind.class) .maximumWeight(2 << 20) - .weigher(ChangeKindWeigher.class); + .weigher(ChangeKindWeigher.class) + .version(1) + .keySerializer(new Key.Serializer()) + .valueSerializer(new EnumCacheSerializer<>(ChangeKind.class)); } }; } @@ -122,9 +126,7 @@ } } - public static class Key implements Serializable { - private static final long serialVersionUID = 1L; - + public static class Key { private transient ObjectId prior; private transient ObjectId next; private transient String strategyName; @@ -171,16 +173,28 @@ return Objects.hash(prior, next, strategyName); } - private void writeObject(ObjectOutputStream out) throws IOException { - writeWithoutMarker(out, prior); - writeWithoutMarker(out, next); - out.writeUTF(strategyName); - } + @VisibleForTesting + static class Serializer implements CacheSerializer<Key> { + @Override + public byte[] serialize(Key object) throws IOException { + byte[] buf = new byte[Constants.OBJECT_ID_LENGTH]; + ChangeKindKeyProto.Builder b = ChangeKindKeyProto.newBuilder(); + object.getPrior().copyRawTo(buf, 0); + b.setPrior(ByteString.copyFrom(buf)); + object.getNext().copyRawTo(buf, 0); + b.setNext(ByteString.copyFrom(buf)); + b.setStrategyName(object.getStrategyName()); + return ProtoCacheSerializers.toByteArray(b.build()); + } - private void readObject(ObjectInputStream in) throws IOException { - prior = readWithoutMarker(in); - next = readWithoutMarker(in); - strategyName = in.readUTF(); + @Override + public Key deserialize(byte[] in) throws IOException { + ChangeKindKeyProto proto = ChangeKindKeyProto.parseFrom(in); + return new Key( + ObjectId.fromRaw(proto.getPrior().toByteArray()), + ObjectId.fromRaw(proto.getNext().toByteArray()), + proto.getStrategyName()); + } } }
diff --git a/javatests/com/google/gerrit/server/BUILD b/javatests/com/google/gerrit/server/BUILD index 2ddfaa9..3864676 100644 --- a/javatests/com/google/gerrit/server/BUILD +++ b/javatests/com/google/gerrit/server/BUILD
@@ -53,6 +53,7 @@ "//lib:gson", "//lib:guava-retrying", "//lib:gwtorm", + "//lib:protobuf", "//lib:truth-java8-extension", "//lib/auto:auto-value", "//lib/auto:auto-value-annotations", @@ -60,5 +61,6 @@ "//lib/guice", "//lib/jgit/org.eclipse.jgit:jgit", "//lib/jgit/org.eclipse.jgit.junit:junit", + "//proto:cache_java_proto", ], )
diff --git a/javatests/com/google/gerrit/server/cache/EnumCacheSerializerTest.java b/javatests/com/google/gerrit/server/cache/EnumCacheSerializerTest.java new file mode 100644 index 0000000..0e04d32 --- /dev/null +++ b/javatests/com/google/gerrit/server/cache/EnumCacheSerializerTest.java
@@ -0,0 +1,39 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.cache; + +import static com.google.common.truth.Truth.assertThat; + +import org.junit.Test; + +public class EnumCacheSerializerTest { + @Test + public void serialize() throws Exception { + assertRoundTrip(MyEnum.FOO); + assertRoundTrip(MyEnum.BAR); + assertRoundTrip(MyEnum.BAZ); + } + + private enum MyEnum { + FOO, + BAR, + BAZ; + } + + private static void assertRoundTrip(MyEnum e) throws Exception { + CacheSerializer<MyEnum> s = new EnumCacheSerializer<>(MyEnum.class); + assertThat(s.deserialize(s.serialize(e))).isEqualTo(e); + } +}
diff --git a/javatests/com/google/gerrit/server/change/ChangeKindCacheImplTest.java b/javatests/com/google/gerrit/server/change/ChangeKindCacheImplTest.java new file mode 100644 index 0000000..4470f55 --- /dev/null +++ b/javatests/com/google/gerrit/server/change/ChangeKindCacheImplTest.java
@@ -0,0 +1,55 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.change; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.gerrit.server.cache.CacheSerializer; +import com.google.gerrit.server.cache.proto.Cache.ChangeKindKeyProto; +import com.google.protobuf.ByteString; +import org.eclipse.jgit.lib.ObjectId; +import org.junit.Test; + +public class ChangeKindCacheImplTest { + @Test + public void keySerializer() throws Exception { + ChangeKindCacheImpl.Key key = + new ChangeKindCacheImpl.Key( + ObjectId.zeroId(), + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"), + "aStrategy"); + CacheSerializer<ChangeKindCacheImpl.Key> s = new ChangeKindCacheImpl.Key.Serializer(); + byte[] serialized = s.serialize(key); + assertThat(ChangeKindKeyProto.parseFrom(serialized)) + .isEqualTo( + ChangeKindKeyProto.newBuilder() + .setPrior(bytes(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)) + .setNext( + bytes( + 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, + 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef)) + .setStrategyName("aStrategy") + .build()); + assertThat(s.deserialize(serialized)).isEqualTo(key); + } + + private static ByteString bytes(int... ints) { + byte[] bytes = new byte[ints.length]; + for (int i = 0; i < ints.length; i++) { + bytes[i] = (byte) ints[i]; + } + return ByteString.copyFrom(bytes); + } +}
diff --git a/proto/BUILD b/proto/BUILD new file mode 100644 index 0000000..4528dcb --- /dev/null +++ b/proto/BUILD
@@ -0,0 +1,10 @@ +proto_library( + name = "cache_proto", + srcs = ["cache.proto"], +) + +java_proto_library( + name = "cache_java_proto", + visibility = ["//visibility:public"], + deps = [":cache_proto"], +)
diff --git a/proto/cache.proto b/proto/cache.proto new file mode 100644 index 0000000..4a84ab1 --- /dev/null +++ b/proto/cache.proto
@@ -0,0 +1,27 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +syntax = "proto3"; + +package gerrit.cache; + +option java_package = "com.google.gerrit.server.cache.proto"; + +// Serialized form of com.google.gerrit.server.change.CHangeKindCacheImpl.Key. +// Next ID: 4 +message ChangeKindKeyProto { + bytes prior = 1; + bytes next = 2; + string strategy_name = 3; +}
diff --git a/tools/eclipse/project.py b/tools/eclipse/project.py index f054fad..a6b0964 100755 --- a/tools/eclipse/project.py +++ b/tools/eclipse/project.py
@@ -145,6 +145,7 @@ doc = make_classpath() src = set() lib = set() + proto = set() gwt_src = set() gwt_lib = set() plugins = set() @@ -170,6 +171,9 @@ # JGit dependency from external repository if 'gerrit-' not in p and 'jgit' in p: lib.add(p) + # Assume any jars in /proto/ are from java_proto_library rules + if '/bin/proto/' in p: + proto.add(p) else: # Don't mess up with Bazel internal test runner dependencies. # When we use Eclipse we rely on it for running the tests @@ -239,6 +243,11 @@ continue classpathentry('lib', j, s) + for p in sorted(proto): + s = p.replace('-fastbuild/bin/proto/lib', '-fastbuild/genfiles/proto/') + s = s.replace('.jar', '-src.jar') + classpathentry('lib', p, s) + for s in sorted(gwt_src): p = path.join(ROOT, s, 'src', 'main', 'java') if path.exists(p):