Serialize ConflictsCache as proto
Convert ConflictKey to AutoValue as well. Rewrite the normalization
logic and add comments to make the intent clearer.
Since boolean serialization doesn't support nulls, change the
ConflictsCache interface to use primitive boolean in the put method.
Inspecting the only caller in ConflictsCacheImpl shows that put can
never be called with null.
Change-Id: I293cffba7609362dabb93e48c1929a05cd0c76fc
diff --git a/java/com/google/gerrit/server/query/change/ConflictKey.java b/java/com/google/gerrit/server/query/change/ConflictKey.java
index 7af0058..9daf886 100644
--- a/java/com/google/gerrit/server/query/change/ConflictKey.java
+++ b/java/com/google/gerrit/server/query/change/ConflictKey.java
@@ -14,68 +14,80 @@
package com.google.gerrit.server.query.change;
+import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Converter;
+import com.google.common.base.Enums;
+import com.google.common.collect.Ordering;
import com.google.gerrit.extensions.client.SubmitType;
-import java.io.Serializable;
-import java.util.Objects;
+import com.google.gerrit.server.cache.CacheSerializer;
+import com.google.gerrit.server.cache.ProtoCacheSerializers;
+import com.google.gerrit.server.cache.ProtoCacheSerializers.ObjectIdConverter;
+import com.google.gerrit.server.cache.proto.Cache.ConflictKeyProto;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
-public class ConflictKey implements Serializable {
- private static final long serialVersionUID = 2L;
-
+@AutoValue
+public abstract class ConflictKey {
public static ConflictKey create(
AnyObjectId commit, AnyObjectId otherCommit, SubmitType submitType, boolean contentMerge) {
- return new ConflictKey(commit.copy(), otherCommit.copy(), submitType, contentMerge);
- }
-
- private final ObjectId commit;
- private final ObjectId otherCommit;
- private final SubmitType submitType;
- private final boolean contentMerge;
-
- public ConflictKey(
- ObjectId commit, ObjectId otherCommit, SubmitType submitType, boolean contentMerge) {
- if (SubmitType.FAST_FORWARD_ONLY.equals(submitType) || commit.compareTo(otherCommit) < 0) {
- this.commit = commit;
- this.otherCommit = otherCommit;
- } else {
- this.commit = otherCommit;
- this.otherCommit = commit;
+ ObjectId commitCopy = commit.copy();
+ ObjectId otherCommitCopy = otherCommit.copy();
+ if (submitType == SubmitType.FAST_FORWARD_ONLY) {
+ // The conflict check for FF-only is non-symmetrical, and we need to treat (X, Y) differently
+ // from (Y, X). Store the commits in the input order.
+ return new AutoValue_ConflictKey(commitCopy, otherCommitCopy, submitType, contentMerge);
}
- this.submitType = submitType;
- this.contentMerge = contentMerge;
+ // Otherwise, the check is symmetrical; sort commit/otherCommit before storing, so the actual
+ // key is independent of the order in which they are passed to this method.
+ return new AutoValue_ConflictKey(
+ Ordering.natural().min(commitCopy, otherCommitCopy),
+ Ordering.natural().max(commitCopy, otherCommitCopy),
+ submitType,
+ contentMerge);
}
- public ObjectId getCommit() {
- return commit;
+ @VisibleForTesting
+ static ConflictKey createWithoutNormalization(
+ AnyObjectId commit, AnyObjectId otherCommit, SubmitType submitType, boolean contentMerge) {
+ return new AutoValue_ConflictKey(commit.copy(), otherCommit.copy(), submitType, contentMerge);
}
- public ObjectId getOtherCommit() {
- return otherCommit;
- }
+ public abstract ObjectId commit();
- public SubmitType getSubmitType() {
- return submitType;
- }
+ public abstract ObjectId otherCommit();
- public boolean isContentMerge() {
- return contentMerge;
- }
+ public abstract SubmitType submitType();
- @Override
- public boolean equals(Object o) {
- if (!(o instanceof ConflictKey)) {
- return false;
+ public abstract boolean contentMerge();
+
+ public static enum Serializer implements CacheSerializer<ConflictKey> {
+ INSTANCE;
+
+ private static final Converter<String, SubmitType> SUBMIT_TYPE_CONVERTER =
+ Enums.stringConverter(SubmitType.class);
+
+ @Override
+ public byte[] serialize(ConflictKey object) {
+ ObjectIdConverter idConverter = ObjectIdConverter.create();
+ return ProtoCacheSerializers.toByteArray(
+ ConflictKeyProto.newBuilder()
+ .setCommit(idConverter.toByteString(object.commit()))
+ .setOtherCommit(idConverter.toByteString(object.otherCommit()))
+ .setSubmitType(SUBMIT_TYPE_CONVERTER.reverse().convert(object.submitType()))
+ .setContentMerge(object.contentMerge())
+ .build());
}
- ConflictKey other = (ConflictKey) o;
- return commit.equals(other.commit)
- && otherCommit.equals(other.otherCommit)
- && submitType.equals(other.submitType)
- && contentMerge == other.contentMerge;
- }
- @Override
- public int hashCode() {
- return Objects.hash(commit, otherCommit, submitType, contentMerge);
+ @Override
+ public ConflictKey deserialize(byte[] in) {
+ ConflictKeyProto proto = ProtoCacheSerializers.parseUnchecked(ConflictKeyProto.parser(), in);
+ ObjectIdConverter idConverter = ObjectIdConverter.create();
+ return create(
+ idConverter.fromByteString(proto.getCommit()),
+ idConverter.fromByteString(proto.getOtherCommit()),
+ SUBMIT_TYPE_CONVERTER.convert(proto.getSubmitType()),
+ proto.getContentMerge());
+ }
}
}
diff --git a/java/com/google/gerrit/server/query/change/ConflictsCache.java b/java/com/google/gerrit/server/query/change/ConflictsCache.java
index e8b2fef..c7ee79b 100644
--- a/java/com/google/gerrit/server/query/change/ConflictsCache.java
+++ b/java/com/google/gerrit/server/query/change/ConflictsCache.java
@@ -18,7 +18,7 @@
public interface ConflictsCache {
- void put(ConflictKey key, Boolean value);
+ void put(ConflictKey key, boolean value);
@Nullable
Boolean getIfPresent(ConflictKey key);
diff --git a/java/com/google/gerrit/server/query/change/ConflictsCacheImpl.java b/java/com/google/gerrit/server/query/change/ConflictsCacheImpl.java
index 1185677..0b8c5ee 100644
--- a/java/com/google/gerrit/server/query/change/ConflictsCacheImpl.java
+++ b/java/com/google/gerrit/server/query/change/ConflictsCacheImpl.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.query.change;
import com.google.common.cache.Cache;
+import com.google.gerrit.server.cache.BooleanCacheSerializer;
import com.google.gerrit.server.cache.CacheModule;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -29,7 +30,11 @@
return new CacheModule() {
@Override
protected void configure() {
- persist(NAME, ConflictKey.class, Boolean.class).maximumWeight(37400);
+ persist(NAME, ConflictKey.class, Boolean.class)
+ .version(1)
+ .keySerializer(ConflictKey.Serializer.INSTANCE)
+ .valueSerializer(BooleanCacheSerializer.INSTANCE)
+ .maximumWeight(37400);
bind(ConflictsCache.class).to(ConflictsCacheImpl.class);
}
};
@@ -43,7 +48,7 @@
}
@Override
- public void put(ConflictKey key, Boolean value) {
+ public void put(ConflictKey key, boolean value) {
conflictsCache.put(key, value);
}
diff --git a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
index f870951..7dc7a0b 100644
--- a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
@@ -115,19 +115,19 @@
ObjectId other = ObjectId.fromString(object.currentPatchSet().getRevision().get());
ConflictKey conflictsKey =
- new ConflictKey(
+ ConflictKey.create(
changeDataCache.getTestAgainst(),
other,
str.type,
projectState.is(BooleanProjectConfig.USE_CONTENT_MERGE));
- Boolean conflicts = args.conflictsCache.getIfPresent(conflictsKey);
- if (conflicts != null) {
- return conflicts;
+ Boolean maybeConflicts = args.conflictsCache.getIfPresent(conflictsKey);
+ if (maybeConflicts != null) {
+ return maybeConflicts;
}
try (Repository repo = args.repoManager.openRepository(otherChange.getProject());
CodeReviewRevWalk rw = CodeReviewCommit.newRevWalk(repo)) {
- conflicts =
+ boolean conflicts =
!args.submitDryRun.run(
str.type,
repo,
diff --git a/javatests/com/google/gerrit/server/query/change/BUILD b/javatests/com/google/gerrit/server/query/change/BUILD
index 6d55cd7..09e3243 100644
--- a/javatests/com/google/gerrit/server/query/change/BUILD
+++ b/javatests/com/google/gerrit/server/query/change/BUILD
@@ -62,10 +62,13 @@
"//java/com/google/gerrit/extensions:api",
"//java/com/google/gerrit/reviewdb:server",
"//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/server/cache/testing",
"//java/com/google/gerrit/testing:gerrit-test-util",
"//lib:guava",
"//lib:gwtorm",
"//lib/jgit/org.eclipse.jgit:jgit",
"//lib/truth",
+ "//lib/truth:truth-proto-extension",
+ "//proto:cache_java_proto",
],
)
diff --git a/javatests/com/google/gerrit/server/query/change/ConflictKeyTest.java b/javatests/com/google/gerrit/server/query/change/ConflictKeyTest.java
index 2cfb652..b87bbf7 100644
--- a/javatests/com/google/gerrit/server/query/change/ConflictKeyTest.java
+++ b/javatests/com/google/gerrit/server/query/change/ConflictKeyTest.java
@@ -15,9 +15,15 @@
package com.google.gerrit.server.query.change;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
import static com.google.gerrit.extensions.client.SubmitType.FAST_FORWARD_ONLY;
import static com.google.gerrit.extensions.client.SubmitType.MERGE_IF_NECESSARY;
+import static com.google.gerrit.server.cache.testing.CacheSerializerTestUtil.bytes;
+import static com.google.gerrit.server.cache.testing.SerializedClassSubject.assertThatSerializedClass;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.server.cache.proto.Cache.ConflictKeyProto;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
@@ -29,8 +35,10 @@
ConflictKey id1First = ConflictKey.create(id1, id2, FAST_FORWARD_ONLY, true);
ConflictKey id2First = ConflictKey.create(id2, id1, FAST_FORWARD_ONLY, true);
- assertThat(id1First).isEqualTo(new ConflictKey(id1, id2, FAST_FORWARD_ONLY, true));
- assertThat(id2First).isEqualTo(new ConflictKey(id2, id1, FAST_FORWARD_ONLY, true));
+ assertThat(id1First)
+ .isEqualTo(ConflictKey.createWithoutNormalization(id1, id2, FAST_FORWARD_ONLY, true));
+ assertThat(id2First)
+ .isEqualTo(ConflictKey.createWithoutNormalization(id2, id1, FAST_FORWARD_ONLY, true));
assertThat(id1First).isNotEqualTo(id2First);
}
@@ -40,9 +48,51 @@
ObjectId id2 = ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
ConflictKey id1First = ConflictKey.create(id1, id2, MERGE_IF_NECESSARY, true);
ConflictKey id2First = ConflictKey.create(id2, id1, MERGE_IF_NECESSARY, true);
- ConflictKey expected = new ConflictKey(id1, id2, MERGE_IF_NECESSARY, true);
+ ConflictKey expected =
+ ConflictKey.createWithoutNormalization(id1, id2, MERGE_IF_NECESSARY, true);
assertThat(id1First).isEqualTo(expected);
assertThat(id2First).isEqualTo(expected);
}
+
+ @Test
+ public void serializer() throws Exception {
+ ConflictKey key =
+ ConflictKey.create(
+ ObjectId.fromString("badc0feebadc0feebadc0feebadc0feebadc0fee"),
+ ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"),
+ SubmitType.MERGE_IF_NECESSARY,
+ false);
+ byte[] serialized = ConflictKey.Serializer.INSTANCE.serialize(key);
+ assertThat(ConflictKeyProto.parseFrom(serialized))
+ .isEqualTo(
+ ConflictKeyProto.newBuilder()
+ .setCommit(
+ bytes(
+ 0xba, 0xdc, 0x0f, 0xee, 0xba, 0xdc, 0x0f, 0xee, 0xba, 0xdc, 0x0f, 0xee,
+ 0xba, 0xdc, 0x0f, 0xee, 0xba, 0xdc, 0x0f, 0xee))
+ .setOtherCommit(
+ bytes(
+ 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef,
+ 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef))
+ .setSubmitType("MERGE_IF_NECESSARY")
+ .setContentMerge(false)
+ .build());
+ assertThat(ConflictKey.Serializer.INSTANCE.deserialize(serialized)).isEqualTo(key);
+ }
+
+ /**
+ * See {@link com.google.gerrit.server.cache.testing.SerializedClassSubject} for background and
+ * what to do if this test fails.
+ */
+ @Test
+ public void methods() throws Exception {
+ assertThatSerializedClass(ConflictKey.class)
+ .hasAutoValueMethods(
+ ImmutableMap.of(
+ "commit", ObjectId.class,
+ "otherCommit", ObjectId.class,
+ "submitType", SubmitType.class,
+ "contentMerge", boolean.class));
+ }
}
diff --git a/proto/cache.proto b/proto/cache.proto
index 7e2e75a..a826f8c 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -184,3 +184,12 @@
int64 read_only_until = 17;
bool has_read_only_until = 18;
}
+
+
+// Serialized form of com.google.gerrit.server.query.change.ConflictKey
+message ConflictKeyProto {
+ bytes commit = 1;
+ bytes other_commit = 2;
+ string submit_type = 3;
+ bool content_merge = 4;
+}
