Support treating NoteDb as the source of truth

Handle the new enum value PrimaryStorage.NOTE_DB. When updating a
change, if its state indicates NoteDb is the source of truth, we don't
have to bother writing to ReviewDb, including to update the
NoteDbChangeState. This means that NoteDb and ReviewDb will get out of
sync, and we would need a separate migration process to copy data from
NoteDb back to ReviewDb, not yet implemented.

Many code changes here come from the fact that various handlers
manually delete entities from ReviewDb, where the keys may now have
originally been read from NoteDb. Since we now don't write new
entities to ReviewDb, trying to delete non-existent keys will fail
with OrmConcurrencyException. In fact the transaction would have been
rolled back later in BatchUpdate, so the delete would never really
take effect, but unfortunately gwtorm actually throws at the call site
of delete, prior to committing the transaction.

This change supports changes in in the database with PrimaryStorage ==
NOTE_DB, but does not yet implement a migration path. In the new
acceptance tests, we just set the state manually.

Change-Id: If177aa0b9666f9d14869552c661d5aa8b86ead3b

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java

@@ -0,0 +1,194 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.acceptance.server.notedb;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
+import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
+
+import static java.util.stream.Collectors.toList;
+
+import com.google.common.collect.Iterables;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.extensions.api.changes.DraftInput;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ChangeStatus;
+import com.google.gerrit.extensions.common.ApprovalInfo;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.server.ReviewDbUtil;
+import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.git.RepoRefCache;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.NoteDbChangeState;
+import com.google.gerrit.testutil.NoteDbMode;
+import com.google.inject.Inject;
+
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+
+public class NoteDbPrimaryIT extends AbstractDaemonTest {
+  @Inject
+  private AllUsersName allUsers;
+
+  @Before
+  public void setUp() throws Exception {
+    assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.READ_WRITE);
+    db = ReviewDbUtil.unwrapDb(db);
+  }
+
+  @Test
+  public void updateChange() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getChange().getId();
+    setNoteDbPrimary(id);
+
+    gApi.changes().id(id.get()).current().review(ReviewInput.approve());
+    gApi.changes().id(id.get()).current().submit();
+
+    ChangeInfo info = gApi.changes().id(id.get()).get();
+    assertThat(info.status).isEqualTo(ChangeStatus.MERGED);
+    ApprovalInfo approval =
+        Iterables.getOnlyElement(info.labels.get("Code-Review").all);
+    assertThat(approval._accountId).isEqualTo(admin.id.get());
+    assertThat(approval.value).isEqualTo(2);
+    assertThat(info.messages).hasSize(3);
+    assertThat(Iterables.getLast(info.messages).message)
+        .isEqualTo("Change has been successfully merged by " + admin.fullName);
+
+    ChangeNotes notes = notesFactory.create(db, project, id);
+    assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.MERGED);
+    assertThat(notes.getChange().getNoteDbState())
+        .isEqualTo(NoteDbChangeState.NOTE_DB_PRIMARY_STATE);
+
+    // Writes weren't reflected in ReviewDb.
+    assertThat(db.changes().get(id).getStatus()).isEqualTo(Change.Status.NEW);
+    assertThat(db.patchSetApprovals().byChange(id)).isEmpty();
+    assertThat(db.changeMessages().byChange(id)).hasSize(1);
+  }
+
+  @Test
+  public void deleteDraftComment() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getChange().getId();
+    setNoteDbPrimary(id);
+
+    DraftInput din = new DraftInput();
+    din.path = PushOneCommit.FILE_NAME;
+    din.line = 1;
+    din.message = "A comment";
+    gApi.changes().id(id.get()).current().createDraft(din);
+
+    CommentInfo di = Iterables.getOnlyElement(
+        gApi.changes().id(id.get()).current().drafts()
+            .get(PushOneCommit.FILE_NAME));
+    assertThat(di.message).isEqualTo(din.message);
+
+    assertThat(
+            db.patchComments().draftByChangeFileAuthor(id, din.path, admin.id))
+        .isEmpty();
+
+    gApi.changes().id(id.get()).current().draft(di.id).delete();
+    assertThat(gApi.changes().id(id.get()).current().drafts()).isEmpty();
+  }
+
+  @Test
+  public void deleteVote() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getChange().getId();
+    setNoteDbPrimary(id);
+
+    gApi.changes().id(id.get()).current().review(ReviewInput.approve());
+    List<ApprovalInfo> approvals =
+        gApi.changes().id(id.get()).get().labels.get("Code-Review").all;
+    assertThat(approvals).hasSize(1);
+    assertThat(approvals.get(0).value).isEqualTo(2);
+
+    gApi.changes().id(id.get()).reviewer(admin.id.toString())
+        .deleteVote("Code-Review");
+
+    approvals = gApi.changes().id(id.get()).get().labels.get("Code-Review").all;
+    assertThat(approvals).hasSize(1);
+    assertThat(approvals.get(0).value).isEqualTo(0);
+  }
+
+  @Test
+  public void deleteVoteViaReview() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getChange().getId();
+    setNoteDbPrimary(id);
+
+    gApi.changes().id(id.get()).current().review(ReviewInput.approve());
+    List<ApprovalInfo> approvals =
+        gApi.changes().id(id.get()).get().labels.get("Code-Review").all;
+    assertThat(approvals).hasSize(1);
+    assertThat(approvals.get(0).value).isEqualTo(2);
+
+    gApi.changes().id(id.get()).current().review(ReviewInput.noScore());
+
+    approvals = gApi.changes().id(id.get()).get().labels.get("Code-Review").all;
+    assertThat(approvals).hasSize(1);
+    assertThat(approvals.get(0).value).isEqualTo(0);
+  }
+
+  @Test
+  public void deleteReviewer() throws Exception {
+    PushOneCommit.Result r = createChange();
+    Change.Id id = r.getChange().getId();
+    setNoteDbPrimary(id);
+
+    gApi.changes().id(id.get()).addReviewer(user.id.toString());
+    assertThat(getReviewers(id)).containsExactly(user.id);
+    gApi.changes().id(id.get()).reviewer(user.id.toString()).remove();
+    assertThat(getReviewers(id)).isEmpty();
+  }
+
+  private void setNoteDbPrimary(Change.Id id) throws Exception {
+    Change c = db.changes().get(id);
+    assertThat(c).named("change " + id).isNotNull();
+    NoteDbChangeState state = NoteDbChangeState.parse(c);
+    assertThat(state.getPrimaryStorage())
+        .named("storage of " + id)
+        .isEqualTo(REVIEW_DB);
+
+    try (Repository changeRepo = repoManager.openRepository(c.getProject());
+        Repository allUsersRepo = repoManager.openRepository(allUsers)) {
+      assertThat(
+              state.isUpToDate(
+                  new RepoRefCache(changeRepo), new RepoRefCache(allUsersRepo)))
+          .named("change " + id + " up to date")
+          .isTrue();
+    }
+
+    c.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE);
+    db.changes().update(Collections.singleton(c));
+  }
+
+  private List<Account.Id> getReviewers(Change.Id id) throws Exception {
+    return gApi.changes().id(id.get()).get()
+        .reviewers.values().stream()
+        .flatMap(Collection::stream)
+        .map(a -> new Account.Id(a._accountId))
+        .collect(toList());
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/CommentsUtil.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server;
 
 import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
 import static java.util.stream.Collectors.toList;
 
 import com.google.common.collect.ComparisonChain;
@@ -42,6 +43,7 @@ import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.patch.PatchListNotAvailableException;
@@ -357,9 +359,12 @@ public class CommentsUtil {
     for (Comment c : comments) {
       update.deleteComment(c);
     }
+    if (PrimaryStorage.of(update.getChange()) == REVIEW_DB) {
+      // Avoid OrmConcurrencyException trying to delete non-existent entities.
       db.patchComments().delete(toPatchLineComments(update.getId(),
           PatchLineComment.Status.DRAFT, comments));
     }
+  }
 
   public void deleteAllDraftsFromAllUsers(Change.Id changeId)
       throws IOException {

gerrit-server/src/main/java/com/google/gerrit/server/PatchSetUtil.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
 import static com.google.gerrit.server.notedb.PatchSetState.DRAFT;
 import static com.google.gerrit.server.notedb.PatchSetState.PUBLISHED;
 
@@ -27,6 +28,7 @@ import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.notedb.PatchSetState;
 import com.google.gwtorm.server.OrmException;
@@ -126,8 +128,11 @@ public class PatchSetUtil {
     checkArgument(ps.isDraft(),
         "cannot delete non-draft patch set %s", ps.getId());
     update.setPatchSetState(PatchSetState.DELETED);
+    if (PrimaryStorage.of(update.getChange()) == REVIEW_DB) {
+      // Avoid OrmConcurrencyException trying to delete non-existent entities.
       db.patchSets().delete(Collections.singleton(ps));
     }
+  }
 
   private void ensurePatchSetMatches(PatchSet.Id psId, ChangeUpdate update) {
     Change.Id changeId = update.getChange().getId();

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.change;
 
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
 
 import com.google.gerrit.extensions.registration.DynamicItem;
 import com.google.gerrit.extensions.restapi.AuthException;
@@ -32,6 +33,7 @@ import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.BatchUpdate.RepoContext;
 import com.google.gerrit.server.git.BatchUpdateReviewDb;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -153,6 +155,10 @@ class DeleteChangeOp extends BatchUpdate.Op {
 
   private void deleteChangeElementsFromDb(ChangeContext ctx, Change.Id id)
       throws OrmException {
+    if (PrimaryStorage.of(ctx.getChange()) != REVIEW_DB) {
+      return;
+    }
+    // Avoid OrmConcurrencyException trying to delete non-existent entities.
     // Only delete from ReviewDb here; deletion from NoteDb is handled in
     // BatchUpdate.
     ReviewDb db = unwrap(ctx.getDb());

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.change;
 
+import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
+
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.extensions.registration.DynamicItem;
 import com.google.gerrit.extensions.restapi.AuthException;
@@ -34,6 +36,7 @@ import com.google.gerrit.server.git.BatchUpdate;
 import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
 import com.google.gerrit.server.git.BatchUpdate.RepoContext;
 import com.google.gerrit.server.git.UpdateException;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
 import com.google.gerrit.server.project.NoSuchChangeException;
@@ -146,12 +149,15 @@ public class DeleteDraftPatchSet implements RestModifyView<RevisionResource, Inp
       psUtil.delete(ctx.getDb(), ctx.getUpdate(patchSet.getId()), patchSet);
 
       accountPatchReviewStore.get().clearReviewed(psId);
+      if (PrimaryStorage.of(ctx.getChange()) == REVIEW_DB) {
+        // Avoid OrmConcurrencyException trying to delete non-existent entities.
         // Use the unwrap from DeleteChangeOp to handle BatchUpdateReviewDb.
         ReviewDb db = DeleteChangeOp.unwrap(ctx.getDb());
         db.changeMessages().delete(db.changeMessages().byPatchSet(psId));
         db.patchComments().delete(db.patchComments().byPatchSet(psId));
         db.patchSetApprovals().delete(db.patchSetApprovals().byPatchSet(psId));
       }
+    }
 
     private void deleteOrUpdateDraftChange(ChangeContext ctx)
         throws OrmException, RestApiException, IOException,

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.server.change;
 
+import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
+
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.gerrit.common.TimeUtil;
@@ -46,6 +48,7 @@ import com.google.gerrit.server.git.BatchUpdateReviewDb;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.mail.send.DeleteReviewerSender;
 import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -179,8 +182,10 @@ public class DeleteReviewer
       } else {
         msg.append(".");
       }
-
+      if (PrimaryStorage.of(ctx.getChange()) == REVIEW_DB) {
+        // Avoid OrmConcurrencyException trying to update non-existent entities.
         ctx.getDb().patchSetApprovals().delete(del);
+      }
       ChangeUpdate update = ctx.getUpdate(currPs.getId());
       update.removeReviewer(reviewerId);
 
@@ -208,8 +213,10 @@ public class DeleteReviewer
         Account.Id accountId) throws OrmException {
       Change.Id changeId = ctx.getNotes().getChangeId();
       Iterable<PatchSetApproval> approvals;
+      PrimaryStorage r = PrimaryStorage.of(ctx.getChange());
 
-      if (migration.readChanges()) {
+      if (migration.readChanges()
+          && r == PrimaryStorage.REVIEW_DB) {
         // Because NoteDb and ReviewDb have different semantics for zero-value
         // approvals, we must fall back to ReviewDb as the source of truth here.
         ReviewDb db = ctx.getDb();

gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteVote.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.change;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
 
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.common.data.LabelTypes;
@@ -44,6 +45,7 @@ import com.google.gerrit.server.git.BatchUpdate.Context;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.mail.send.DeleteVoteSender;
 import com.google.gerrit.server.mail.send.ReplyToChangeSender;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.util.LabelVote;
 import com.google.gwtorm.server.OrmException;
@@ -166,8 +168,11 @@ public class DeleteVote
       }
 
       ctx.getUpdate(psId).removeApprovalFor(accountId, label);
+      if (PrimaryStorage.of(ctx.getChange()) == REVIEW_DB) {
+        // Avoid OrmConcurrencyException trying to update non-existent entities.
         ctx.getDb().patchSetApprovals().upsert(
             Collections.singleton(deletedApproval(ctx)));
+      }
 
       StringBuilder msg = new StringBuilder();
       msg.append("Removed ");

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.change;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.CommentsUtil.setCommentRevId;
+import static com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage.REVIEW_DB;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.stream.Collectors.joining;
@@ -83,6 +84,7 @@ import com.google.gerrit.server.git.BatchUpdate.Context;
 import com.google.gerrit.server.git.UpdateException;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.project.ChangeControl;
@@ -833,8 +835,11 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
       }
 
       forceCallerAsReviewer(ctx, current, ups, del);
+      if (PrimaryStorage.of(update.getChange()) == REVIEW_DB) {
+        // Avoid OrmConcurrencyException trying to delete non-existent entities.
         ctx.getDb().patchSetApprovals().delete(del);
         ctx.getDb().patchSetApprovals().upsert(ups);
+      }
       return !del.isEmpty() || !ups.isEmpty();
     }
 

gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java

@@ -53,7 +53,9 @@ import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.index.change.ChangeIndexer;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager;
+import com.google.gerrit.server.notedb.NoteDbUpdateManager.MismatchedStateException;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
@@ -61,7 +63,6 @@ import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.NoSuchProjectException;
 import com.google.gerrit.server.project.NoSuchRefException;
 import com.google.gerrit.server.util.RequestId;
-import com.google.gwtorm.server.OrmConcurrencyException;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
@@ -937,10 +938,17 @@ public class BatchUpdate implements AutoCloseable {
       @SuppressWarnings("resource") // Not always opened.
       NoteDbUpdateManager updateManager = null;
       try {
-        ChangeContext ctx;
+        PrimaryStorage storage;
         db.changes().beginTransaction(id);
         try {
-          ctx = newChangeContext(db, repo, rw, id);
+          ChangeContext ctx = newChangeContext(db, repo, rw, id);
+          storage = PrimaryStorage.of(ctx.getChange());
+          if (storage == PrimaryStorage.NOTE_DB
+              && !notesMigration.readChanges()) {
+            throw new OrmException(
+                "must have NoteDb enabled to update change " + id);
+          }
+
           // Call updateChange on each op.
           logDebug("Calling updateChange on {} ops", changeOps.size());
           for (Op op : changeOps) {
@@ -960,9 +968,12 @@ public class BatchUpdate implements AutoCloseable {
             updateManager = stageNoteDbUpdate(ctx, deleted);
           }
 
-          // Bump lastUpdatedOn or rowVersion and commit.
+          if (storage == PrimaryStorage.REVIEW_DB) {
+            // If primary storage of this change is in ReviewDb, bump
+            // lastUpdatedOn or rowVersion and commit. Otherwise, don't waste
+            // time updating ReviewDb at all.
             Iterable<Change> cs = changesToUpdate(ctx);
-          if (newChanges.containsKey(id)) {
+            if (isNewChange(id)) {
               // Insert rather than upsert in case of a race on change IDs.
               logDebug("Inserting change");
               db.changes().insert(cs);
@@ -976,18 +987,26 @@ public class BatchUpdate implements AutoCloseable {
             if (!dryrun) {
               db.commit();
             }
+          } else {
+            logDebug(
+                "Skipping ReviewDb write since primary storage is {}", storage);
+          }
         } finally {
           db.rollback();
         }
 
-        if (notesMigration.commitChangeWrites()) {
-          try {
         // Do not execute the NoteDbUpdateManager, as we don't want too much
-            // contention on the underlying repo, and we would rather use a
-            // single ObjectInserter/BatchRefUpdate later.
+        // contention on the underlying repo, and we would rather use a single
+        // ObjectInserter/BatchRefUpdate later.
         //
-            // TODO(dborowitz): May or may not be worth trying to batch
-            // together flushed inserters as well.
+        // TODO(dborowitz): May or may not be worth trying to batch together
+        // flushed inserters as well.
+        if (storage == PrimaryStorage.NOTE_DB) {
+          // Should have failed above if NoteDb is disabled.
+          checkState(notesMigration.commitChangeWrites());
+          noteDbResult = updateManager.stage().get(id);
+        } else if (notesMigration.commitChangeWrites()) {
+          try {
             noteDbResult = updateManager.stage().get(id);
           } catch (IOException ex) {
             // Ignore all errors trying to update NoteDb at this point. We've
@@ -1033,20 +1052,29 @@ public class BatchUpdate implements AutoCloseable {
       for (ChangeUpdate u : ctx.updates.values()) {
         updateManager.add(u);
       }
+
+      Change c = ctx.getChange();
       if (deleted) {
-        updateManager.deleteChange(ctx.getChange().getId());
+        updateManager.deleteChange(c.getId());
       }
       try {
-        updateManager.stageAndApplyDelta(ctx.getChange());
-      } catch (OrmConcurrencyException ex) {
-        // Refused to apply update because NoteDb was out of sync. Go ahead with
-        // this ReviewDb update; it's still out of sync, but this is no worse
-        // than before, and it will eventually get rebuilt.
-        logDebug("Ignoring OrmConcurrencyException while staging");
+        updateManager.stageAndApplyDelta(c);
+      } catch (MismatchedStateException ex) {
+        // Refused to apply update because NoteDb was out of sync, which can
+        // only happen if ReviewDb is the primary storage for this change.
+        //
+        // Go ahead with this ReviewDb update; it's still out of sync, but this
+        // is no worse than before, and it will eventually get rebuilt.
+        logDebug("Ignoring MismatchedStateException while staging");
       }
+
       return updateManager;
     }
 
+    private boolean isNewChange(Change.Id id) {
+      return newChanges.containsKey(id);
+    }
+
     private void logDebug(String msg, Throwable t) {
       if (log.isDebugEnabled()) {
         BatchUpdate.this.logDebug("[" + taskId + "]" + msg, t);

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

@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -109,16 +110,20 @@ public abstract class AbstractChangeNotes<T> {
   }
 
   protected final Args args;
+  protected final PrimaryStorage primaryStorage;
   protected final boolean autoRebuild;
   private final Change.Id changeId;
 
   private ObjectId revision;
   private boolean loaded;
 
-  AbstractChangeNotes(Args args, Change.Id changeId, boolean autoRebuild) {
+  AbstractChangeNotes(Args args, Change.Id changeId,
+      @Nullable PrimaryStorage primaryStorage, boolean autoRebuild) {
     this.args = checkNotNull(args);
     this.changeId = checkNotNull(changeId);
-    this.autoRebuild = autoRebuild;
+    this.primaryStorage = primaryStorage;
+    this.autoRebuild = primaryStorage == PrimaryStorage.REVIEW_DB
+        && autoRebuild;
   }
 
   public Change.Id getChangeId() {
@@ -135,6 +140,9 @@ public abstract class AbstractChangeNotes<T> {
       return self();
     }
     boolean read = args.migration.readChanges();
+    if (!read && primaryStorage == PrimaryStorage.NOTE_DB) {
+      throw new OrmException("NoteDb is required to read change " + changeId);
+    }
     boolean readOrWrite = read || args.migration.writeChanges();
     if (!readOrWrite && !autoRebuild) {
       loadDefaults();

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

@@ -98,16 +98,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   public static Change readOneReviewDbChange(ReviewDb db, Change.Id id)
       throws OrmException {
-    return checkNoteDbState(ReviewDbUtil.unwrapDb(db).changes().get(id));
-  }
-
-  private static Change checkNoteDbState(Change c) throws OrmException {
-    NoteDbChangeState s = NoteDbChangeState.parse(c);
-    if (s != null && s.getPrimaryStorage() != PrimaryStorage.REVIEW_DB) {
-      throw new OrmException(
-          "invalid NoteDbChangeState in " + c.getId() + ": " + s);
-    }
-    return c;
+    return ReviewDbUtil.unwrapDb(db).changes().get(id);
   }
 
   @Singleton
@@ -276,7 +267,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
         }
       } else {
         for (Change change : ReviewDbUtil.unwrapDb(db).changes().all()) {
-          checkNoteDbState(change);
           ChangeNotes notes = createFromChangeOnlyWhenNoteDbDisabled(change);
           if (predicate.test(notes)) {
             m.put(change.getProject(), notes);
@@ -373,7 +363,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
   private ChangeNotes(Args args, Change change, boolean autoRebuild,
       @Nullable RefCache refs) {
-    super(args, change.getId(), autoRebuild);
+    super(args, change.getId(), PrimaryStorage.of(change), autoRebuild);
     this.change = new Change(change);
     this.refs = refs;
   }

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

@@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.Comment;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.ReviewerStatusUpdate;
@@ -141,7 +142,10 @@ public abstract class ChangeNotesState {
     abstract Timestamp createdOn();
     abstract Timestamp lastUpdatedOn();
     abstract Account.Id owner();
-    abstract String branch(); // Project not included.
+
+    // Project not included, as it's not stored anywhere in the meta ref.
+    abstract String branch();
+
     @Nullable abstract PatchSet.Id currentPatchSetId();
     abstract String subject();
     @Nullable abstract String topic();
@@ -172,17 +176,35 @@ public abstract class ChangeNotesState {
       changeMessagesByPatchSet();
   abstract ImmutableListMultimap<RevId, Comment> publishedComments();
 
+  Change newChange(Project.NameKey project) {
+    ChangeColumns c = checkNotNull(columns(), "columns are required");
+    Change change = new Change(
+        c.changeKey(),
+        changeId(),
+        c.owner(),
+        new Branch.NameKey(project, c.branch()),
+        c.createdOn());
+    copyNonConstructorColumnsTo(change);
+    change.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE);
+    return change;
+  }
+
   void copyColumnsTo(Change change) {
-    ChangeColumns c = checkNotNull(columns());
+    ChangeColumns c = checkNotNull(columns(), "columns are required");
+    change.setKey(c.changeKey());
+    change.setOwner(c.owner());
+    change.setDest(new Branch.NameKey(change.getProject(), c.branch()));
+    change.setCreatedOn(c.createdOn());
+    copyNonConstructorColumnsTo(change);
+  }
+
+  private void copyNonConstructorColumnsTo(Change change) {
+    ChangeColumns c = checkNotNull(columns(), "columns are required");
     if (c.status() != null) {
       change.setStatus(c.status());
     }
-    change.setKey(c.changeKey());
-    change.setDest(new Branch.NameKey(change.getProject(), c.branch()));
     change.setTopic(Strings.emptyToNull(c.topic()));
-    change.setCreatedOn(c.createdOn());
     change.setLastUpdatedOn(c.lastUpdatedOn());
-    change.setOwner(c.owner());
     change.setSubmissionId(c.submissionId());
     change.setAssignee(c.assignee());
 

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

@@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.git.RepoRefCache;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager.StagedResult;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder;
 import com.google.gerrit.server.project.NoSuchChangeException;
@@ -85,7 +86,9 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
       Args args,
       @Assisted Change.Id changeId,
       @Assisted Account.Id author) {
-    super(args, changeId, true);
+    // PrimaryStorage is unknown; this should only called by
+    // PatchLineCommentsUtil#draftByAuthor, which can live with this.
+    super(args, changeId, null, false);
     this.change = null;
     this.author = author;
     this.rebuildResult = null;
@@ -97,7 +100,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
       Account.Id author,
       boolean autoRebuild,
       NoteDbUpdateManager.Result rebuildResult) {
-    super(args, change.getId(), autoRebuild);
+    super(args, change.getId(), PrimaryStorage.of(change), autoRebuild);
     this.change = change;
     this.author = author;
     this.rebuildResult = rebuildResult;

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

@@ -34,12 +34,14 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
 import com.google.gerrit.server.git.RefCache;
+
+import org.eclipse.jgit.lib.ObjectId;
+
 import java.io.IOException;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import org.eclipse.jgit.lib.ObjectId;
 
 /**
  * The state of all relevant NoteDb refs across all repos corresponding to a
@@ -61,19 +63,21 @@ public class NoteDbChangeState {
   public static final String NOTE_DB_PRIMARY_STATE = "N";
 
   public enum PrimaryStorage {
-    REVIEW_DB('R', true),
-    NOTE_DB('N', false);
+    REVIEW_DB('R'),
+    NOTE_DB('N');
 
     private final char code;
-    private final boolean writeToReviewDb;
 
-    private PrimaryStorage(char code, boolean writeToReviewDb) {
+    private PrimaryStorage(char code) {
       this.code = code;
-      this.writeToReviewDb = writeToReviewDb;
     }
 
-    public boolean writeToReviewDb() {
-      return writeToReviewDb;
+    public static PrimaryStorage of(Change c) {
+      return of(NoteDbChangeState.parse(c));
+    }
+
+    public static PrimaryStorage of(NoteDbChangeState s) {
+      return s != null ? s.getPrimaryStorage() : REVIEW_DB;
     }
   }
 
@@ -232,6 +236,9 @@ public class NoteDbChangeState {
 
   public static boolean isChangeUpToDate(@Nullable NoteDbChangeState state,
       RefCache changeRepoRefs, Change.Id changeId) throws IOException {
+    if (PrimaryStorage.of(state) == NOTE_DB) {
+      return true; // Primary storage is NoteDb, up to date by definition.
+    }
     if (state == null) {
       return !changeRepoRefs.get(changeMetaRef(changeId)).isPresent();
     }
@@ -241,6 +248,9 @@ public class NoteDbChangeState {
   public static boolean areDraftsUpToDate(@Nullable NoteDbChangeState state,
       RefCache draftsRepoRefs, Change.Id changeId, Account.Id accountId)
       throws IOException {
+    if (PrimaryStorage.of(state) == NOTE_DB) {
+      return true; // Primary storage is NoteDb, up to date by definition.
+    }
     if (state == null) {
       return !draftsRepoRefs.get(refsDraftComments(changeId, accountId))
           .isPresent();
@@ -284,6 +294,9 @@ public class NoteDbChangeState {
   }
 
   public boolean isChangeUpToDate(RefCache changeRepoRefs) throws IOException {
+    if (primaryStorage == NOTE_DB) {
+      return true; // Primary storage is NoteDb, up to date by definition.
+    }
     Optional<ObjectId> id = changeRepoRefs.get(changeMetaRef(changeId));
     if (!id.isPresent()) {
       return getChangeMetaId().equals(ObjectId.zeroId());
@@ -293,6 +306,9 @@ public class NoteDbChangeState {
 
   public boolean areDraftsUpToDate(RefCache draftsRepoRefs, Account.Id accountId)
       throws IOException {
+    if (primaryStorage == NOTE_DB) {
+      return true; // Primary storage is NoteDb, up to date by definition.
+    }
     Optional<ObjectId> id =
         draftsRepoRefs.get(refsDraftComments(changeId, accountId));
     if (!id.isPresent()) {
@@ -303,6 +319,9 @@ public class NoteDbChangeState {
 
   public boolean isUpToDate(RefCache changeRepoRefs, RefCache draftsRepoRefs)
       throws IOException {
+    if (primaryStorage == NOTE_DB) {
+      return true; // Primary storage is NoteDb, up to date by definition.
+    }
     if (!isChangeUpToDate(changeRepoRefs)) {
       return false;
     }

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

@@ -37,6 +37,7 @@ import com.google.gerrit.server.git.ChainedReceiveCommands;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.InMemoryInserter;
 import com.google.gerrit.server.git.InsertedObject;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gwtorm.server.OrmConcurrencyException;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.assistedinject.Assisted;
@@ -489,6 +490,17 @@ public class NoteDbUpdateManager implements AutoCloseable {
     }
   }
 
+  public static class MismatchedStateException extends OrmException {
+    private static final long serialVersionUID = 1L;
+
+    private MismatchedStateException(Change.Id id, NoteDbChangeState expectedState) {
+      super(String.format(
+          "cannot apply NoteDb updates for change %s;"
+          + " change meta ref does not match %s",
+          id, expectedState.getChangeMetaId().name()));
+    }
+  }
+
   private void checkExpectedState() throws OrmException, IOException {
     if (!checkExpectedState) {
       return;
@@ -513,15 +525,17 @@ public class NoteDbUpdateManager implements AutoCloseable {
         //  - We short-circuited before adding any commands that update this
         //    ref, and we won't stage a delta for this change either.
         // Either way, it is safe to proceed here rather than throwing
-        // OrmConcurrencyException.
+        // MismatchedStateException.
+        continue;
+      }
+
+      if (expectedState.getPrimaryStorage() == PrimaryStorage.NOTE_DB) {
+        // NoteDb is primary, no need to compare state to ReviewDb.
         continue;
       }
 
       if (!expectedState.isChangeUpToDate(changeRepo.cmds.getRepoRefCache())) {
-        throw new OrmConcurrencyException(String.format(
-            "cannot apply NoteDb updates for change %s;"
-            + " change meta ref does not match %s",
-            u.getId(), expectedState.getChangeMetaId().name()));
+        throw new MismatchedStateException(u.getId(), expectedState);
       }
     }
 
@@ -529,7 +543,8 @@ public class NoteDbUpdateManager implements AutoCloseable {
       ChangeDraftUpdate u = us.iterator().next();
       NoteDbChangeState expectedState = NoteDbChangeState.parse(u.getChange());
 
-      if (expectedState == null) {
+      if (expectedState == null
+          || expectedState.getPrimaryStorage() == PrimaryStorage.NOTE_DB) {
         continue; // See above.
       }
 
@@ -539,7 +554,8 @@ public class NoteDbUpdateManager implements AutoCloseable {
         throw new OrmConcurrencyException(String.format(
             "cannot apply NoteDb updates for change %s;"
             + " draft ref for account %s does not match %s",
-            u.getId(), accountId, expectedState.getChangeMetaId().name()));
+            u.getId(), accountId,
+            expectedState.getChangeMetaId().name()));
       }
     }
   }

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

@@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.client.RobotComment;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.inject.assistedinject.Assisted;
 import com.google.inject.assistedinject.AssistedInject;
 
@@ -47,7 +48,7 @@ public class RobotCommentNotes extends AbstractChangeNotes<RobotCommentNotes> {
   RobotCommentNotes(
       Args args,
       @Assisted Change change) {
-    super(args, change.getId(), false);
+    super(args, change.getId(), PrimaryStorage.of(change), false);
     this.change = change;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java

@@ -57,6 +57,7 @@ import com.google.gerrit.server.notedb.ChangeNoteUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.notedb.NoteDbChangeState;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager.OpenRepo;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result;
@@ -185,7 +186,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
   public NoteDbUpdateManager stage(ReviewDb db, Change.Id changeId)
       throws NoSuchChangeException, IOException, OrmException {
     db = ReviewDbUtil.unwrapDb(db);
-    Change change = ChangeNotes.readOneReviewDbChange(db, changeId);
+    Change change =
+        checkNoteDbState(ChangeNotes.readOneReviewDbChange(db, changeId));
     if (change == null) {
       throw new NoSuchChangeException(changeId);
     }
@@ -201,7 +203,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
       NoteDbUpdateManager manager) throws NoSuchChangeException, OrmException,
       IOException {
     db = ReviewDbUtil.unwrapDb(db);
-    Change change = ChangeNotes.readOneReviewDbChange(db, changeId);
+    Change change =
+        checkNoteDbState(ChangeNotes.readOneReviewDbChange(db, changeId));
     if (change == null) {
       throw new NoSuchChangeException(changeId);
     }
@@ -254,6 +257,16 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     return r;
   }
 
+  private static Change checkNoteDbState(Change c) throws OrmException {
+    // Can only rebuild a change if its primary storage is ReviewDb.
+    NoteDbChangeState s = NoteDbChangeState.parse(c);
+    if (s != null && s.getPrimaryStorage() != PrimaryStorage.REVIEW_DB) {
+      throw new OrmException(String.format(
+          "cannot rebuild change " + c.getId() + " with state " + s));
+    }
+    return c;
+  }
+
   @Override
   public void buildUpdates(NoteDbUpdateManager manager, ChangeBundle bundle)
       throws IOException, OrmException {