From c15b8ef404e2c934550bb19917966b4168b9c4b6 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 18 May 2018 08:11:22 -0400 Subject: [PATCH] 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 --- .../server/query/change/ConflictKey.java | 106 ++++++++++-------- .../server/query/change/ConflictsCache.java | 2 +- .../query/change/ConflictsCacheImpl.java | 9 +- .../query/change/ConflictsPredicate.java | 10 +- .../google/gerrit/server/query/change/BUILD | 3 + .../server/query/change/ConflictKeyTest.java | 56 ++++++++- proto/cache.proto | 9 ++ 7 files changed, 137 insertions(+), 58 deletions(-) diff --git a/java/com/google/gerrit/server/query/change/ConflictKey.java b/java/com/google/gerrit/server/query/change/ConflictKey.java index 7af0058c44..9daf886d21 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 { + INSTANCE; + + private static final Converter 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 e8b2fef89e..c7ee79b9db 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 @@ import com.google.gerrit.common.Nullable; 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 11856770f4..0b8c5ee3c9 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 @@ public class ConflictsCacheImpl implements ConflictsCache { 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 @@ public class ConflictsCacheImpl implements ConflictsCache { } @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 f870951b68..7dc7a0b080 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 @@ public class ConflictsPredicate { 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 6d55cd76ec..09e3243e8b 100644 --- a/javatests/com/google/gerrit/server/query/change/BUILD +++ b/javatests/com/google/gerrit/server/query/change/BUILD @@ -62,10 +62,13 @@ junit_tests( "//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 2cfb652cce..b87bbf73c0 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 @@ public class ConflictKeyTest { 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 @@ public class ConflictKeyTest { 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 7e2e75a801..a826f8cc6e 100644 --- a/proto/cache.proto +++ b/proto/cache.proto @@ -184,3 +184,12 @@ message ChangeNotesStateProto { 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; +}