Merge changes Id316cffa,I14b8f005
* changes:
Add hook for exceptions
Add a dedicated exception that is thrown in case of Git update failures
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index 77ef60d..74ed725 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -2738,6 +2738,20 @@
}
----
+[[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/git/BUILD b/java/com/google/gerrit/git/BUILD
index 5ece37a..1edba38 100644
--- a/java/com/google/gerrit/git/BUILD
+++ b/java/com/google/gerrit/git/BUILD
@@ -7,6 +7,8 @@
deps = [
"//java/com/google/gerrit/common:annotations",
"//lib:guava",
+ "//lib/auto:auto-value",
+ "//lib/auto:auto-value-annotations",
"//lib/jgit/org.eclipse.jgit:jgit",
],
)
diff --git a/java/com/google/gerrit/git/GitUpdateFailureException.java b/java/com/google/gerrit/git/GitUpdateFailureException.java
new file mode 100644
index 0000000..76ef217
--- /dev/null
+++ b/java/com/google/gerrit/git/GitUpdateFailureException.java
@@ -0,0 +1,95 @@
+// 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.git;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.common.UsedAt;
+import java.io.IOException;
+import java.util.Optional;
+import org.eclipse.jgit.lib.BatchRefUpdate;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.transport.ReceiveCommand;
+
+/** Thrown when updating a ref in Git fails. */
+public class GitUpdateFailureException extends IOException {
+ private static final long serialVersionUID = 1L;
+
+ private final ImmutableList<GitUpdateFailure> failures;
+
+ public GitUpdateFailureException(String message, RefUpdate refUpdate) {
+ super(message);
+ this.failures = ImmutableList.of(GitUpdateFailure.create(refUpdate));
+ }
+
+ public GitUpdateFailureException(String message, BatchRefUpdate batchRefUpdate) {
+ super(message);
+ this.failures =
+ batchRefUpdate.getCommands().stream()
+ .filter(c -> c.getResult() != ReceiveCommand.Result.OK)
+ .map(GitUpdateFailure::create)
+ .collect(toImmutableList());
+ }
+
+ /** @return the names of the refs for which the update failed. */
+ public ImmutableList<String> getFailedRefs() {
+ return failures.stream().map(GitUpdateFailure::ref).collect(toImmutableList());
+ }
+
+ /** @return the failures that caused this exception. */
+ @UsedAt(UsedAt.Project.GOOGLE)
+ public ImmutableList<GitUpdateFailure> getFailures() {
+ return failures;
+ }
+
+ @AutoValue
+ public abstract static class GitUpdateFailure {
+ private static GitUpdateFailure create(RefUpdate refUpdate) {
+ return builder().ref(refUpdate.getName()).result(refUpdate.getResult().name()).build();
+ }
+
+ private static GitUpdateFailure create(ReceiveCommand receiveCommand) {
+ return builder()
+ .ref(receiveCommand.getRefName())
+ .result(receiveCommand.getResult().name())
+ .message(receiveCommand.getMessage())
+ .build();
+ }
+
+ public abstract String ref();
+
+ public abstract String result();
+
+ public abstract Optional<String> message();
+
+ public static GitUpdateFailure.Builder builder() {
+ return new AutoValue_GitUpdateFailureException_GitUpdateFailure.Builder();
+ }
+
+ @AutoValue.Builder
+ abstract static class Builder {
+ abstract Builder ref(String ref);
+
+ abstract Builder result(String result);
+
+ abstract Builder message(@Nullable String message);
+
+ abstract GitUpdateFailure build();
+ }
+ }
+}
diff --git a/java/com/google/gerrit/git/LockFailureException.java b/java/com/google/gerrit/git/LockFailureException.java
index 9e67d70..371488d 100644
--- a/java/com/google/gerrit/git/LockFailureException.java
+++ b/java/com/google/gerrit/git/LockFailureException.java
@@ -14,36 +14,18 @@
package com.google.gerrit.git;
-import static com.google.common.collect.ImmutableList.toImmutableList;
-
-import com.google.common.collect.ImmutableList;
-import java.io.IOException;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.RefUpdate;
-import org.eclipse.jgit.transport.ReceiveCommand;
/** Thrown when updating a ref in Git fails with LOCK_FAILURE. */
-public class LockFailureException extends IOException {
+public class LockFailureException extends GitUpdateFailureException {
private static final long serialVersionUID = 1L;
- private final ImmutableList<String> refs;
-
public LockFailureException(String message, RefUpdate refUpdate) {
- super(message);
- refs = ImmutableList.of(refUpdate.getName());
+ super(message, refUpdate);
}
public LockFailureException(String message, BatchRefUpdate batchRefUpdate) {
- super(message);
- refs =
- batchRefUpdate.getCommands().stream()
- .filter(c -> c.getResult() == ReceiveCommand.Result.LOCK_FAILURE)
- .map(ReceiveCommand::getRefName)
- .collect(toImmutableList());
- }
-
- /** Subset of ref names that caused the lock failure. */
- public ImmutableList<String> getFailedRefs() {
- return refs;
+ super(message, batchRefUpdate);
}
}
diff --git a/java/com/google/gerrit/git/RefUpdateUtil.java b/java/com/google/gerrit/git/RefUpdateUtil.java
index 520d0f2..fa7b98f 100644
--- a/java/com/google/gerrit/git/RefUpdateUtil.java
+++ b/java/com/google/gerrit/git/RefUpdateUtil.java
@@ -99,7 +99,7 @@
if (lockFailure + aborted == bru.getCommands().size()) {
throw new LockFailureException("Update aborted with one or more lock failures: " + bru, bru);
} else if (failure > 0) {
- throw new IOException("Update failed: " + bru);
+ throw new GitUpdateFailureException("Update failed: " + bru, bru);
}
}
@@ -130,7 +130,8 @@
case REJECTED_CURRENT_BRANCH:
case REJECTED_MISSING_OBJECT:
case REJECTED_OTHER_REASON:
- throw new IOException("Failed to update " + ru.getName() + ": " + ru.getResult());
+ throw new GitUpdateFailureException(
+ "Failed to update " + ru.getName() + ": " + ru.getResult(), ru);
}
}
@@ -174,7 +175,8 @@
case REJECTED_MISSING_OBJECT:
case REJECTED_OTHER_REASON:
default:
- throw new IOException("Failed to delete " + refName + ": " + ru.getResult());
+ throw new GitUpdateFailureException(
+ "Failed to delete " + refName + ": " + ru.getResult(), ru);
}
}
diff --git a/java/com/google/gerrit/server/ExceptionHook.java b/java/com/google/gerrit/server/ExceptionHook.java
new file mode 100644
index 0000000..ea76330
--- /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.
+ *
+ * <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;
+ }
+}
diff --git a/java/com/google/gerrit/server/StarredChangesUtil.java b/java/com/google/gerrit/server/StarredChangesUtil.java
index 6a23084..1b1fab6 100644
--- a/java/com/google/gerrit/server/StarredChangesUtil.java
+++ b/java/com/google/gerrit/server/StarredChangesUtil.java
@@ -32,6 +32,7 @@
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.git.GitUpdateFailureException;
import com.google.gerrit.git.LockFailureException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@@ -268,7 +269,7 @@
if (command.getResult() == ReceiveCommand.Result.LOCK_FAILURE) {
throw new LockFailureException(message, batchUpdate);
}
- throw new IOException(message);
+ throw new GitUpdateFailureException(message, batchUpdate);
}
}
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 45b70b2..3794f04 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.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 @@
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/git/meta/VersionedMetaData.java b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java
index d8829bf..aa735f9 100644
--- a/java/com/google/gerrit/server/git/meta/VersionedMetaData.java
+++ b/java/com/google/gerrit/server/git/meta/VersionedMetaData.java
@@ -18,6 +18,7 @@
import com.google.common.base.MoreObjects;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.git.GitUpdateFailureException;
import com.google.gerrit.git.LockFailureException;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.reviewdb.client.Change;
@@ -433,13 +434,14 @@
case REJECTED_MISSING_OBJECT:
case REJECTED_OTHER_REASON:
default:
- throw new IOException(
+ throw new GitUpdateFailureException(
"Cannot update "
+ ru.getName()
+ " in "
+ db.getDirectory()
+ ": "
- + ru.getResult());
+ + ru.getResult(),
+ ru);
}
}
};
diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java
index b120379..bea3867 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.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 @@
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) {
- this(cfg, metrics, updateFactory, null);
+ RetryHelper(
+ @GerritServerConfig Config cfg,
+ Metrics metrics,
+ PluginSetContext<ExceptionHook> exceptionHooks,
+ BatchUpdate.Factory updateFactory) {
+ this(cfg, metrics, updateFactory, exceptionHooks, null);
}
@VisibleForTesting
@@ -197,9 +204,11 @@
@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 @@
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 405c463..eb59669 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.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 @@
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 @@
cfg,
retryMetrics,
null,
+ new PluginSetContext<>(DynamicSet.emptySet(), PluginMetrics.DISABLED_INSTANCE),
r ->
r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size()))
.withBlockStrategy(noSleepBlockStrategy)),
@@ -2696,7 +2703,11 @@
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 @@
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 7b743f0..0e51208 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.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 @@
@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;
@@ -606,6 +608,36 @@
}
@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();
approve(changeId);
