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());