From 38eba5f350ea35086293afbbd53ea2a311a3abef Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 22 Nov 2016 10:05:48 -0500 Subject: [PATCH] Implement staleness checker for index changes As described in I622dbbb3, we use a combination of the change row version, the ref states, and ref patterns stored in the index to check whether the change document stored in the secondary index is up to date with the primary storage. Change-Id: I8d4a63b705527c5b774373f75dbc28ca9fd10158 --- .../server/index/change/StalenessChecker.java | 159 +++++++++++++- .../index/change/StalenessCheckerTest.java | 205 ++++++++++++++++++ 2 files changed, 363 insertions(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java index f341970dad..0c3d89c66d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java @@ -20,25 +20,133 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.joining; import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.HashMultimap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; +import com.google.common.collect.Multimap; import com.google.common.collect.SetMultimap; +import com.google.common.collect.Sets; import com.google.gerrit.common.Nullable; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.index.IndexConfig; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import java.io.IOException; import java.util.List; +import java.util.Optional; +import java.util.Set; import java.util.regex.Pattern; import java.util.stream.StreamSupport; +@Singleton public class StalenessChecker { - public static SetMultimap parseStates( + private static final Logger log = + LoggerFactory.getLogger(StalenessChecker.class); + + private final ImmutableSet FIELDS = ImmutableSet.of( + ChangeField.CHANGE.getName(), + ChangeField.REF_STATE.getName(), + ChangeField.REF_STATE_PATTERN.getName()); + + private final ChangeIndexCollection indexes; + private final GitRepositoryManager repoManager; + private final IndexConfig indexConfig; + private final Provider db; + + @Inject + StalenessChecker( + ChangeIndexCollection indexes, + GitRepositoryManager repoManager, + IndexConfig indexConfig, + Provider db) { + this.indexes = indexes; + this.repoManager = repoManager; + this.indexConfig = indexConfig; + this.db = db; + } + + boolean isStale(Change.Id id) throws IOException, OrmException { + ChangeIndex i = indexes.getSearchIndex(); + if (i == null) { + return false; // No index; caller couldn't do anything if it is stale. + } + if (!i.getSchema().hasField(ChangeField.REF_STATE) + || !i.getSchema().hasField(ChangeField.REF_STATE_PATTERN)) { + return false; // Index version not new enough for this check. + } + + Optional result = i.get( + id, IndexedChangeQuery.createOptions(indexConfig, 0, 1, FIELDS)); + if (!result.isPresent()) { + return true; // Not in index, but caller wants it to be. + } + ChangeData cd = result.get(); + if (reviewDbChangeIsStale( + cd.change(), + ChangeNotes.readOneReviewDbChange(db.get(), cd.getId()))) { + return true; + } + + return isStale(repoManager, id, parseStates(cd), parsePatterns(cd)); + } + + @VisibleForTesting + static boolean isStale(GitRepositoryManager repoManager, + Change.Id id, + SetMultimap states, + Multimap patterns) { + Set projects = + Sets.union(states.keySet(), patterns.keySet()); + + for (Project.NameKey p : projects) { + if (isStale(repoManager, id, p, states, patterns)) { + return true; + } + } + + return false; + } + + @VisibleForTesting + static boolean reviewDbChangeIsStale( + Change indexChange, @Nullable Change reviewDbChange) { + if (reviewDbChange == null) { + return false; // Nothing the caller can do. + } + checkArgument(indexChange.getId().equals(reviewDbChange.getId()), + "mismatched change ID: %s != %s", + indexChange.getId(), reviewDbChange.getId()); + if (PrimaryStorage.of(reviewDbChange) != PrimaryStorage.REVIEW_DB) { + return false; // Not a ReviewDb change, don't check rowVersion. + } + return reviewDbChange.getRowVersion() != indexChange.getRowVersion(); + } + + private SetMultimap parseStates(ChangeData cd) { + return parseStates(cd.getRefStates()); + } + + @VisibleForTesting + static SetMultimap parseStates( Iterable states) { RefState.check(states != null, null); SetMultimap result = HashMultimap.create(); @@ -58,6 +166,11 @@ public class StalenessChecker { return result; } + private Multimap parsePatterns( + ChangeData cd) { + return parsePatterns(cd.getRefStatePatterns()); + } + public static ListMultimap parsePatterns( Iterable patterns) { RefStatePattern.check(patterns != null, null); @@ -75,6 +188,31 @@ public class StalenessChecker { return result; } + private static boolean isStale(GitRepositoryManager repoManager, + Change.Id id, Project.NameKey project, + SetMultimap allStates, + Multimap allPatterns) { + try (Repository repo = repoManager.openRepository(project)) { + Set states = allStates.get(project); + for (RefState state : states) { + if (!state.match(repo)) { + return true; + } + } + for (RefStatePattern pattern : allPatterns.get(project)) { + if (!pattern.match(repo, states)) { + return true; + } + } + return false; + } catch (IOException e) { + log.warn( + String.format("error checking staleness of %s in %s", id, project), + e); + return true; + } + } + @AutoValue public abstract static class RefState { static RefState create(String ref, String sha) { @@ -106,6 +244,12 @@ public class StalenessChecker { abstract String ref(); abstract ObjectId id(); + + private boolean match(Repository repo) throws IOException { + Ref ref = repo.exactRef(ref()); + ObjectId expected = ref != null ? ref.getObjectId() : ObjectId.zeroId(); + return id().equals(expected); + } } /** @@ -147,5 +291,18 @@ public class StalenessChecker { boolean match(String refName) { return regex().matcher(refName).find(); } + + private boolean match(Repository repo, Set expected) + throws IOException { + for (Ref r : repo.getRefDatabase().getRefs(prefix()).values()) { + if (!match(r.getName())) { + continue; + } + if (!expected.contains(RefState.of(r))) { + return false; + } + } + return true; + } } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java index 4c86fa1a96..cdd1e07891 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java @@ -16,17 +16,30 @@ package com.google.gerrit.server.index.change; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assert_; +import static com.google.gerrit.server.index.change.StalenessChecker.isStale; +import static com.google.gerrit.testutil.TestChanges.newChange; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.toList; +import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.ListMultimap; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.index.change.StalenessChecker.RefState; import com.google.gerrit.server.index.change.StalenessChecker.RefStatePattern; +import com.google.gerrit.server.notedb.NoteDbChangeState; import com.google.gerrit.testutil.GerritBaseTests; +import com.google.gerrit.testutil.InMemoryRepositoryManager; +import com.google.gwtorm.protobuf.CodecFactory; +import com.google.gwtorm.protobuf.ProtobufCodec; +import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; +import org.junit.Before; import org.junit.Test; import java.util.stream.Stream; @@ -38,6 +51,26 @@ public class StalenessCheckerTest extends GerritBaseTests { private static final Project.NameKey P1 = new Project.NameKey("project1"); private static final Project.NameKey P2 = new Project.NameKey("project2"); + private static final Change.Id C = new Change.Id(1234); + + private static final ProtobufCodec CHANGE_CODEC = + CodecFactory.encoder(Change.class); + + private GitRepositoryManager repoManager; + private Repository r1; + private Repository r2; + private TestRepository tr1; + private TestRepository tr2; + + @Before + public void setUp() throws Exception { + repoManager = new InMemoryRepositoryManager(); + r1 = repoManager.createRepository(P1); + tr1 = new TestRepository<>(r1); + r2 = repoManager.createRepository(P2); + tr2 = new TestRepository<>(r2); + } + @Test public void parseStates() { assertInvalidState(null); @@ -147,8 +180,180 @@ public class StalenessCheckerTest extends GerritBaseTests { } } + @Test + public void isStaleRefStatesOnly() throws Exception { + String ref1 = "refs/heads/foo"; + ObjectId id1 = tr1.update(ref1, tr1.commit().message("commit 1")); + String ref2 = "refs/heads/bar"; + ObjectId id2 = tr2.update(ref2, tr2.commit().message("commit 2")); + + // Not stale. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P2, RefState.create(ref2, id2.name())), + ImmutableMultimap.of())) + .isFalse(); + + // Wrong ref value. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, SHA1), + P2, RefState.create(ref2, id2.name())), + ImmutableMultimap.of())) + .isTrue(); + + // Swapped repos. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id2.name()), + P2, RefState.create(ref2, id1.name())), + ImmutableMultimap.of())) + .isTrue(); + + // Two refs in same repo, not stale. + String ref3 = "refs/heads/baz"; + ObjectId id3 = tr1.update(ref3, tr1.commit().message("commit 3")); + tr1.update(ref3, id3); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref3, id3.name())), + ImmutableMultimap.of())) + .isFalse(); + + // Ignore ref not mentioned. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of())) + .isFalse(); + + // One ref wrong. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref3, SHA1)), + ImmutableMultimap.of())) + .isTrue(); + } + + @Test + public void isStaleWithRefStatePatterns() throws Exception { + String ref1 = "refs/heads/foo"; + ObjectId id1 = tr1.update(ref1, tr1.commit().message("commit 1")); + + // ref1 is only ref matching pattern. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/heads/*")))) + .isFalse(); + + // Now ref2 matches pattern, so stale unless ref2 is present in state map. + String ref2 = "refs/heads/bar"; + ObjectId id2 = tr1.update(ref2, tr1.commit().message("commit 2")); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/heads/*")))) + .isTrue(); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref2, id2.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/heads/*")))) + .isFalse(); + } + + @Test + public void isStaleWithNonPrefixPattern() throws Exception { + String ref1 = "refs/heads/foo"; + ObjectId id1 = tr1.update(ref1, tr1.commit().message("commit 1")); + tr1.update("refs/heads/bar", tr1.commit().message("commit 2")); + + // ref1 is only ref matching pattern. + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/*/foo")))) + .isFalse(); + + // Now ref2 matches pattern, so stale unless ref2 is present in state map. + String ref3 = "refs/other/foo"; + ObjectId id3 = tr1.update(ref3, tr1.commit().message("commit 3")); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/*/foo")))) + .isTrue(); + assertThat( + isStale( + repoManager, C, + ImmutableSetMultimap.of( + P1, RefState.create(ref1, id1.name()), + P1, RefState.create(ref3, id3.name())), + ImmutableMultimap.of( + P1, RefStatePattern.create("refs/*/foo")))) + .isFalse(); + } + + @Test + public void reviewDbChangeIsStale() throws Exception { + Change indexChange = newChange(P1, new Account.Id(1)); + indexChange.setNoteDbState(SHA1); + + assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, null)) + .isFalse(); + + Change noteDbPrimary = clone(indexChange); + noteDbPrimary.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE); + assertThat( + StalenessChecker.reviewDbChangeIsStale(indexChange, noteDbPrimary)) + .isFalse(); + + assertThat( + StalenessChecker.reviewDbChangeIsStale( + indexChange, clone(indexChange))) + .isFalse(); + + // Can't easily change row version to check true case. + } + private static Iterable byteArrays(String... strs) { return Stream.of(strs).map(s -> s != null ? s.getBytes(UTF_8) : null) .collect(toList()); } + + private static Change clone(Change change) { + return CHANGE_CODEC.decode(CHANGE_CODEC.encodeToByteArray(change)); + } + }