opendev/gerrit
Code Issues Proposed changes

RetryHelper: Let callers care about exception handling

Browse Source 
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 <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-01-15 12:55:29 +01:00
parent b65e264faa
commit 14e65d16dc
3 changed files with 51 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
java/com/google/gerrit/server/account/AccountsUpdate.java
View File

@@ -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<AccountState> 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<AccountState> executeAccountUpdate(Action<Optional<AccountState>> 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<ObjectId> rev,

23
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -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<ChangeData> 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> T executeIndexQuery(Action<T> 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;

41
java/com/google/gerrit/server/update/RetryHelper.java
View File

@@ -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> T execute(ActionType actionType, Action<T> action)
throws IOException, ConfigInvalidException, OrmException {
return execute(actionType, action, t -> t instanceof LockFailureException);
public <T> T execute(
ActionType actionType, Action<T> action, Predicate<Throwable> exceptionPredicate)
throws Exception {
return execute(actionType, action, defaults(), exceptionPredicate);
}
public <T> T execute(
ActionType actionType, Action<T> action, Predicate<Throwable> exceptionPredicate)
throws IOException, ConfigInvalidException, OrmException {
ActionType actionType,
Action<T> action,
Options opts,
Predicate<Throwable> 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.<T>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> T execute(
private <T> T executeWithAttemptAndTimeoutCount(
ActionType actionType,
Action<T> action,
Options opts,
@@ -257,14 +256,14 @@ public class RetryHelper {
try {
RetryerBuilder<T> 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> T execute(ActionType actionType, Action<T> action, Retryer<T> retryer)
private <T> T executeWithTimeoutCount(ActionType actionType, Action<T> action, Retryer<T> retryer)
throws Throwable {
try {
return retryer.call(() -> action.call());