Return more structured result from ChangeRebuilder

Refactors ChangeRebuilder and NoteDbUpdateManager to return a new type
encapsulating both the new NoteDbChangeState (which is intentionally
not part of the original staged result) and the underlying staged
result with its possibly-not-yet-written NoteDb data.

Change-Id: I2ff014e9fe01910a2b77b1f5417f1f11dc84b610

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

@@ -44,7 +44,6 @@ 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.InsertedObject;
-import com.google.gerrit.server.notedb.NoteDbChangeState;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.project.ChangeControl;
@@ -874,7 +873,7 @@ public class BatchUpdate implements AutoCloseable {
         updateManager.deleteChange(ctx.getChange().getId());
       }
       try {
-        NoteDbChangeState.applyDelta(ctx.getChange(), updateManager.stage());
+        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

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

@@ -53,6 +53,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.git.RefCache;
 import com.google.gerrit.server.git.RepoRefCache;
+import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -595,7 +596,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     try {
       NoteDbChangeState newState;
       try {
-        newState = args.rebuilder.get().rebuild(args.db.get(), getChangeId());
+        NoteDbUpdateManager.Result r =
+            args.rebuilder.get().rebuild(args.db.get(), getChangeId());
+        newState = r != null ? r.newState() : null;
         repo.scanForRepoChanges();
       } catch (IOException e) {
         newState = recheckUpToDate(repo, e);

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

@@ -20,6 +20,7 @@ import com.google.common.util.concurrent.ListeningExecutorService;
 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.notedb.NoteDbUpdateManager.Result;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
@@ -37,11 +38,11 @@ public abstract class ChangeRebuilder {
     this.schemaFactory = schemaFactory;
   }
 
-  public final ListenableFuture<NoteDbChangeState> rebuildAsync(
+  public final ListenableFuture<Result> rebuildAsync(
       final Change.Id id, ListeningExecutorService executor) {
-    return executor.submit(new Callable<NoteDbChangeState>() {
+    return executor.submit(new Callable<Result>() {
         @Override
-      public NoteDbChangeState call() throws Exception {
+      public Result call() throws Exception {
         try (ReviewDb db = schemaFactory.open()) {
           return rebuild(db, id);
         }
@@ -49,11 +50,11 @@ public abstract class ChangeRebuilder {
     });
   }
 
-  public abstract NoteDbChangeState rebuild(ReviewDb db, Change.Id changeId)
+  public abstract Result rebuild(ReviewDb db, Change.Id changeId)
       throws NoSuchChangeException, IOException, OrmException,
       ConfigInvalidException;
 
-  public abstract NoteDbChangeState rebuild(NoteDbUpdateManager manager,
+  public abstract Result rebuild(NoteDbUpdateManager manager,
       ChangeBundle bundle) throws NoSuchChangeException, IOException,
       OrmException, ConfigInvalidException;
 

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

@@ -57,6 +57,7 @@ import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.git.ChainedReceiveCommands;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager.OpenRepo;
+import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result;
 import com.google.gerrit.server.patch.PatchListCache;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectCache;
@@ -150,7 +151,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
   }
 
   @Override
-  public NoteDbChangeState rebuild(ReviewDb db, Change.Id changeId)
+  public Result rebuild(ReviewDb db, Change.Id changeId)
       throws NoSuchChangeException, IOException, OrmException,
       ConfigInvalidException {
     db = unwrapDb(db);
@@ -164,7 +165,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     return execute(db, changeId, manager);
   }
 
-  private NoteDbChangeState execute(ReviewDb db, Change.Id changeId,
+  private Result execute(ReviewDb db, Change.Id changeId,
       NoteDbUpdateManager manager)
       throws NoSuchChangeException, OrmException, IOException {
     Change change = db.changes().get(changeId);
@@ -173,8 +174,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     }
 
     final String oldNoteDbState = change.getNoteDbState();
-    NoteDbChangeState newState =
+    Result r = manager.stageAndApplyDelta(change);
-        NoteDbChangeState.applyDelta(change, manager.stage());
     final String newNoteDbState = change.getNoteDbState();
     try {
       db.changes().atomicUpdate(changeId, new AtomicUpdate<Change>() {
@@ -191,7 +191,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
     } catch (AbortUpdateException e) {
       // Drop this rebuild; another thread completed it.
     }
-    return newState;
+    return r;
   }
 
   private static class AbortUpdateException extends OrmRuntimeException {
@@ -203,12 +203,12 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
   }
 
   @Override
-  public NoteDbChangeState rebuild(NoteDbUpdateManager manager,
+  public Result rebuild(NoteDbUpdateManager manager,
       ChangeBundle bundle) throws NoSuchChangeException, IOException,
       OrmException, ConfigInvalidException {
     Change change = new Change(bundle.getChange());
     buildUpdates(manager, bundle);
-    return NoteDbChangeState.applyDelta(change, manager.stage());
+    return manager.stageAndApplyDelta(change);
   }
 
   @Override

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

@@ -221,7 +221,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     NoteDbUpdateManager updateManager =
         updateManagerFactory.create(getProjectName());
     updateManager.add(this);
-    NoteDbChangeState.applyDelta(getChange(), updateManager.stage());
+    updateManager.stageAndApplyDelta(getChange());
     updateManager.execute();
     return getResult();
   }

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

@@ -160,12 +160,12 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
 
   private LoadHandle rebuildAndOpen(Repository repo) throws IOException {
     try {
-      NoteDbChangeState newState =
+      NoteDbUpdateManager.Result r =
           args.rebuilder.get().rebuild(args.db.get(), getChangeId());
-      if (newState == null) {
+      if (r == null) {
         return super.openHandle(repo); // May be null in tests.
       }
-      ObjectId draftsId = newState.getDraftIds().get(author);
+      ObjectId draftsId = r.newState().getDraftIds().get(author);
       repo.scanForRepoChanges();
       return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), draftsId);
     } catch (NoSuchChangeException e) {

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

@@ -100,12 +100,6 @@ public class NoteDbChangeState {
     return new NoteDbChangeState(id, changeMetaId, draftIds);
   }
 
-  public static NoteDbChangeState applyDelta(Change change,
-      Map<Change.Id, NoteDbUpdateManager.StagedResult> stagedResults) {
-    NoteDbUpdateManager.StagedResult r = stagedResults.get(change.getId());
-    return applyDelta(change, r != null ? r.delta() : null);
-  }
-
   public static NoteDbChangeState applyDelta(Change change, Delta delta) {
     if (delta == null) {
       return null;

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

@@ -22,17 +22,13 @@ import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Change.Id;
 import com.google.gerrit.reviewdb.client.Project.NameKey;
 import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result;
-import com.google.gwtorm.server.OrmException;
 import com.google.inject.TypeLiteral;
 import com.google.inject.name.Names;
 
-import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Repository;
 
-import java.io.IOException;
-
 public class NoteDbModule extends FactoryModule {
   private final Config cfg;
   private final boolean useTestBindings;
@@ -68,23 +64,20 @@ public class NoteDbModule extends FactoryModule {
     } else {
       bind(ChangeRebuilder.class).toInstance(new ChangeRebuilder(null) {
         @Override
-        public NoteDbChangeState rebuild(ReviewDb db, Change.Id changeId)
+        public Result rebuild(ReviewDb db, Change.Id changeId) {
-            throws OrmException {
           return null;
         }
 
         @Override
-        public NoteDbChangeState rebuild(NoteDbUpdateManager manager,
+        public Result rebuild(NoteDbUpdateManager manager,
-            ChangeBundle bundle) throws NoSuchChangeException, IOException,
+            ChangeBundle bundle) {
-            OrmException, ConfigInvalidException {
           return null;
         }
 
         @Override
         public boolean rebuildProject(ReviewDb db,
             ImmutableMultimap<NameKey, Id> allChanges, NameKey project,
-            Repository allUsersRepo) throws NoSuchChangeException, IOException,
+            Repository allUsersRepo) {
-            OrmException, ConfigInvalidException {
           return false;
         }
       });

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

@@ -103,6 +103,18 @@ public class NoteDbUpdateManager {
     public abstract ImmutableList<InsertedObject> allUsersObjects();
   }
 
+  @AutoValue
+  public abstract static class Result {
+    static Result create(NoteDbUpdateManager.StagedResult staged,
+        NoteDbChangeState newState) {
+      return new AutoValue_NoteDbUpdateManager_Result(newState, staged);
+    }
+
+    @Nullable public abstract NoteDbChangeState newState();
+
+    @Nullable abstract NoteDbUpdateManager.StagedResult staged();
+  }
+
   static class OpenRepo implements AutoCloseable {
     final Repository repo;
     final RevWalk rw;
@@ -332,6 +344,14 @@ public class NoteDbUpdateManager {
     }
   }
 
+  public Result stageAndApplyDelta(Change change)
+      throws OrmException, IOException {
+    StagedResult sr = stage().get(change.getId());
+    NoteDbChangeState newState =
+        NoteDbChangeState.applyDelta(change, sr != null ? sr.delta() : null);
+    return Result.create(sr, newState);
+  }
+
   private Table<Change.Id, Account.Id, ObjectId> getDraftIds() {
     Table<Change.Id, Account.Id, ObjectId> draftIds = HashBasedTable.create();
     if (allUsersRepo == null) {

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

@@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableMultimap;
 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.notedb.NoteDbUpdateManager.Result;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
@@ -50,10 +51,10 @@ public class TestChangeRebuilderWrapper extends ChangeRebuilder {
   }
 
   @Override
-  public NoteDbChangeState rebuild(ReviewDb db, Change.Id changeId)
+  public Result rebuild(ReviewDb db, Change.Id changeId)
       throws NoSuchChangeException, IOException, OrmException,
       ConfigInvalidException {
-    NoteDbChangeState result = delegate.rebuild(db, changeId);
+    Result result = delegate.rebuild(db, changeId);
     if (stealNextUpdate.getAndSet(false)) {
       throw new IOException("Update stolen");
     }
@@ -61,7 +62,7 @@ public class TestChangeRebuilderWrapper extends ChangeRebuilder {
   }
 
   @Override
-  public NoteDbChangeState rebuild(NoteDbUpdateManager manager,
+  public Result rebuild(NoteDbUpdateManager manager,
       ChangeBundle bundle) throws NoSuchChangeException, IOException,
       OrmException, ConfigInvalidException {
     // stealNextUpdate doesn't really apply in this case because the IOException