StalenessChecker: Change is stale if primary storage doesn't match
After running PrimaryStorageMigrator on a change, the note_db_state
field changes, but the change is not automatically reindexed. The idea
was that the change should be detected as stale--after all, its
row_version and note_db_state changed. Unfortunately, the implementation
of StalenessChecker doesn't actually flag a change as stale in this
case, because it ignores row_version entirely when the change is NoteDb
primary.
Worse, the now-stale value in the index still doesn't include the change
meta ref in the ref_state field, since that ref isn't recorded when
ReviewDb is primary, so we aren't able to check for staleness using the
NoteDb state either.
Break this stalemate by considering a change as stale if its
PrimaryStorage in the index differs between the version of the Change
located in primary storage and the version in the index.
Change-Id: I3c5d9981bf6e8a465509d94cac13c66e8368bdbe
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 872dfaf..07e0203 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
@@ -16,6 +16,7 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.joining;
@@ -135,15 +136,24 @@
@VisibleForTesting
static boolean reviewDbChangeIsStale(Change indexChange, @Nullable Change reviewDbChange) {
+ checkNotNull(indexChange);
+ PrimaryStorage storageFromIndex = PrimaryStorage.of(indexChange);
+ PrimaryStorage storageFromReviewDb = PrimaryStorage.of(reviewDbChange);
if (reviewDbChange == null) {
- return false; // Nothing the caller can do.
+ if (storageFromIndex == PrimaryStorage.REVIEW_DB) {
+ return true; // Index says it should have been in ReviewDb, but it wasn't.
+ }
+ return false; // Not in ReviewDb, but that's ok.
}
checkArgument(
indexChange.getId().equals(reviewDbChange.getId()),
"mismatched change ID: %s != %s",
indexChange.getId(),
reviewDbChange.getId());
- if (PrimaryStorage.of(reviewDbChange) != PrimaryStorage.REVIEW_DB) {
+ if (storageFromIndex != storageFromReviewDb) {
+ return true; // Primary storage differs, definitely stale.
+ }
+ if (storageFromReviewDb != PrimaryStorage.REVIEW_DB) {
return false; // Not a ReviewDb change, don't check rowVersion.
}
return reviewDbChange.getRowVersion() != indexChange.getRowVersion();
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 4eef629..0af642d 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
@@ -321,11 +321,13 @@
Change indexChange = newChange(P1, new Account.Id(1));
indexChange.setNoteDbState(SHA1);
- assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, null)).isFalse();
+ // Change is missing from ReviewDb but present in index.
+ assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, null)).isTrue();
+ // Change differs only in primary storage.
Change noteDbPrimary = clone(indexChange);
noteDbPrimary.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE);
- assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, noteDbPrimary)).isFalse();
+ assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, noteDbPrimary)).isTrue();
assertThat(StalenessChecker.reviewDbChangeIsStale(indexChange, clone(indexChange))).isFalse();
