Add hook for exceptions

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

Documentation/dev-plugins.txt

@@ -2738,6 +2738,20 @@ public class MyPluginChangeETagComputation implements ChangeETagComputation {
 }
 ----
 
+[[exception-hook]]
+== ExceptionHook
+
+An `ExceptionHook` allows implementors to control how certain
+exceptions should be handled.
+
+This interface is intended to be implemented for multi-master setups to
+control the behavior for handling exceptions that are thrown by a lower
+layer that handles the consensus and synchronization between different
+server nodes. E.g. if an operation fails because consensus for a Git
+update could not be achieved (e.g. due to slow responding server nodes)
+this interface can be used to retry the request instead of failing it
+immediately.
+
 [[quota-enforcer]]
 == Quota Enforcer
 

java/com/google/gerrit/server/ExceptionHook.java

@@ -0,0 +1,42 @@
+// 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;
+
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+
+/**
+ * Allows implementors to control how certain exceptions should be handled.
+ *
+ * <p>This interface is intended to be implemented for multi-master setups to control the behavior
+ * for handling exceptions that are thrown by a lower layer that handles the consensus and
+ * synchronization between different server nodes. E.g. if an operation fails because consensus for
+ * a Git update could not be achieved (e.g. due to slow responding server nodes) this interface can
+ * be used to retry the request instead of failing it immediately.
+ */
+@ExtensionPoint
+public interface ExceptionHook {
+  /**
+   * Whether an operation should be retried if it failed with the given throwable.
+   *
+   * <p>Only affects operations that are executed with {@link
+   * com.google.gerrit.server.update.RetryHelper}.
+   *
+   * @param throwable throwable that was thrown while executing the operation
+   * @return whether the operation should be retried
+   */
+  default boolean shouldRetry(Throwable throwable) {
+    return false;
+  }
+}

java/com/google/gerrit/server/config/GerritGlobalModule.java

@@ -78,6 +78,7 @@ import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.CmdLineParserModule;
 import com.google.gerrit.server.CreateGroupPermissionSyncer;
 import com.google.gerrit.server.DynamicOptions;
+import com.google.gerrit.server.ExceptionHook;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.RequestListener;
 import com.google.gerrit.server.TraceRequestListener;
@@ -390,6 +391,7 @@ public class GerritGlobalModule extends FactoryModule {
     DynamicSet.setOf(binder(), RequestListener.class);
     DynamicSet.bind(binder(), RequestListener.class).to(TraceRequestListener.class);
     DynamicSet.setOf(binder(), ChangeETagComputation.class);
+    DynamicSet.setOf(binder(), ExceptionHook.class);
 
     DynamicMap.mapOf(binder(), MailFilter.class);
     bind(MailFilter.class).annotatedWith(Exports.named("ListMailFilter")).to(ListMailFilter.class);

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

@@ -39,10 +39,12 @@ import com.google.gerrit.metrics.Counter2;
 import com.google.gerrit.metrics.Description;
 import com.google.gerrit.metrics.Field;
 import com.google.gerrit.metrics.MetricMaker;
+import com.google.gerrit.server.ExceptionHook;
 import com.google.gerrit.server.config.GerritServerConfig;
 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.inject.Inject;
 import com.google.inject.Singleton;
 import java.time.Duration;
@@ -182,14 +184,19 @@ public class RetryHelper {
 
   private final Metrics metrics;
   private final BatchUpdate.Factory updateFactory;
+  private final PluginSetContext<ExceptionHook> exceptionHooks;
   private final Map<ActionType, Duration> defaultTimeouts;
   private final WaitStrategy waitStrategy;
   @Nullable private final Consumer<RetryerBuilder<?>> overwriteDefaultRetryerStrategySetup;
   private final boolean retryWithTraceOnFailure;
 
   @Inject
-  RetryHelper(@GerritServerConfig Config cfg, Metrics metrics, BatchUpdate.Factory updateFactory) {
+  RetryHelper(
-    this(cfg, metrics, updateFactory, null);
+      @GerritServerConfig Config cfg,
+      Metrics metrics,
+      PluginSetContext<ExceptionHook> exceptionHooks,
+      BatchUpdate.Factory updateFactory) {
+    this(cfg, metrics, updateFactory, exceptionHooks, null);
   }
 
   @VisibleForTesting
@@ -197,9 +204,11 @@ public class RetryHelper {
       @GerritServerConfig Config cfg,
       Metrics metrics,
       BatchUpdate.Factory updateFactory,
+      PluginSetContext<ExceptionHook> exceptionHooks,
       @Nullable Consumer<RetryerBuilder<?>> overwriteDefaultRetryerStrategySetup) {
     this.metrics = metrics;
     this.updateFactory = updateFactory;
+    this.exceptionHooks = exceptionHooks;
 
     Duration defaultTimeout =
         Duration.ofMillis(
@@ -308,6 +317,11 @@ public class RetryHelper {
                   return true;
                 }
 
+                // Exception hooks may identify additional exceptions for retry.
+                if (exceptionHooks.stream().anyMatch(h -> h.shouldRetry(t))) {
+                  return true;
+                }
+
                 // A non-recoverable failure occurred. Check if we should retry to capture a trace
                 // of the failure. If a trace was already done there is no need to retry.
                 if (retryWithTraceOnFailure

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

@@ -134,6 +134,8 @@ import com.google.gerrit.server.index.account.StalenessChecker;
 import com.google.gerrit.server.notedb.Sequences;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
+import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
+import com.google.gerrit.server.plugincontext.PluginSetContext;
 import com.google.gerrit.server.project.ProjectConfig;
 import com.google.gerrit.server.project.RefPattern;
 import com.google.gerrit.server.query.account.InternalAccountQuery;
@@ -2589,7 +2591,11 @@ public class AccountIT extends AbstractDaemonTest {
             externalIds,
             metaDataUpdateInternalFactory,
             new RetryHelper(
-                cfg, retryMetrics, null, r -> r.withBlockStrategy(noSleepBlockStrategy)),
+                cfg,
+                retryMetrics,
+                null,
+                new PluginSetContext<>(DynamicSet.emptySet(), PluginMetrics.DISABLED_INSTANCE),
+                r -> r.withBlockStrategy(noSleepBlockStrategy)),
             extIdNotesFactory,
             ident,
             ident,
@@ -2642,6 +2648,7 @@ public class AccountIT extends AbstractDaemonTest {
                 cfg,
                 retryMetrics,
                 null,
+                new PluginSetContext<>(DynamicSet.emptySet(), PluginMetrics.DISABLED_INSTANCE),
                 r ->
                     r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size()))
                         .withBlockStrategy(noSleepBlockStrategy)),
@@ -2696,7 +2703,11 @@ public class AccountIT extends AbstractDaemonTest {
             externalIds,
             metaDataUpdateInternalFactory,
             new RetryHelper(
-                cfg, retryMetrics, null, r -> r.withBlockStrategy(noSleepBlockStrategy)),
+                cfg,
+                retryMetrics,
+                null,
+                new PluginSetContext<>(DynamicSet.emptySet(), PluginMetrics.DISABLED_INSTANCE),
+                r -> r.withBlockStrategy(noSleepBlockStrategy)),
             extIdNotesFactory,
             ident,
             ident,
@@ -2765,7 +2776,11 @@ public class AccountIT extends AbstractDaemonTest {
             externalIds,
             metaDataUpdateInternalFactory,
             new RetryHelper(
-                cfg, retryMetrics, null, r -> r.withBlockStrategy(noSleepBlockStrategy)),
+                cfg,
+                retryMetrics,
+                null,
+                new PluginSetContext<>(DynamicSet.emptySet(), PluginMetrics.DISABLED_INSTANCE),
+                r -> r.withBlockStrategy(noSleepBlockStrategy)),
             extIdNotesFactory,
             ident,
             ident,

javatests/com/google/gerrit/acceptance/rest/TraceIT.java

@@ -36,6 +36,7 @@ import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.registration.RegistrationHandle;
 import com.google.gerrit.httpd.restapi.ParameterParser;
 import com.google.gerrit.httpd.restapi.RestApiServlet;
+import com.google.gerrit.server.ExceptionHook;
 import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.WorkQueue;
 import com.google.gerrit.server.git.validators.CommitValidationException;
@@ -85,6 +86,7 @@ public class TraceIT extends AbstractDaemonTest {
   @Inject private DynamicSet<ChangeIndexedListener> changeIndexedListeners;
   @Inject private DynamicSet<PerformanceLogger> performanceLoggers;
   @Inject private DynamicSet<SubmitRule> submitRules;
+  @Inject private DynamicSet<ExceptionHook> exceptionHooks;
   @Inject private WorkQueue workQueue;
 
   private TraceValidatingProjectCreationValidationListener projectCreationListener;
@@ -605,6 +607,36 @@ public class TraceIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  @GerritConfig(name = "retry.retryWithTraceOnFailure", value = "true")
+  public void noAutoRetryIfExceptionCausesNormalRetrying() throws Exception {
+    String changeId = createChange().getChangeId();
+    approve(changeId);
+
+    TraceSubmitRule traceSubmitRule = new TraceSubmitRule();
+    traceSubmitRule.failAlways = true;
+    RegistrationHandle submitRuleRegistrationHandle = submitRules.add("gerrit", traceSubmitRule);
+    RegistrationHandle exceptionHookRegistrationHandle =
+        exceptionHooks.add(
+            "gerrit",
+            new ExceptionHook() {
+              @Override
+              public boolean shouldRetry(Throwable t) {
+                return true;
+              }
+            });
+    try {
+      RestResponse response = adminRestSession.post("/changes/" + changeId + "/submit");
+      assertThat(response.getStatusCode()).isEqualTo(SC_INTERNAL_SERVER_ERROR);
+      assertThat(response.getHeader(RestApiServlet.X_GERRIT_TRACE)).isNull();
+      assertThat(traceSubmitRule.traceId).isNull();
+      assertThat(traceSubmitRule.isLoggingForced).isFalse();
+    } finally {
+      submitRuleRegistrationHandle.remove();
+      exceptionHookRegistrationHandle.remove();
+    }
+  }
+
   @Test
   public void noAutoRetryWithTraceIfDisabled() throws Exception {
     String changeId = createChange().getChangeId();