From 14e65d16dc8ba7890fb0f8d076db82b34c9a0cd7 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 15 Jan 2018 12:55:29 +0100 Subject: [PATCH] RetryHelper: Let callers care about exception handling When retrying actions it is specific to the caller which exceptions should be rethrown and which exceptions should be wrapped. At the moment RetryHelper has 2 kinds of execute methods that are suitable for change updates and account updates. These execute methods do not fit for all cases, e.g. ReceiveCommits uses RetryHelper to retry change queries, but the execute method requires handling of ConfigInvalidException which cannot occur during index queries. Instead of adding more specific execute methods to RetryHelper, just let the execute method throw Exception and let the caller take care about the exception handling. The execute method that is specific for change updates is kept in RetryHelper because it has many callers. All other exception handling is currently unique for one caller, hence it's not needed to keep a specialized execute method for these cases in RetryHelper. We can add more specifc execute methods to RetryHelper once the same exception handling is required by multiple callers to share the exception handling code. Change-Id: I7cf2b92fb228d2d6dddd89df5c338e53a2f8f526 Signed-off-by: Edwin Kempin --- .../gerrit/server/account/AccountsUpdate.java | 19 ++++++++- .../server/git/receive/ReceiveCommits.java | 23 +++++++---- .../gerrit/server/update/RetryHelper.java | 41 +++++++++---------- 3 files changed, 51 insertions(+), 32 deletions(-) diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java index 6729b86f24..154ce31381 100644 --- a/java/com/google/gerrit/server/account/AccountsUpdate.java +++ b/java/com/google/gerrit/server/account/AccountsUpdate.java @@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkState; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; +import com.google.common.base.Throwables; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.util.concurrent.Runnables; @@ -37,6 +38,7 @@ import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.index.change.ReindexAfterRefUpdate; import com.google.gerrit.server.update.RefUpdateUtil; import com.google.gerrit.server.update.RetryHelper; +import com.google.gerrit.server.update.RetryHelper.Action; import com.google.gerrit.server.update.RetryHelper.ActionType; import com.google.gwtorm.server.OrmDuplicateKeyException; import com.google.gwtorm.server.OrmException; @@ -498,8 +500,7 @@ public class AccountsUpdate { private Optional updateAccount(AccountUpdate accountUpdate) throws IOException, ConfigInvalidException, OrmException { - return retryHelper.execute( - ActionType.ACCOUNT_UPDATE, + return executeAccountUpdate( () -> { try (Repository allUsersRepo = repoManager.openRepository(allUsersName)) { UpdatedAccount updatedAccount = accountUpdate.update(allUsersRepo); @@ -513,6 +514,20 @@ public class AccountsUpdate { }); } + private Optional executeAccountUpdate(Action> action) + throws IOException, ConfigInvalidException, OrmException { + try { + return retryHelper.execute( + ActionType.ACCOUNT_UPDATE, action, LockFailureException.class::isInstance); + } catch (Exception e) { + Throwables.throwIfUnchecked(e); + Throwables.throwIfInstanceOf(e, IOException.class); + Throwables.throwIfInstanceOf(e, ConfigInvalidException.class); + Throwables.throwIfInstanceOf(e, OrmException.class); + throw new OrmException(e); + } + } + private ExternalIdNotes createExternalIdNotes( Repository allUsersRepo, Optional rev, diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index a9e3ed5f01..3b31543b38 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -41,6 +41,7 @@ import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_RE import com.google.common.base.Function; 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; @@ -149,6 +150,7 @@ 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.RetryHelper.Action; import com.google.gerrit.server.update.RetryHelper.ActionType; import com.google.gerrit.server.update.UpdateException; import com.google.gerrit.server.util.LabelVote; @@ -2891,10 +2893,7 @@ class ReceiveCommits { for (Ref ref : byCommit.get(c.copy())) { PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName()); Optional cd = - retryHelper.execute( - ActionType.CHANGE_QUERY, - () -> byLegacyId(psId.getParentKey()), - t -> t instanceof OrmException); + executeIndexQuery(() -> byLegacyId(psId.getParentKey())); if (cd.isPresent() && cd.get().change().getDest().equals(branch)) { existingPatchSets++; bu.addOp( @@ -2906,11 +2905,7 @@ class ReceiveCommits { for (String changeId : c.getFooterLines(CHANGE_ID)) { if (byKey == null) { - byKey = - retryHelper.execute( - ActionType.CHANGE_QUERY, - () -> openChangesByKeyByBranch(branch), - t -> t instanceof OrmException); + byKey = executeIndexQuery(() -> openChangesByKeyByBranch(branch)); } ChangeNotes onto = byKey.get(new Change.Key(changeId.trim())); @@ -2967,6 +2962,16 @@ class ReceiveCommits { } } + private T executeIndexQuery(Action action) throws OrmException { + try { + return retryHelper.execute(ActionType.INDEX_QUERY, action, OrmException.class::isInstance); + } catch (Exception e) { + Throwables.throwIfUnchecked(e); + Throwables.throwIfInstanceOf(e, OrmException.class); + throw new OrmException(e); + } + } + private void updateAccountInfo() { if (setFullNameTo == null) { return; diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java index 0f5b00fcb4..25b242bf95 100644 --- a/java/com/google/gerrit/server/update/RetryHelper.java +++ b/java/com/google/gerrit/server/update/RetryHelper.java @@ -40,14 +40,11 @@ import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.LockFailureException; import com.google.gerrit.server.notedb.NotesMigration; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; -import java.io.IOException; import java.time.Duration; import java.util.concurrent.ExecutionException; import java.util.function.Consumer; -import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; @Singleton @@ -64,8 +61,8 @@ public class RetryHelper { public enum ActionType { ACCOUNT_UPDATE, - CHANGE_QUERY, - CHANGE_UPDATE + CHANGE_UPDATE, + INDEX_QUERY, } /** @@ -182,22 +179,24 @@ public class RetryHelper { return defaultTimeout; } - public T execute(ActionType actionType, Action action) - throws IOException, ConfigInvalidException, OrmException { - return execute(actionType, action, t -> t instanceof LockFailureException); + public T execute( + ActionType actionType, Action action, Predicate exceptionPredicate) + throws Exception { + return execute(actionType, action, defaults(), exceptionPredicate); } public T execute( - ActionType actionType, Action action, Predicate exceptionPredicate) - throws IOException, ConfigInvalidException, OrmException { + ActionType actionType, + Action action, + Options opts, + Predicate exceptionPredicate) + throws Exception { try { - return execute(actionType, action, defaults(), exceptionPredicate); + return executeWithAttemptAndTimeoutCount(actionType, action, opts, exceptionPredicate); } catch (Throwable t) { Throwables.throwIfUnchecked(t); - Throwables.throwIfInstanceOf(t, IOException.class); - Throwables.throwIfInstanceOf(t, ConfigInvalidException.class); - Throwables.throwIfInstanceOf(t, OrmException.class); - throw new OrmException(t); + Throwables.throwIfInstanceOf(t, Exception.class); + throw new IllegalStateException(t); } } @@ -212,7 +211,7 @@ public class RetryHelper { // Either we aren't full-NoteDb, or the underlying ref storage doesn't support atomic // transactions. Either way, retrying a partially-failed operation is not idempotent, so // don't do it automatically. Let the end user decide whether they want to retry. - return execute( + return executeWithTimeoutCount( ActionType.CHANGE_UPDATE, () -> changeAction.call(updateFactory), RetryerBuilder.newBuilder().build()); @@ -237,7 +236,7 @@ public class RetryHelper { } /** - * Executes an action with a given retryer. + * Executes an action and records the number of attempts and the timeout as metrics. * * @param actionType the type of the action * @param action the action which should be executed and retried on failure @@ -247,7 +246,7 @@ public class RetryHelper { * @throws Throwable any error or exception that made the action fail, callers are expected to * catch and inspect this Throwable to decide carefully whether it should be re-thrown */ - private T execute( + private T executeWithAttemptAndTimeoutCount( ActionType actionType, Action action, Options opts, @@ -257,14 +256,14 @@ public class RetryHelper { try { RetryerBuilder retryerBuilder = createRetryerBuilder(opts, exceptionPredicate); retryerBuilder.withRetryListener(listener); - return execute(actionType, action, retryerBuilder.build()); + return executeWithTimeoutCount(actionType, action, retryerBuilder.build()); } finally { metrics.attemptCounts.record(actionType, listener.getAttemptCount()); } } /** - * Executes an action with a given retryer. + * Executes an action and records the timeout as metric. * * @param actionType the type of the action * @param action the action which should be executed and retried on failure @@ -273,7 +272,7 @@ public class RetryHelper { * @throws Throwable any error or exception that made the action fail, callers are expected to * catch and inspect this Throwable to decide carefully whether it should be re-thrown */ - private T execute(ActionType actionType, Action action, Retryer retryer) + private T executeWithTimeoutCount(ActionType actionType, Action action, Retryer retryer) throws Throwable { try { return retryer.call(() -> action.call());