diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index 77ef60da97..74ed72548b 100644 --- a/Documentation/dev-plugins.txt +++ b/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 diff --git a/java/com/google/gerrit/server/ExceptionHook.java b/java/com/google/gerrit/server/ExceptionHook.java new file mode 100644 index 0000000000..ea76330244 --- /dev/null +++ b/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. + * + *

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. + * + *

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; + } +} diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index 45b70b2c07..3794f04d79 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/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); diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java index b12037906f..bea38677a8 100644 --- a/java/com/google/gerrit/server/update/RetryHelper.java +++ b/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 exceptionHooks; private final Map defaultTimeouts; private final WaitStrategy waitStrategy; @Nullable private final Consumer> overwriteDefaultRetryerStrategySetup; private final boolean retryWithTraceOnFailure; @Inject - RetryHelper(@GerritServerConfig Config cfg, Metrics metrics, BatchUpdate.Factory updateFactory) { - this(cfg, metrics, updateFactory, null); + RetryHelper( + @GerritServerConfig Config cfg, + Metrics metrics, + PluginSetContext 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 exceptionHooks, @Nullable Consumer> 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 diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 405c46307f..eb59669c7a 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/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, diff --git a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java b/javatests/com/google/gerrit/acceptance/rest/TraceIT.java index 7b743f0a9d..0e5120830b 100644 --- a/javatests/com/google/gerrit/acceptance/rest/TraceIT.java +++ b/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 changeIndexedListeners; @Inject private DynamicSet performanceLoggers; @Inject private DynamicSet submitRules; + @Inject private DynamicSet 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();