Merge changes from topic 'stale-after-migration'

* changes:
  Never autoRebuild when ReviewDb is disabled for changes
  StalenessChecker: Change is stale if primary storage doesn't match
  ChangeNotes: Mark readOneReviewDbChange @Nullable
  NoteDbChangeState: Mark some params @Nullable

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

@@ -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();

gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java

@@ -123,7 +123,10 @@ public abstract class AbstractChangeNotes<T> {
     this.args = checkNotNull(args);
     this.changeId = checkNotNull(changeId);
     this.primaryStorage = primaryStorage;
-    this.autoRebuild = primaryStorage == PrimaryStorage.REVIEW_DB && autoRebuild;
+    this.autoRebuild =
+        primaryStorage == PrimaryStorage.REVIEW_DB
+            && !args.migration.disableChangeReviewDb()
+            && autoRebuild;
   }
 
   public Change.Id getChangeId() {

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -95,6 +95,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return new ConfigInvalidException("Change " + changeId + ": " + String.format(fmt, args));
   }
 
+  @Nullable
   public static Change readOneReviewDbChange(ReviewDb db, Change.Id id) throws OrmException {
     return ReviewDbUtil.unwrapDb(db).changes().get(id);
   }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java

@@ -78,11 +78,11 @@ public class NoteDbChangeState {
       this.code = code;
     }
 
-    public static PrimaryStorage of(Change c) {
+    public static PrimaryStorage of(@Nullable Change c) {
       return of(NoteDbChangeState.parse(c));
     }
 
-    public static PrimaryStorage of(NoteDbChangeState s) {
+    public static PrimaryStorage of(@Nullable NoteDbChangeState s) {
       return s != null ? s.getPrimaryStorage() : REVIEW_DB;
     }
   }
@@ -150,12 +150,12 @@ public class NoteDbChangeState {
     }
   }
 
-  public static NoteDbChangeState parse(Change c) {
+  public static NoteDbChangeState parse(@Nullable Change c) {
     return c != null ? parse(c.getId(), c.getNoteDbState()) : null;
   }
 
   @VisibleForTesting
-  public static NoteDbChangeState parse(Change.Id id, String str) {
+  public static NoteDbChangeState parse(Change.Id id, @Nullable String str) {
     if (Strings.isNullOrEmpty(str)) {
       // Return null rather than Optional as this is what goes in the field in
       // ReviewDb.

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

@@ -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();