Merge changes from topics 'submodule-op-cleanup', 'retry-config'

* changes:
  SubmoduleOp: Preserve exception from constructor
  SubmoduleOp: TODO format cleanup
  SubmoduleException: Minor cleanup
  RetryHelper: Make time limits configurable
  RetryHelper: Disable retrying when updates are non-idempotent

Documentation/dev-note-db.txt

@@ -146,3 +146,34 @@ Here, the migration process looks like:
 - Run a Flume to migrate all existing changes to NoteDb primary. (Also
   closed-source, but basically just a wrapper around `PrimaryStorageMigrator`.)
 - Turn off access to ReviewDb changes tables.
+
+== Configuration
+
+This section describes general configuration for the NoteDb backend that is not
+directly related to the migration process. These options will continue to be
+supported after the migration is complete and the migration-related options are
+obsolete.
+
+[[noteDb.retryMaxWait]]noteDb.retryMaxWait::
++
+Maximum time to wait between attempts to retry update operations when one
+attempt fails due to contention (aka lock failure) on the underlying ref
+storage. Operations are retried with exponential backoff, plus some random
+jitter, until the interval reaches this limit. After that, retries continue to
+occur after a fixed timeout (plus jitter), up to
+link:#noteDb.retryTimeout[`noteDb.retryTimeout`].
++
+Only applicable when `noteDb.changes.fuseUpdates=true`.
++
+Defaults to 5 seconds; unit suffixes are supported, and assumes milliseconds if
+not specified.
+
+[[noteDb.retryTimeout]]noteDb.retryTimeout::
++
+Total timeout for retrying update operations when one attempt fails due to
+contention (aka lock failure) on the underlying ref storage.
++
+Only applicable when `noteDb.changes.fuseUpdates=true`.
++
+Defaults to 20 seconds; unit suffixes are supported, and assumes milliseconds if
+not specified.

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

@@ -36,6 +36,7 @@ import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.RepoContext;
 import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.testutil.ConfigSuite;
 import com.google.inject.Inject;
 import java.io.IOException;
 import java.util.Collections;
@@ -45,6 +46,7 @@ import java.util.Optional;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
@@ -58,6 +60,14 @@ import org.junit.Before;
 import org.junit.Test;
 
 public class NoteDbOnlyIT extends AbstractDaemonTest {
+  @ConfigSuite.Default
+  public static Config defaultConfig() {
+    Config cfg = new Config();
+    // Avoid spurious timeouts during intentional retries due to overloaded test machines.
+    cfg.setString("noteDb", null, "retryTimeout", Integer.MAX_VALUE + "s");
+    return cfg;
+  }
+
   @Inject private RetryHelper retryHelper;
 
   @Before

gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -120,7 +120,6 @@ import com.google.gerrit.server.git.MergedByPushOp;
 import com.google.gerrit.server.git.NotesBranchUtil;
 import com.google.gerrit.server.git.ReceivePackInitializer;
 import com.google.gerrit.server.git.ReplaceOp;
-import com.google.gerrit.server.git.SubmoduleOp;
 import com.google.gerrit.server.git.TagCache;
 import com.google.gerrit.server.git.TransferConfig;
 import com.google.gerrit.server.git.strategy.SubmitStrategy;
@@ -399,7 +398,6 @@ public class GerritGlobalModule extends FactoryModule {
     factory(MergedByPushOp.Factory.class);
     factory(GitModules.Factory.class);
     factory(VersionedAuthorizedKeys.Factory.class);
-    factory(SubmoduleOp.Factory.class);
 
     bind(AccountManager.class);
     factory(ChangeUserName.Factory.class);

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

@@ -460,8 +460,8 @@ public class MergeOp implements AutoCloseable {
     }
     // Done checks that don't involve running submit strategies.
     commitStatus.maybeFailVerbose();
-    SubmoduleOp submoduleOp = subOpFactory.create(branches, orm);
     try {
+      SubmoduleOp submoduleOp = subOpFactory.create(branches, orm);
       List<SubmitStrategy> strategies = getSubmitStrategies(toSubmit, submoduleOp, dryrun);
       this.allProjects = submoduleOp.getProjectsInOrder();
       batchUpdateFactory.execute(

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

@@ -14,15 +14,19 @@
 
 package com.google.gerrit.server.git;
 
-/** Indicates the gitlink's update cannot be processed at this time. */
+/**
+ * Indicates the gitlink's update cannot be processed at this time.
+ *
+ * <p>Message should be considered user-visible.
+ */
 public class SubmoduleException extends Exception {
   private static final long serialVersionUID = 1L;
 
-  SubmoduleException(final String msg) {
+  SubmoduleException(String msg) {
     super(msg, null);
   }
 
-  SubmoduleException(final String msg, final Throwable why) {
+  SubmoduleException(String msg, Throwable why) {
     super(msg, why);
   }
 }

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

@@ -36,7 +36,7 @@ import com.google.gerrit.server.update.RepoContext;
 import com.google.gerrit.server.update.RepoOnlyOp;
 import com.google.gerrit.server.update.UpdateException;
 import com.google.inject.Inject;
-import com.google.inject.assistedinject.Assisted;
+import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.ArrayDeque;
 import java.util.ArrayList;
@@ -87,8 +87,43 @@ public class SubmoduleOp {
     }
   }
 
-  public interface Factory {
-    SubmoduleOp create(Set<Branch.NameKey> updatedBranches, MergeOpRepoManager orm);
+  @Singleton
+  public static class Factory {
+    private final GitModules.Factory gitmodulesFactory;
+    private final PersonIdent myIdent;
+    private final Config cfg;
+    private final ProjectCache projectCache;
+    private final ProjectState.Factory projectStateFactory;
+    private final BatchUpdate.Factory batchUpdateFactory;
+
+    @Inject
+    Factory(
+        GitModules.Factory gitmodulesFactory,
+        @GerritPersonIdent PersonIdent myIdent,
+        @GerritServerConfig Config cfg,
+        ProjectCache projectCache,
+        ProjectState.Factory projectStateFactory,
+        BatchUpdate.Factory batchUpdateFactory) {
+      this.gitmodulesFactory = gitmodulesFactory;
+      this.myIdent = myIdent;
+      this.cfg = cfg;
+      this.projectCache = projectCache;
+      this.projectStateFactory = projectStateFactory;
+      this.batchUpdateFactory = batchUpdateFactory;
+    }
+
+    public SubmoduleOp create(Set<Branch.NameKey> updatedBranches, MergeOpRepoManager orm)
+        throws SubmoduleException {
+      return new SubmoduleOp(
+          gitmodulesFactory,
+          myIdent,
+          cfg,
+          projectCache,
+          projectStateFactory,
+          batchUpdateFactory,
+          updatedBranches,
+          orm);
+    }
   }
 
   private static final Logger log = LoggerFactory.getLogger(SubmoduleOp.class);
@@ -116,16 +151,15 @@ public class SubmoduleOp {
   // map of superproject and its branches which has submodule subscriptions
   private final SetMultimap<Project.NameKey, Branch.NameKey> branchesByProject;
 
-  @Inject
-  public SubmoduleOp(
+  private SubmoduleOp(
       GitModules.Factory gitmodulesFactory,
-      @GerritPersonIdent PersonIdent myIdent,
-      @GerritServerConfig Config cfg,
+      PersonIdent myIdent,
+      Config cfg,
       ProjectCache projectCache,
       ProjectState.Factory projectStateFactory,
       BatchUpdate.Factory batchUpdateFactory,
-      @Assisted Set<Branch.NameKey> updatedBranches,
-      @Assisted MergeOpRepoManager orm)
+      Set<Branch.NameKey> updatedBranches,
+      MergeOpRepoManager orm)
       throws SubmoduleException {
     this.gitmodulesFactory = gitmodulesFactory;
     this.myIdent = myIdent;
@@ -434,7 +468,7 @@ public class SubmoduleOp {
     commit.setTreeId(newTreeId);
     commit.setParentIds(currentCommit.getParents());
     if (verboseSuperProject != VerboseSuperprojectUpdate.FALSE) {
-      //TODO:czhen handle cherrypick footer
+      // TODO(czhen): handle cherrypick footer
       commit.setMessage(currentCommit.getFullMessage() + "\n\n* submodules:\n" + msgbuf.toString());
     } else {
       commit.setMessage(currentCommit.getFullMessage());

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

@@ -14,18 +14,24 @@
 
 package com.google.gerrit.server.update;
 
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
+
 import com.github.rholder.retry.RetryException;
 import com.github.rholder.retry.RetryerBuilder;
 import com.github.rholder.retry.StopStrategies;
+import com.github.rholder.retry.StopStrategy;
 import com.github.rholder.retry.WaitStrategies;
+import com.github.rholder.retry.WaitStrategy;
 import com.google.common.base.Throwables;
 import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.LockFailureException;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.lib.Config;
 
 @Singleton
 public class RetryHelper {
@@ -33,34 +39,51 @@ public class RetryHelper {
     T call(BatchUpdate.Factory updateFactory) throws Exception;
   }
 
+  private final NotesMigration migration;
   private final BatchUpdate.Factory updateFactory;
+  private final StopStrategy stopStrategy;
+  private final WaitStrategy waitStrategy;
 
   @Inject
   RetryHelper(
+      @GerritServerConfig Config cfg,
       NotesMigration migration,
       ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory,
       FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory,
       UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory) {
+    this.migration = migration;
     this.updateFactory =
         new BatchUpdate.Factory(
             migration,
             reviewDbBatchUpdateFactory,
             fusedNoteDbBatchUpdateFactory,
             unfusedNoteDbBatchUpdateFactory);
+    this.stopStrategy =
+        StopStrategies.stopAfterDelay(
+            cfg.getTimeUnit("noteDb", null, "retryTimeout", SECONDS.toMillis(5), MILLISECONDS),
+            MILLISECONDS);
+    this.waitStrategy =
+        WaitStrategies.join(
+            WaitStrategies.exponentialWait(
+                cfg.getTimeUnit("noteDb", null, "retryMaxWait", SECONDS.toMillis(20), MILLISECONDS),
+                MILLISECONDS),
+            WaitStrategies.randomWait(50, MILLISECONDS));
   }
 
   public <T> T execute(Action<T> action) throws RestApiException, UpdateException {
     try {
-      // TODO(dborowitz): Make configurable.
-      return RetryerBuilder.<T>newBuilder()
-          .withStopStrategy(StopStrategies.stopAfterDelay(20, TimeUnit.SECONDS))
-          .withWaitStrategy(
-              WaitStrategies.join(
-                  WaitStrategies.exponentialWait(5, TimeUnit.SECONDS),
-                  WaitStrategies.randomWait(50, TimeUnit.MILLISECONDS)))
-          .retryIfException(RetryHelper::isLockFailure)
-          .build()
-          .call(() -> action.call(updateFactory));
+      RetryerBuilder<T> builder = RetryerBuilder.newBuilder();
+      if (migration.disableChangeReviewDb() && migration.fuseUpdates()) {
+        builder
+            .withStopStrategy(stopStrategy)
+            .withWaitStrategy(waitStrategy)
+            .retryIfException(RetryHelper::isLockFailure);
+      } else {
+        // Either we aren't full-NoteDb, or the underlying ref storage doesn't support atomic
+        // transactions. Either way, retrying a partially-failed operation is not idempotent, so
+        // don't do it automatically. Let the end user decide whether they want to retry.
+      }
+      return builder.build().call(() -> action.call(updateFactory));
     } catch (ExecutionException | RetryException e) {
       if (e.getCause() != null) {
         Throwables.throwIfInstanceOf(e.getCause(), UpdateException.class);