From fc417a149b2c3109fa330d1c850b79576af3480b Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 20 Nov 2019 15:13:26 +0100 Subject: [PATCH] Extract detection of LOCK_FAILURE errors into an ExceptionHook ExceptionHook allows implementors to detect and handle exceptions that are caused by temporary errors, and hence should cause a retry of the failed operation. We should make use of it to detect and handle LOCK_FAILURE errors. This way the code to detect LOCK_FAILURE is in a single place instead of in RetryHelper for change actions and in RestApiServlet for REST requests. As it turns out the detection of LOCK_FAILURE errors in these places was inconsistent, RetryHelper did unwrapping for UpdateException and StorageException, while RestApiServlet did unwrapping only for UpdateException. Due to this some LOCK_FAILURE errors such as [1] didn't trigger request retries. [1] com.google.gerrit.exceptions.StorageException: Star change 133855 for account 6003 failed at com.google.gerrit.server.StarredChangesUtil.star(StarredChangesUtil.java:234) at com.google.gerrit.server.restapi.account.StarredChanges$Create.apply(StarredChanges.java:133) at com.google.gerrit.server.restapi.account.StarredChanges$Create.apply(StarredChanges.java:97) at com.google.gerrit.httpd.restapi.RestApiServlet.lambda$invokeRestCollectionCreateViewWithRetry$5(RestApiServlet.java:758) at com.github.rholder.retry.AttemptTimeLimiters$NoAttemptTimeLimit.call(AttemptTimeLimiters.java:78) at com.github.rholder.retry.Retryer.call(Retryer.java:160) at com.google.gerrit.server.update.RetryHelper.executeWithTimeoutCount(RetryHelper.java:417) at com.google.gerrit.server.update.RetryHelper.executeWithAttemptAndTimeoutCount(RetryHelper.java:368) at com.google.gerrit.server.update.RetryHelper.execute(RetryHelper.java:271) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestEndpointWithRetry(RestApiServlet.java:820) at com.google.gerrit.httpd.restapi.RestApiServlet.invokeRestCollectionCreateViewWithRetry(RestApiServlet.java:753) at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:527) at javax.servlet.http.HttpServlet.service(HttpServlet.java:717) ... Caused by: com.google.gerrit.git.LockFailureException: Update star labels on ref refs/starred-changes/55/133855/6003 failed at com.google.gerrit.server.StarredChangesUtil.updateLabels(StarredChangesUtil.java:484) at com.google.gerrit.server.StarredChangesUtil.star(StarredChangesUtil.java:227) ... 208 more Signed-off-by: Edwin Kempin Change-Id: Ia7162ee5441d72b9ed32ff5b145b93d8626713c6 --- .../gerrit/httpd/restapi/RestApiServlet.java | 40 +++++----------- .../gerrit/server/ExceptionHookImpl.java | 47 +++++++++++++++++++ .../server/config/GerritGlobalModule.java | 2 + .../gerrit/server/update/RetryHelper.java | 15 +----- 4 files changed, 61 insertions(+), 43 deletions(-) create mode 100644 java/com/google/gerrit/server/ExceptionHookImpl.java diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java index 7700740db7..1514df82da 100644 --- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java @@ -173,7 +173,6 @@ import java.util.Optional; import java.util.Set; import java.util.TreeMap; import java.util.concurrent.TimeUnit; -import java.util.function.Predicate; import java.util.regex.Pattern; import java.util.stream.Stream; import java.util.zip.GZIPOutputStream; @@ -704,8 +703,7 @@ public class RestApiServlet extends HttpServlet { traceContext, globals.metrics.view(restCollection.getClass(), pluginName) + "#parse", ActionType.REST_READ_REQUEST, - () -> restCollection.parse(parentResource, id), - noRetry()); + () -> restCollection.parse(parentResource, id)); } private Response invokeRestReadViewWithRetry( @@ -720,8 +718,7 @@ public class RestApiServlet extends HttpServlet { traceContext, getViewName(viewData), ActionType.REST_READ_REQUEST, - () -> view.apply(rsrc), - noRetry()); + () -> view.apply(rsrc)); } private Response invokeRestModifyViewWithRetry( @@ -737,8 +734,7 @@ public class RestApiServlet extends HttpServlet { traceContext, getViewName(viewData), ActionType.REST_WRITE_REQUEST, - () -> view.apply(rsrc, inputRequestBody), - retryOnLockFailure()); + () -> view.apply(rsrc, inputRequestBody)); } private Response invokeRestCollectionCreateViewWithRetry( @@ -755,8 +751,7 @@ public class RestApiServlet extends HttpServlet { traceContext, getViewName(viewData), ActionType.REST_WRITE_REQUEST, - () -> view.apply(rsrc, path, inputRequestBody), - retryOnLockFailure()); + () -> view.apply(rsrc, path, inputRequestBody)); } private Response invokeRestCollectionDeleteMissingViewWithRetry( @@ -773,8 +768,7 @@ public class RestApiServlet extends HttpServlet { traceContext, getViewName(viewData), ActionType.REST_WRITE_REQUEST, - () -> view.apply(rsrc, path, inputRequestBody), - retryOnLockFailure()); + () -> view.apply(rsrc, path, inputRequestBody)); } private Response invokeRestCollectionModifyViewWithRetry( @@ -790,8 +784,7 @@ public class RestApiServlet extends HttpServlet { traceContext, getViewName(viewData), ActionType.REST_WRITE_REQUEST, - () -> view.apply(rsrc, inputRequestBody), - retryOnLockFailure()); + () -> view.apply(rsrc, inputRequestBody)); } private T invokeRestEndpointWithRetry( @@ -799,8 +792,7 @@ public class RestApiServlet extends HttpServlet { TraceContext traceContext, String caller, ActionType actionType, - Action action, - Predicate retryExceptionPredicate) + Action action) throws Exception { RetryHelper.Options.Builder retryOptionsBuilder = RetryHelper.options().caller(caller); if (!traceContext.isTracing()) { @@ -817,8 +809,11 @@ public class RestApiServlet extends HttpServlet { }); } try { + // ExceptionHookImpl controls on which exceptions we retry. + // The passed in exceptionPredicate allows to define additional exceptions on which retry + // should happen, but here we have none (hence pass in "t -> false" as exceptionPredicate). return globals.retryHelper.execute( - actionType, action, retryOptionsBuilder.build(), retryExceptionPredicate); + actionType, action, retryOptionsBuilder.build(), t -> false); } finally { // If auto-tracing got triggered due to a non-recoverable failure, also trace the rest of // this request. This means logging is forced for all further log statements and the logs are @@ -827,19 +822,6 @@ public class RestApiServlet extends HttpServlet { } } - private static Predicate noRetry() { - return t -> false; - } - - private static Predicate retryOnLockFailure() { - return t -> { - if (t instanceof UpdateException) { - t = t.getCause(); - } - return t instanceof LockFailureException; - }; - } - private String getViewName(ViewData viewData) { return viewData != null && viewData.view != null ? globals.metrics.view(viewData) : "_unknown"; } diff --git a/java/com/google/gerrit/server/ExceptionHookImpl.java b/java/com/google/gerrit/server/ExceptionHookImpl.java new file mode 100644 index 0000000000..b5edb24329 --- /dev/null +++ b/java/com/google/gerrit/server/ExceptionHookImpl.java @@ -0,0 +1,47 @@ +// 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.exceptions.StorageException; +import com.google.gerrit.git.LockFailureException; +import com.google.gerrit.server.update.UpdateException; +import java.util.Optional; +import org.eclipse.jgit.lib.RefUpdate; + +/** + * Class to detect and handle exceptions that are caused by temporary errors, and hence should cause + * a retry of the failed operation. + */ +public class ExceptionHookImpl implements ExceptionHook { + @Override + public boolean shouldRetry(Throwable throwable) { + return isLockFailure(throwable); + } + + @Override + public Optional formatCause(Throwable throwable) { + if (isLockFailure(throwable)) { + return Optional.of(RefUpdate.Result.LOCK_FAILURE.name()); + } + return Optional.empty(); + } + + private static boolean isLockFailure(Throwable throwable) { + if (throwable instanceof UpdateException || throwable instanceof StorageException) { + throwable = throwable.getCause(); + } + return throwable instanceof LockFailureException; + } +} diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index 3b9c40e67f..2a0466fc52 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -79,6 +79,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.ExceptionHookImpl; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.RequestListener; import com.google.gerrit.server.TraceRequestListener; @@ -393,6 +394,7 @@ public class GerritGlobalModule extends FactoryModule { DynamicSet.bind(binder(), RequestListener.class).to(TraceRequestListener.class); DynamicSet.setOf(binder(), ChangeETagComputation.class); DynamicSet.setOf(binder(), ExceptionHook.class); + DynamicSet.bind(binder(), ExceptionHook.class).to(ExceptionHookImpl.class); DynamicSet.setOf(binder(), MailSoyTemplateProvider.class); DynamicMap.mapOf(binder(), MailFilter.class); diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java index f8cf4cfff8..6f8ef12a0f 100644 --- a/java/com/google/gerrit/server/update/RetryHelper.java +++ b/java/com/google/gerrit/server/update/RetryHelper.java @@ -34,7 +34,6 @@ import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.Nullable; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.restapi.RestApiException; -import com.google.gerrit.git.LockFailureException; import com.google.gerrit.metrics.Counter1; import com.google.gerrit.metrics.Counter2; import com.google.gerrit.metrics.Counter3; @@ -57,7 +56,6 @@ import java.util.concurrent.ExecutionException; import java.util.function.Consumer; import java.util.function.Predicate; import org.eclipse.jgit.lib.Config; -import org.eclipse.jgit.lib.RefUpdate; @Singleton public class RetryHelper { @@ -284,15 +282,7 @@ public class RetryHelper { throws RestApiException, UpdateException { try { return execute( - ActionType.CHANGE_UPDATE, - () -> changeAction.call(updateFactory), - opts, - t -> { - if (t instanceof UpdateException || t instanceof StorageException) { - t = t.getCause(); - } - return t instanceof LockFailureException; - }); + ActionType.CHANGE_UPDATE, () -> changeAction.call(updateFactory), opts, t -> false); } catch (Throwable t) { Throwables.throwIfUnchecked(t); Throwables.throwIfInstanceOf(t, UpdateException.class); @@ -390,9 +380,6 @@ public class RetryHelper { return formattedCause.get(); } - if (t instanceof LockFailureException) { - return RefUpdate.Result.LOCK_FAILURE.name(); - } return t.getClass().getSimpleName(); }