Reduce boilerplate code to execute index queries with retry

To execute an index query with retry one currently needs to do:

  @Inject RetryHelper retryHelper;
  @Inject Provider<InternalChangeQuery> internalChangeQueryProvider;

  List<ChangeData> changes;
  try {
    changes = retryHelper
      .indexQuery(actionName, () -> internalChangeQueryProvider.get().byProject(project))
      .call();
  } catch (Exception e) {
    Throwables.throwIfUnchecked(e);
    throw new StorageException(e);
  }

The new code removes the need to

* get the InternalQuery injected
* catch and wrap exceptions

  @Inject RetryHelper retryHelper;

  List<ChangeData> changes = retryHelper
      .changeIndexQuery(actionName, q -> q.byProject(project))
      .call();

To make this work we add RetryableIndexQueryAction as a new subclass of
RetryableAction that defines an IndexQueryAction that provides the
InternalQuery and that handles the exceptions on call. This follows the
example of RetryableChangeAction that defines a ChangeAction that
provides a BatchUpdate.Factory and that handles the exceptions on call.

In result we can remove a bunch of executeIndexQuery methods that were
spread around the code base.

Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I9b577a827bf83d9ed5f11d1eac32a37e78063186

java/com/google/gerrit/server/account/Emails.java

@@ -17,21 +17,16 @@ package com.google.gerrit.server.account;
 import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
-import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.MultimapBuilder;
 import com.google.common.collect.SetMultimap;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.UserIdentity;
-import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.server.account.externalids.ExternalId;
 import com.google.gerrit.server.account.externalids.ExternalIds;
-import com.google.gerrit.server.query.account.InternalAccountQuery;
 import com.google.gerrit.server.update.RetryHelper;
-import com.google.gerrit.server.update.RetryableAction.Action;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.sql.Timestamp;
@@ -44,16 +39,11 @@ import org.eclipse.jgit.lib.PersonIdent;
 @Singleton
 public class Emails {
   private final ExternalIds externalIds;
-  private final Provider<InternalAccountQuery> queryProvider;
   private final RetryHelper retryHelper;
 
   @Inject
-  public Emails(
+  public Emails(ExternalIds externalIds, RetryHelper retryHelper) {
-      ExternalIds externalIds,
-      Provider<InternalAccountQuery> queryProvider,
-      RetryHelper retryHelper) {
     this.externalIds = externalIds;
-    this.queryProvider = queryProvider;
     this.retryHelper = retryHelper;
   }
 
@@ -84,9 +74,9 @@ public class Emails {
       return accounts;
     }
 
-    return executeIndexQuery(
+    return retryHelper
-            "queryAccountsByPreferredEmail",
+        .accountIndexQuery("queryAccountsByPreferredEmail", q -> q.byPreferredEmail(email)).call()
-            () -> queryProvider.get().byPreferredEmail(email).stream())
+        .stream()
         .map(a -> a.account().id())
         .collect(toImmutableSet());
   }
@@ -105,9 +95,10 @@ public class Emails {
     List<String> emailsToBackfill =
         Arrays.stream(emails).filter(e -> !result.containsKey(e)).collect(toImmutableList());
     if (!emailsToBackfill.isEmpty()) {
-      executeIndexQuery(
+      retryHelper
-              "queryAccountsByPreferredEmails",
+          .accountIndexQuery(
-              () -> queryProvider.get().byPreferredEmail(emailsToBackfill).entries().stream())
+              "queryAccountsByPreferredEmails", q -> q.byPreferredEmail(emailsToBackfill))
+          .call().entries().stream()
           .forEach(e -> result.put(e.getKey(), e.getValue().account().id()));
     }
     return ImmutableSetMultimap.copyOf(result);
@@ -140,13 +131,4 @@ public class Emails {
 
     return u;
   }
-
-  private <T> T executeIndexQuery(String actionName, Action<T> action) {
-    try {
-      return retryHelper.indexQuery(actionName, action).call();
-    } catch (Exception e) {
-      Throwables.throwIfUnchecked(e);
-      throw new StorageException(e);
-    }
-  }
 }

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -43,7 +43,6 @@ import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_RE
 import com.google.common.base.Joiner;
 import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
-import com.google.common.base.Throwables;
 import com.google.common.collect.BiMap;
 import com.google.common.collect.HashBiMap;
 import com.google.common.collect.ImmutableList;
@@ -167,7 +166,6 @@ import com.google.gerrit.server.update.Context;
 import com.google.gerrit.server.update.RepoContext;
 import com.google.gerrit.server.update.RepoOnlyOp;
 import com.google.gerrit.server.update.RetryHelper;
-import com.google.gerrit.server.update.RetryableAction.Action;
 import com.google.gerrit.server.update.UpdateException;
 import com.google.gerrit.server.util.LabelVote;
 import com.google.gerrit.server.util.MagicBranch;
@@ -3315,9 +3313,11 @@ class ReceiveCommits {
                       for (String changeId : c.getFooterLines(FooterConstants.CHANGE_ID)) {
                         if (byKey == null) {
                           byKey =
-                              executeIndexQuery(
+                              retryHelper
-                                  "queryOpenChangesByKeyByBranch",
+                                  .changeIndexQuery(
-                                  () -> openChangesByKeyByBranch(branch));
+                                      "queryOpenChangesByKeyByBranch",
+                                      q -> openChangesByKeyByBranch(q, branch))
+                                  .call();
                         }
 
                         ChangeNotes onto = byKey.get(Change.key(changeId.trim()));
@@ -3397,20 +3397,12 @@ class ReceiveCommits {
     }
   }
 
-  private <T> T executeIndexQuery(String actionName, Action<T> action) {
+  private Map<Change.Key, ChangeNotes> openChangesByKeyByBranch(
-    try (TraceTimer traceTimer = newTimer("executeIndexQuery")) {
+      InternalChangeQuery internalChangeQuery, BranchNameKey branch) {
-      return retryHelper.indexQuery(actionName, action).call();
-    } catch (Exception e) {
-      Throwables.throwIfUnchecked(e);
-      throw new StorageException(e);
-    }
-  }
-
-  private Map<Change.Key, ChangeNotes> openChangesByKeyByBranch(BranchNameKey branch) {
     try (TraceTimer traceTimer =
         newTimer("openChangesByKeyByBranch", Metadata.builder().branchName(branch.branch()))) {
       Map<Change.Key, ChangeNotes> r = new HashMap<>();
-      for (ChangeData cd : queryProvider.get().byBranchOpen(branch)) {
+      for (ChangeData cd : internalChangeQuery.byBranchOpen(branch)) {
         try {
           r.put(cd.change().getKey(), cd.notes());
         } catch (NoSuchChangeException e) {

java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java

@@ -48,12 +48,10 @@ import com.google.gerrit.server.index.change.ChangeField;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.ChangeIdPredicate;
 import com.google.gerrit.server.query.change.CommitPredicate;
-import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gerrit.server.query.change.ProjectPredicate;
 import com.google.gerrit.server.query.change.RefPredicate;
 import com.google.gerrit.server.update.RetryHelper;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.ArrayList;
@@ -75,7 +73,6 @@ public class ProjectsConsistencyChecker {
 
   private final GitRepositoryManager repoManager;
   private final RetryHelper retryHelper;
-  private final Provider<InternalChangeQuery> changeQueryProvider;
   private final ChangeJson.Factory changeJsonFactory;
   private final IndexConfig indexConfig;
 
@@ -83,12 +80,10 @@ public class ProjectsConsistencyChecker {
   ProjectsConsistencyChecker(
       GitRepositoryManager repoManager,
       RetryHelper retryHelper,
-      Provider<InternalChangeQuery> changeQueryProvider,
       ChangeJson.Factory changeJsonFactory,
       IndexConfig indexConfig) {
     this.repoManager = repoManager;
     this.retryHelper = retryHelper;
-    this.changeQueryProvider = changeQueryProvider;
     this.changeJsonFactory = changeJsonFactory;
     this.indexConfig = indexConfig;
   }
@@ -264,12 +259,10 @@ public class ProjectsConsistencyChecker {
     try {
       List<ChangeData> queryResult =
           retryHelper
-              .indexQuery(
+              .changeIndexQuery(
                   "projectsConsistencyCheckerQueryChanges",
-                  () ->
+                  q ->
-                      changeQueryProvider
+                      q.setRequestedFields(ChangeField.CHANGE, ChangeField.PATCH_SET)
-                          .get()
-                          .setRequestedFields(ChangeField.CHANGE, ChangeField.PATCH_SET)
                           .query(and(basePredicate, or(predicates))))
               .call();
 

java/com/google/gerrit/server/restapi/project/CommitsCollection.java

@@ -16,10 +16,8 @@ package com.google.gerrit.server.restapi.project;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
 
-import com.google.common.base.Throwables;
 import com.google.gerrit.entities.Project;
 import com.google.gerrit.entities.RefNames;
-import com.google.gerrit.exceptions.StorageException;
 import com.google.gerrit.extensions.registration.DynamicMap;
 import com.google.gerrit.extensions.restapi.ChildCollection;
 import com.google.gerrit.extensions.restapi.IdString;
@@ -35,12 +33,9 @@ import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.project.Reachable;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.CommitPredicate;
-import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gerrit.server.query.change.ProjectPredicate;
 import com.google.gerrit.server.update.RetryHelper;
-import com.google.gerrit.server.update.RetryableAction.Action;
 import com.google.inject.Inject;
-import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.io.IOException;
 import java.util.Arrays;
@@ -62,7 +57,6 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
   private final GitRepositoryManager repoManager;
   private final RetryHelper retryHelper;
   private final ChangeIndexCollection indexes;
-  private final Provider<InternalChangeQuery> queryProvider;
   private final Reachable reachable;
 
   @Inject
@@ -71,13 +65,11 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
       GitRepositoryManager repoManager,
       RetryHelper retryHelper,
       ChangeIndexCollection indexes,
-      Provider<InternalChangeQuery> queryProvider,
       Reachable reachable) {
     this.views = views;
     this.repoManager = repoManager;
     this.retryHelper = retryHelper;
     this.indexes = indexes;
-    this.queryProvider = queryProvider;
     this.reachable = reachable;
   }
 
@@ -130,14 +122,11 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
     // Check first if any patchset of any change references the commit in question. This is much
     // cheaper than ref visibility filtering and reachability computation.
     List<ChangeData> changes =
-        executeIndexQuery(
+        retryHelper
-            "queryChangesByProjectCommitWithLimit1",
+            .changeIndexQuery(
-            () ->
+                "queryChangesByProjectCommitWithLimit1",
-                queryProvider
+                q -> q.enforceVisibility(true).setLimit(1).byProjectCommit(project, commit))
-                    .get()
+            .call();
-                    .enforceVisibility(true)
-                    .setLimit(1)
-                    .byProjectCommit(project, commit));
     if (!changes.isEmpty()) {
       return true;
     }
@@ -152,9 +141,10 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
                     .map(parent -> new CommitPredicate(parent.getId().getName()))
                     .collect(toImmutableList())));
     changes =
-        executeIndexQuery(
+        retryHelper
-            "queryChangesByProjectCommit",
+            .changeIndexQuery(
-            () -> queryProvider.get().enforceVisibility(true).query(pred));
+                "queryChangesByProjectCommit", q -> q.enforceVisibility(true).query(pred))
+            .call();
 
     Set<Ref> branchesForCommitParents = new HashSet<>(changes.size());
     for (ChangeData cd : changes) {
@@ -177,13 +167,4 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
             .collect(toImmutableList());
     return reachable.fromRefs(project, repo, commit, refs);
   }
-
-  private <T> T executeIndexQuery(String actionName, Action<T> action) {
-    try {
-      return retryHelper.indexQuery(actionName, action).call();
-    } catch (Exception e) {
-      Throwables.throwIfUnchecked(e);
-      throw new StorageException(e);
-    }
-  }
 }

java/com/google/gerrit/server/update/RetryHelper.java

@@ -42,10 +42,14 @@ import com.google.gerrit.server.logging.Metadata;
 import com.google.gerrit.server.logging.RequestId;
 import com.google.gerrit.server.logging.TraceContext;
 import com.google.gerrit.server.plugincontext.PluginSetContext;
+import com.google.gerrit.server.query.account.InternalAccountQuery;
+import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gerrit.server.update.RetryableAction.Action;
 import com.google.gerrit.server.update.RetryableAction.ActionType;
 import com.google.gerrit.server.update.RetryableChangeAction.ChangeAction;
+import com.google.gerrit.server.update.RetryableIndexQueryAction.IndexQueryAction;
 import com.google.inject.Inject;
+import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import java.time.Duration;
 import java.util.Arrays;
@@ -177,6 +181,8 @@ public class RetryHelper {
 
   private final Metrics metrics;
   private final BatchUpdate.Factory updateFactory;
+  private final Provider<InternalAccountQuery> internalAccountQuery;
+  private final Provider<InternalChangeQuery> internalChangeQuery;
   private final PluginSetContext<ExceptionHook> exceptionHooks;
   private final Map<ActionType, Duration> defaultTimeouts;
   private final WaitStrategy waitStrategy;
@@ -188,8 +194,17 @@ public class RetryHelper {
       @GerritServerConfig Config cfg,
       Metrics metrics,
       PluginSetContext<ExceptionHook> exceptionHooks,
-      BatchUpdate.Factory updateFactory) {
+      BatchUpdate.Factory updateFactory,
-    this(cfg, metrics, updateFactory, exceptionHooks, null);
+      Provider<InternalAccountQuery> internalAccountQuery,
+      Provider<InternalChangeQuery> internalChangeQuery) {
+    this(
+        cfg,
+        metrics,
+        updateFactory,
+        internalAccountQuery,
+        internalChangeQuery,
+        exceptionHooks,
+        null);
   }
 
   @VisibleForTesting
@@ -197,10 +212,14 @@ public class RetryHelper {
       @GerritServerConfig Config cfg,
       Metrics metrics,
       BatchUpdate.Factory updateFactory,
+      Provider<InternalAccountQuery> internalAccountQuery,
+      Provider<InternalChangeQuery> internalChangeQuery,
       PluginSetContext<ExceptionHook> exceptionHooks,
       @Nullable Consumer<RetryerBuilder<?>> overwriteDefaultRetryerStrategySetup) {
     this.metrics = metrics;
     this.updateFactory = updateFactory;
+    this.internalAccountQuery = internalAccountQuery;
+    this.internalChangeQuery = internalChangeQuery;
     this.exceptionHooks = exceptionHooks;
 
     Duration defaultTimeout =
@@ -309,15 +328,37 @@ public class RetryHelper {
   }
 
   /**
-   * Creates an action for querying an index that is executed with retrying when called.
+   * Creates an action for querying the account index that is executed with retrying when called.
+   *
+   * <p>The index query action gets a {@link InternalAccountQuery} provided that can be used to
+   * query the account index.
    *
    * @param actionName the name of the action, used as metric bucket
-   * @param action the action that should be executed
+   * @param indexQueryAction the action that should be executed
-   * @return the retryable action, callers need to call {@link RetryableAction#call()} to execute
+   * @return the retryable action, callers need to call {@link RetryableIndexQueryAction#call()} to
-   *     the action
+   *     execute the action
    */
-  public <T> RetryableAction<T> indexQuery(String actionName, Action<T> action) {
+  public <T> RetryableIndexQueryAction<InternalAccountQuery, T> accountIndexQuery(
-    return new RetryableAction<>(this, ActionType.INDEX_QUERY, actionName, action);
+      String actionName, IndexQueryAction<T, InternalAccountQuery> indexQueryAction) {
+    return new RetryableIndexQueryAction<>(
+        this, internalAccountQuery.get(), actionName, indexQueryAction);
+  }
+
+  /**
+   * Creates an action for querying the change index that is executed with retrying when called.
+   *
+   * <p>The index query action gets a {@link InternalChangeQuery} provided that can be used to query
+   * the change index.
+   *
+   * @param actionName the name of the action, used as metric bucket
+   * @param indexQueryAction the action that should be executed
+   * @return the retryable action, callers need to call {@link RetryableIndexQueryAction#call()} to
+   *     execute the action
+   */
+  public <T> RetryableIndexQueryAction<InternalChangeQuery, T> changeIndexQuery(
+      String actionName, IndexQueryAction<T, InternalChangeQuery> indexQueryAction) {
+    return new RetryableIndexQueryAction<>(
+        this, internalChangeQuery.get(), actionName, indexQueryAction);
   }
 
   Duration getDefaultTimeout(ActionType actionType) {

java/com/google/gerrit/server/update/RetryableAction.java

@@ -30,8 +30,7 @@ import java.util.function.Predicate;
  * <p>Instances of this class are created via {@link RetryHelper} (see {@link
  * RetryHelper#action(ActionType, String, Action)}, {@link RetryHelper#accountUpdate(String,
  * Action)}, {@link RetryHelper#changeUpdate(String, Action)}, {@link
- * RetryHelper#groupUpdate(String, Action)}, {@link RetryHelper#pluginUpdate(String, Action)},
+ * RetryHelper#groupUpdate(String, Action)}, {@link RetryHelper#pluginUpdate(String, Action)}).
- * {@link RetryHelper#indexQuery(String, Action)}).
  *
  * <p>Which exceptions cause a retry is controlled by {@link ExceptionHook#shouldRetry(Throwable)}.
  * In addition callers can specify additional exception that should cause a retry via {@link

java/com/google/gerrit/server/update/RetryableIndexQueryAction.java

@@ -0,0 +1,90 @@
+// Copyright (C) 2019 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.server.update;
+
+import com.github.rholder.retry.RetryListener;
+import com.google.common.base.Throwables;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.index.query.InternalQuery;
+import java.util.function.Consumer;
+import java.util.function.Predicate;
+
+/**
+ * An action to query an index that is executed with retrying.
+ *
+ * <p>Instances of this class are created via {@link RetryHelper#accountIndexQuery(String,
+ * IndexQueryAction)} and {@link RetryHelper#changeIndexQuery(String, IndexQueryAction)}.
+ *
+ * <p>In contrast to normal {@link RetryableAction.Action}s that are called via {@link
+ * RetryableAction} {@link IndexQueryAction}s get a {@link InternalQuery} provided.
+ *
+ * <p>In addition when an index query action is called any exception that is not an unchecked
+ * exception gets wrapped into an {@link StorageException}.
+ */
+public class RetryableIndexQueryAction<Q extends InternalQuery<?, Q>, T>
+    extends RetryableAction<T> {
+  @FunctionalInterface
+  public interface IndexQueryAction<T, Q> {
+    T call(Q internalQuery) throws Exception;
+  }
+
+  RetryableIndexQueryAction(
+      RetryHelper retryHelper,
+      Q internalQuery,
+      String actionName,
+      IndexQueryAction<T, Q> indexQuery) {
+    super(retryHelper, ActionType.INDEX_QUERY, actionName, () -> indexQuery.call(internalQuery));
+  }
+
+  @Override
+  public RetryableIndexQueryAction<Q, T> retryOn(Predicate<Throwable> exceptionPredicate) {
+    super.retryOn(exceptionPredicate);
+    return this;
+  }
+
+  @Override
+  public RetryableIndexQueryAction<Q, T> retryWithTrace(Predicate<Throwable> exceptionPredicate) {
+    super.retryWithTrace(exceptionPredicate);
+    return this;
+  }
+
+  @Override
+  public RetryableIndexQueryAction<Q, T> onAutoTrace(Consumer<String> traceIdConsumer) {
+    super.onAutoTrace(traceIdConsumer);
+    return this;
+  }
+
+  @Override
+  public RetryableIndexQueryAction<Q, T> listener(RetryListener retryListener) {
+    super.listener(retryListener);
+    return this;
+  }
+
+  @Override
+  public RetryableIndexQueryAction<Q, T> defaultTimeoutMultiplier(int multiplier) {
+    super.defaultTimeoutMultiplier(multiplier);
+    return this;
+  }
+
+  @Override
+  public T call() {
+    try {
+      return super.call();
+    } catch (Throwable t) {
+      Throwables.throwIfUnchecked(t);
+      throw new StorageException(t);
+    }
+  }
+}

javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java

@@ -2761,6 +2761,8 @@ public class AccountIT extends AbstractDaemonTest {
                 cfg,
                 retryMetrics,
                 null,
+                null,
+                null,
                 exceptionHooks,
                 r -> r.withBlockStrategy(noSleepBlockStrategy)),
             extIdNotesFactory,
@@ -2815,6 +2817,8 @@ public class AccountIT extends AbstractDaemonTest {
                 cfg,
                 retryMetrics,
                 null,
+                null,
+                null,
                 exceptionHooks,
                 r ->
                     r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size()))
@@ -2873,6 +2877,8 @@ public class AccountIT extends AbstractDaemonTest {
                 cfg,
                 retryMetrics,
                 null,
+                null,
+                null,
                 exceptionHooks,
                 r -> r.withBlockStrategy(noSleepBlockStrategy)),
             extIdNotesFactory,
@@ -2946,6 +2952,8 @@ public class AccountIT extends AbstractDaemonTest {
                 cfg,
                 retryMetrics,
                 null,
+                null,
+                null,
                 exceptionHooks,
                 r -> r.withBlockStrategy(noSleepBlockStrategy)),
             extIdNotesFactory,