opendev/gerrit
Code Issues Proposed changes

StalenessChecker: Change is stale if primary storage doesn't match

Browse Source 
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
This commit is contained in:
Dave Borowitz
2017-04-04 11:32:22 -04:00
parent 28dd2ae37c
commit 98cd9ea59b
2 changed files with 16 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.index.change;
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 @@ public class StalenessChecker {
@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();

6
gerrit-server/src/test/java/com/google/gerrit/server/index/change/StalenessCheckerTest.java
View File

@@ -321,11 +321,13 @@ public class StalenessCheckerTest extends GerritBaseTests {
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();