Merge changes from topic 'notedb-batch-update-prep'

* changes:
  Extract more helper methods into BatchUpdate
  ReviewDbBatchUpdate: Simplify some loop code
  BatchUpdate: Remove protected from static methods
  Expand javadoc for BatchUpdate contexts
  ChangeContext: Remove arg from bumpLastUpdatedOn

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java

@@ -586,7 +586,7 @@ public class GetRelatedIT extends AbstractDaemonTest {
             public boolean updateChange(ChangeContext ctx) throws OrmException {
               PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
               psUtil.setGroups(ctx.getDb(), ctx.getUpdate(psId), ps, ImmutableList.<String>of());
-              ctx.bumpLastUpdatedOn(false);
+              ctx.dontBumpLastUpdatedOn();
               return true;
             }
           });

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

@@ -122,7 +122,7 @@ public class CreateDraftComment implements RestModifyView<RevisionResource, Draf
 
       commentsUtil.putComments(
           ctx.getDb(), ctx.getUpdate(psId), Status.DRAFT, Collections.singleton(comment));
-      ctx.bumpLastUpdatedOn(false);
+      ctx.dontBumpLastUpdatedOn();
       return true;
     }
   }

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

@@ -101,7 +101,7 @@ public class DeleteDraftComment implements RestModifyView<DraftCommentResource,
       Comment c = maybeComment.get();
       setCommentRevId(c, patchListCache, ctx.getChange(), ps);
       commentsUtil.deleteComments(ctx.getDb(), ctx.getUpdate(psId), Collections.singleton(c));
-      ctx.bumpLastUpdatedOn(false);
+      ctx.dontBumpLastUpdatedOn();
       return true;
     }
   }

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

@@ -146,7 +146,7 @@ public class PutDraftComment implements RestModifyView<DraftCommentResource, Dra
           update,
           Status.DRAFT,
           Collections.singleton(update(comment, in, ctx.getWhen())));
-      ctx.bumpLastUpdatedOn(false);
+      ctx.dontBumpLastUpdatedOn();
       return true;
     }
   }

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

@@ -18,18 +18,26 @@ import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
+import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.Account;
 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.CurrentUser;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.validators.OnSubmitValidators;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
+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.inject.Inject;
 import com.google.inject.Module;
@@ -41,6 +49,7 @@ import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.TimeZone;
 import org.eclipse.jgit.lib.BatchRefUpdate;
 import org.eclipse.jgit.lib.ObjectInserter;
@@ -117,7 +126,21 @@ public abstract class BatchUpdate implements AutoCloseable {
     }
   }
 
-  protected static Order getOrder(Collection<? extends BatchUpdate> updates) {
+  static void setRequestIds(
+      Collection<? extends BatchUpdate> updates, @Nullable RequestId requestId) {
+    if (requestId != null) {
+      for (BatchUpdate u : updates) {
+        checkArgument(
+            u.requestId == null || u.requestId == requestId,
+            "refusing to overwrite RequestId %s in update with %s",
+            u.requestId,
+            requestId);
+        u.setRequestId(requestId);
+      }
+    }
+  }
+
+  static Order getOrder(Collection<? extends BatchUpdate> updates) {
     Order o = null;
     for (BatchUpdate u : updates) {
       if (o == null) {
@@ -129,7 +152,7 @@ public abstract class BatchUpdate implements AutoCloseable {
     return o;
   }
 
-  protected static boolean getUpdateChangesInParallel(Collection<? extends BatchUpdate> updates) {
+  static boolean getUpdateChangesInParallel(Collection<? extends BatchUpdate> updates) {
     checkArgument(!updates.isEmpty());
     Boolean p = null;
     for (BatchUpdate u : updates) {
@@ -148,6 +171,28 @@ public abstract class BatchUpdate implements AutoCloseable {
     return p;
   }
 
+  static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException {
+    Throwables.throwIfUnchecked(e);
+
+    // Propagate REST API exceptions thrown by operations; they commonly throw exceptions like
+    // ResourceConflictException to indicate an atomic update failure.
+    Throwables.throwIfInstanceOf(e, UpdateException.class);
+    Throwables.throwIfInstanceOf(e, RestApiException.class);
+
+    // Convert other common non-REST exception types with user-visible messages to corresponding
+    // REST exception types
+    if (e instanceof InvalidChangeOperationException) {
+      throw new ResourceConflictException(e.getMessage(), e);
+    } else if (e instanceof NoSuchChangeException
+        || e instanceof NoSuchRefException
+        || e instanceof NoSuchProjectException) {
+      throw new ResourceNotFoundException(e.getMessage(), e);
+    }
+
+    // Otherwise, wrap in a generic UpdateException, which does not include a user-visible message.
+    throw new UpdateException(e);
+  }
+
   protected GitRepositoryManager repoManager;
 
   protected final Project.NameKey project;
@@ -198,7 +243,9 @@ public abstract class BatchUpdate implements AutoCloseable {
   public abstract void execute(BatchUpdateListener listener)
       throws UpdateException, RestApiException;
 
-  public abstract void execute() throws UpdateException, RestApiException;
+  public void execute() throws UpdateException, RestApiException {
+    execute(BatchUpdateListener.NONE);
+  }
 
   protected abstract Context newContext();
 
@@ -251,6 +298,12 @@ public abstract class BatchUpdate implements AutoCloseable {
     return user;
   }
 
+  protected Optional<Account> getAccount() {
+    return user.isIdentifiedUser()
+        ? Optional.of(user.asIdentifiedUser().getAccount())
+        : Optional.empty();
+  }
+
   protected Repository getRepository() throws IOException {
     initRepository();
     return repo;

gerrit-server/src/main/java/com/google/gerrit/server/update/ChangeContext.java

@@ -44,17 +44,24 @@ public interface ChangeContext extends Context {
   ChangeUpdate getUpdate(PatchSet.Id psId);
 
   /**
-   * @return control for this change. The user will be the same as {@link #getUser()}, and the
-   *     change data is read within the same transaction that {@code updateChange} is executing.
+   * Get the control for this change, encapsulating the user and up-to-date change data.
+   *
+   * <p>The user will be the same as {@link #getUser()}, and the change data is read within the same
+   * transaction that {@link BatchUpdateOp#updateChange(ChangeContext)} is executing.
+   *
+   * @return control for this change.
    */
   ChangeControl getControl();
 
   /**
-   * @param bump whether to bump the value of {@link Change#getLastUpdatedOn()} field before storing
-   *     to ReviewDb. For NoteDb, the value is always incremented (assuming the update is not
-   *     otherwise a no-op).
+   * Don't bump the value of {@link Change#getLastUpdatedOn()}.
+   *
+   * <p>If called, don't bump the timestamp before storing to ReviewDb. Only has an effect in
+   * ReviewDb, and the only usage should be to match the behavior of NoteDb. Specifically, in NoteDb
+   * the timestamp is updated if and only if the change meta graph is updated, and is not updated
+   * when only drafts are modified.
    */
-  void bumpLastUpdatedOn(boolean bump);
+  void dontBumpLastUpdatedOn();
 
   /**
    * Instruct {@link BatchUpdate} to delete this change.
@@ -63,7 +70,11 @@ public interface ChangeContext extends Context {
    */
   void deleteChange();
 
-  /** @return notes corresponding to {@link #getControl()}. */
+  /**
+   * Get notes corresponding to {@link #getControl()}.
+   *
+   * @return loaded notes instance.
+   */
   default ChangeNotes getNotes() {
     return checkNotNull(getControl().getNotes());
   }

gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java

@@ -33,7 +33,11 @@ import org.eclipse.jgit.revwalk.RevWalk;
  * <p>A single update may span multiple changes, but they all belong to a single repo.
  */
 public interface Context {
-  /** @return the project name this update operates on. */
+  /**
+   * Get the project name this update operates on.
+   *
+   * @return project.
+   */
   Project.NameKey getProject();
 
   /**
@@ -57,50 +61,80 @@ public interface Context {
    */
   RevWalk getRevWalk() throws IOException;
 
-  /** @return the timestamp at which this update takes place. */
+  /**
+   * Get the timestamp at which this update takes place.
+   *
+   * @return timestamp.
+   */
   Timestamp getWhen();
 
   /**
-   * @return the time zone in which this update takes place. In the current implementation, this is
-   *     always the time zone of the server.
+   * Get the time zone in which this update takes place.
+   *
+   * <p>In the current implementation, this is always the time zone of the server.
+   *
+   * @return time zone.
    */
   TimeZone getTimeZone();
 
   /**
-   * @return an open ReviewDb database. Callers should not manage transactions or call mutating
-   *     methods on the Changes table. Mutations on other tables (including other entities in the
-   *     change entity group) are fine.
+   * Get the ReviewDb database.
+   *
+   * <p>Callers should not manage transactions or call mutating methods on the Changes table.
+   * Mutations on other tables (including other entities in the change entity group) are fine.
+   *
+   * @return open database instance.
    */
   ReviewDb getDb();
 
   /**
-   * @return user performing the update. In the current implementation, this is always an {@link
-   *     IdentifiedUser} or {@link com.google.gerrit.server.InternalUser}.
+   * Get the user performing the update.
+   *
+   * <p>In the current implementation, this is always an {@link IdentifiedUser} or {@link
+   * com.google.gerrit.server.InternalUser}.
+   *
+   * @return user.
    */
   CurrentUser getUser();
 
-  /** @return order in which operations are executed in this update. */
+  /**
+   * Get the order in which operations are executed in this update.
+   *
+   * @return order of operations.
+   */
   Order getOrder();
 
   /**
-   * @return identified user performing the update; throws an unchecked exception if the user is not
-   *     an {@link IdentifiedUser}
+   * Get the identified user performing the update.
+   *
+   * <p>Convenience method for {@code getUser().asIdentifiedUser()}.
+   *
+   * @see CurrentUser#asIdentifiedUser()
+   * @return user.
    */
   default IdentifiedUser getIdentifiedUser() {
     return checkNotNull(getUser()).asIdentifiedUser();
   }
 
   /**
-   * @return account of the user performing the update; throws if the user is not an {@link
-   *     IdentifiedUser}
+   * Get the account of the user performing the update.
+   *
+   * <p>Convenience method for {@code getIdentifiedUser().getAccount()}.
+   *
+   * @see CurrentUser#asIdentifiedUser()
+   * @return account.
    */
   default Account getAccount() {
     return getIdentifiedUser().getAccount();
   }
 
   /**
-   * @return account ID of the user performing the update; throws if the user is not an {@link
-   *     IdentifiedUser}
+   * Get the account ID of the user performing the update.
+   *
+   * <p>Convenience method for {@code getUser().getAccountId()}
+   *
+   * @see CurrentUser#getAccountId()
+   * @return account ID.
    */
   default Account.Id getAccountId() {
     return getIdentifiedUser().getAccountId();

gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java

@@ -14,11 +14,11 @@
 
 package com.google.gerrit.server.update;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static java.util.Comparator.comparing;
 import static java.util.concurrent.TimeUnit.NANOSECONDS;
+import static java.util.stream.Collectors.toList;
 
 import com.google.common.base.Stopwatch;
 import com.google.common.base.Throwables;
@@ -30,7 +30,6 @@ import com.google.common.util.concurrent.ListeningExecutorService;
 import com.google.common.util.concurrent.MoreExecutors;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.metrics.Description;
 import com.google.gerrit.metrics.Description.Units;
@@ -59,10 +58,6 @@ 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;
-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.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
@@ -236,8 +231,8 @@ class ReviewDbBatchUpdate extends BatchUpdate {
     }
 
     @Override
-    public void bumpLastUpdatedOn(boolean bump) {
-      bumpLastUpdatedOn = bump;
+    public void dontBumpLastUpdatedOn() {
+      bumpLastUpdatedOn = false;
     }
 
     @Override
@@ -271,16 +266,7 @@ class ReviewDbBatchUpdate extends BatchUpdate {
     if (updates.isEmpty()) {
       return;
     }
-    if (requestId != null) {
-      for (BatchUpdate u : updates) {
-        checkArgument(
-            u.requestId == null || u.requestId == requestId,
-            "refusing to overwrite RequestId %s in update with %s",
-            u.requestId,
-            requestId);
-        u.setRequestId(requestId);
-      }
-    }
+    setRequestIds(updates, requestId);
     try {
       Order order = getOrder(updates);
       boolean updateChangesInParallel = getUpdateChangesInParallel(updates);
@@ -317,45 +303,26 @@ class ReviewDbBatchUpdate extends BatchUpdate {
           throw new IllegalStateException("invalid execution order: " + order);
       }
 
-      List<CheckedFuture<?, IOException>> indexFutures = new ArrayList<>();
-      for (ReviewDbBatchUpdate u : updates) {
-        indexFutures.addAll(u.indexFutures);
-      }
-      ChangeIndexer.allAsList(indexFutures).get();
+      ChangeIndexer.allAsList(
+              updates.stream().flatMap(u -> u.indexFutures.stream()).collect(toList()))
+          .get();
+
+      // Fire ref update events only after all mutations are finished, since callers may assume a
+      // patch set ref being created means the change was created, or a branch advancing meaning
+      // some changes were closed.
+      updates
+          .stream()
+          .filter(u -> u.batchRefUpdate != null)
+          .forEach(
+              u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null)));
 
-      for (ReviewDbBatchUpdate u : updates) {
-        if (u.batchRefUpdate != null) {
-          // Fire ref update events only after all mutations are finished, since
-          // callers may assume a patch set ref being created means the change
-          // was created, or a branch advancing meaning some changes were
-          // closed.
-          u.gitRefUpdated.fire(
-              u.project,
-              u.batchRefUpdate,
-              u.getUser().isIdentifiedUser() ? u.getUser().asIdentifiedUser().getAccount() : null);
-        }
-      }
       if (!dryrun) {
         for (ReviewDbBatchUpdate u : updates) {
           u.executePostOps();
         }
       }
-    } catch (UpdateException | RestApiException e) {
-      // Propagate REST API exceptions thrown by operations; they commonly throw
-      // exceptions like ResourceConflictException to indicate an atomic update
-      // failure.
-      throw e;
-
-      // Convert other common non-REST exception types with user-visible
-      // messages to corresponding REST exception types
-    } catch (InvalidChangeOperationException e) {
-      throw new ResourceConflictException(e.getMessage(), e);
-    } catch (NoSuchChangeException | NoSuchRefException | NoSuchProjectException e) {
-      throw new ResourceNotFoundException(e.getMessage(), e);
-
     } catch (Exception e) {
-      Throwables.throwIfUnchecked(e);
-      throw new UpdateException(e);
+      wrapAndThrowException(e);
     }
   }
 
@@ -410,11 +377,6 @@ class ReviewDbBatchUpdate extends BatchUpdate {
     skewMs = NoteDbChangeState.getReadOnlySkew(cfg);
   }
 
-  @Override
-  public void execute() throws UpdateException, RestApiException {
-    execute(BatchUpdateListener.NONE);
-  }
-
   @Override
   public void execute(BatchUpdateListener listener) throws UpdateException, RestApiException {
     execute(ImmutableList.of(this), listener, requestId, false);