opendev/gerrit
Code Issues Proposed changes

Merge changes from topics "remove-redundant-specification-of-LockFailureException-for-retrying", "remove-retrying-index-queries-on-StorageException", "simplify-retrying-index-queries"

Browse Source 
* changes:
  ExceptionHook: Provide action type/name to the methods that decide on retries
  RetryHelper: Allow arbitrary string as action type
  Reduce boilerplate code to execute index queries with retry
  Remove retrying index queries on StorageException
  Remove redundant specification of LockFailureExceptions for retrying
This commit is contained in:
Edwin Kempin
2019-12-11 13:07:11 +00:00
committed by Gerrit Code Review
parent c902572b5b 10e36459b2
commit 6b92574bf2
14 changed files with 296 additions and 155 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
java/com/google/gerrit/gpg/server/PostGpgKeys.java
View File

@@ -40,7 +40,6 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.git.LockFailureException;
import com.google.gerrit.gpg.CheckResult;
import com.google.gerrit.gpg.Fingerprint;
import com.google.gerrit.gpg.GerritPublicKeyChecker;
@@ -208,7 +207,6 @@ public class PostGpgKeys implements RestModifyView<AccountResource, GpgKeysInput
try {
retryHelper
.accountUpdate("storeGpgKeys", () -> tryStoreKeys(rsrc, keyRings, toRemove))
.retryOn(LockFailureException.class::isInstance)
.call();
} catch (Exception e) {
Throwables.throwIfUnchecked(e);

12
java/com/google/gerrit/server/ExceptionHook.java
View File

@@ -38,12 +38,14 @@ public interface ExceptionHook {
* retry of the operation has a chance to succeed.
*
* <p>If {@code false} is returned the operation is still retried once to capture a trace, unless
* {@link #skipRetryWithTrace(Throwable)} skips the auto-retry.
* {@link #skipRetryWithTrace(String, String, Throwable)} skips the auto-retry.
*
* @param throwable throwable that was thrown while executing the operation
* @param actionType the type of the action for which the exception occurred
* @param actionName the name of the action for which the exception occurred
* @return whether the operation should be retried
*/
default boolean shouldRetry(Throwable throwable) {
default boolean shouldRetry(String actionType, String actionName, Throwable throwable) {
return false;
}
@@ -54,7 +56,7 @@ public interface ExceptionHook {
* com.google.gerrit.server.update.RetryHelper}.
*
* <p>This method is only called for exceptions for which the operation should not be retried
* ({@link #shouldRetry(Throwable)} returned {@code false}).
* ({@link #shouldRetry(String, String, Throwable)} returned {@code false}).
*
* <p>By default this method returns {@code false}, so that by default traces for unexpected
* exceptions are captured, which allows to investigate them.
@@ -68,10 +70,12 @@ public interface ExceptionHook {
* retry.retryWithTraceOnFailure} in {@code gerrit.config} is set to {@code true}).
*
* @param throwable throwable that was thrown while executing the operation
* @param actionType the type of the action for which the exception occurred
* @param actionName the name of the action for which the exception occurred
* @return whether auto-retrying of an operation with tracing should be skipped for the given
* throwable
*/
default boolean skipRetryWithTrace(Throwable throwable) {
default boolean skipRetryWithTrace(String actionType, String actionName, Throwable throwable) {
return false;
}

2
java/com/google/gerrit/server/ExceptionHookImpl.java
View File

@@ -31,7 +31,7 @@ public class ExceptionHookImpl implements ExceptionHook {
+ "Please retry later.";
@Override
public boolean shouldRetry(Throwable throwable) {
public boolean shouldRetry(String actionType, String actionName, Throwable throwable) {
return isLockFailure(throwable);
}

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

@@ -420,10 +420,7 @@ public class AccountsUpdate {
private Optional<AccountState> executeAccountUpdate(Action<Optional<AccountState>> action)
throws IOException, ConfigInvalidException {
try {
return retryHelper
.accountUpdate("updateAccount", action)
.retryOn(LockFailureException.class::isInstance)
.call();
return retryHelper.accountUpdate("updateAccount", action).call();
} catch (Exception e) {
Throwables.throwIfUnchecked(e);
Throwables.throwIfInstanceOf(e, IOException.class);

37
java/com/google/gerrit/server/account/Emails.java
View File

@@ -17,21 +17,16 @@ package com.google.gerrit.server.account;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.SetMultimap;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.UserIdentity;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.query.account.InternalAccountQuery;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryableAction.Action;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.sql.Timestamp;
@@ -44,16 +39,11 @@ import org.eclipse.jgit.lib.PersonIdent;
@Singleton
public class Emails {
private final ExternalIds externalIds;
private final Provider<InternalAccountQuery> queryProvider;
private final RetryHelper retryHelper;
@Inject
public Emails(
ExternalIds externalIds,
Provider<InternalAccountQuery> queryProvider,
RetryHelper retryHelper) {
public Emails(ExternalIds externalIds, RetryHelper retryHelper) {
this.externalIds = externalIds;
this.queryProvider = queryProvider;
this.retryHelper = retryHelper;
}
@@ -84,9 +74,9 @@ public class Emails {
return accounts;
}
return executeIndexQuery(
"queryAccountsByPreferredEmail",
() -> queryProvider.get().byPreferredEmail(email).stream())
return retryHelper
.accountIndexQuery("queryAccountsByPreferredEmail", q -> q.byPreferredEmail(email)).call()
.stream()
.map(a -> a.account().id())
.collect(toImmutableSet());
}
@@ -105,9 +95,10 @@ public class Emails {
List<String> emailsToBackfill =
Arrays.stream(emails).filter(e -> !result.containsKey(e)).collect(toImmutableList());
if (!emailsToBackfill.isEmpty()) {
executeIndexQuery(
"queryAccountsByPreferredEmails",
() -> queryProvider.get().byPreferredEmail(emailsToBackfill).entries().stream())
retryHelper
.accountIndexQuery(
"queryAccountsByPreferredEmails", q -> q.byPreferredEmail(emailsToBackfill))
.call().entries().stream()
.forEach(e -> result.put(e.getKey(), e.getValue().account().id()));
}
return ImmutableSetMultimap.copyOf(result);
@@ -140,16 +131,4 @@ public class Emails {
return u;
}
private <T> T executeIndexQuery(String actionName, Action<T> action) {
try {
return retryHelper
.indexQuery(actionName, action)
.retryOn(StorageException.class::isInstance)
.call();
} catch (Exception e) {
Throwables.throwIfUnchecked(e);
throw new StorageException(e);
}
}
}

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

@@ -43,7 +43,6 @@ import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_RE
import com.google.common.base.Joiner;
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;
@@ -167,7 +166,6 @@ 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.RetryableAction.Action;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.util.MagicBranch;
@@ -3315,9 +3313,11 @@ class ReceiveCommits {
for (String changeId : c.getFooterLines(FooterConstants.CHANGE_ID)) {
if (byKey == null) {
byKey =
executeIndexQuery(
"queryOpenChangesByKeyByBranch",
() -> openChangesByKeyByBranch(branch));
retryHelper
.changeIndexQuery(
"queryOpenChangesByKeyByBranch",
q -> openChangesByKeyByBranch(q, branch))
.call();
}
ChangeNotes onto = byKey.get(Change.key(changeId.trim()));
@@ -3397,23 +3397,12 @@ class ReceiveCommits {
}
}
private <T> T executeIndexQuery(String actionName, Action<T> action) {
try (TraceTimer traceTimer = newTimer("executeIndexQuery")) {
return retryHelper
.indexQuery(actionName, action)
.retryOn(StorageException.class::isInstance)
.call();
} catch (Exception e) {
Throwables.throwIfUnchecked(e);
throw new StorageException(e);
}
}
private Map<Change.Key, ChangeNotes> openChangesByKeyByBranch(BranchNameKey branch) {
private Map<Change.Key, ChangeNotes> openChangesByKeyByBranch(
InternalChangeQuery internalChangeQuery, BranchNameKey branch) {
try (TraceTimer traceTimer =
newTimer("openChangesByKeyByBranch", Metadata.builder().branchName(branch.branch()))) {
Map<Change.Key, ChangeNotes> r = new HashMap<>();
for (ChangeData cd : queryProvider.get().byBranchOpen(branch)) {
for (ChangeData cd : internalChangeQuery.byBranchOpen(branch)) {
try {
r.put(cd.change().getKey(), cd.notes());
} catch (NoSuchChangeException e) {

3
java/com/google/gerrit/server/group/db/GroupsUpdate.java
View File

@@ -26,7 +26,6 @@ import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.DuplicateKeyException;
import com.google.gerrit.exceptions.NoSuchGroupException;
import com.google.gerrit.git.LockFailureException;
import com.google.gerrit.git.RefUpdateUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
@@ -311,7 +310,6 @@ public class GroupsUpdate {
try {
return retryHelper
.groupUpdate("createGroup", () -> createGroupInNoteDb(groupCreation, groupUpdate))
.retryOn(LockFailureException.class::isInstance)
.call();
} catch (Exception e) {
Throwables.throwIfUnchecked(e);
@@ -351,7 +349,6 @@ public class GroupsUpdate {
try {
return retryHelper
.groupUpdate("updateGroup", () -> updateGroupInNoteDb(groupUuid, groupUpdate))
.retryOn(LockFailureException.class::isInstance)
.call();
} catch (Exception e) {
Throwables.throwIfUnchecked(e);

15
java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java
View File

@@ -48,12 +48,10 @@ import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeIdPredicate;
import com.google.gerrit.server.query.change.CommitPredicate;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.query.change.ProjectPredicate;
import com.google.gerrit.server.query.change.RefPredicate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
@@ -75,7 +73,6 @@ public class ProjectsConsistencyChecker {
private final GitRepositoryManager repoManager;
private final RetryHelper retryHelper;
private final Provider<InternalChangeQuery> changeQueryProvider;
private final ChangeJson.Factory changeJsonFactory;
private final IndexConfig indexConfig;
@@ -83,12 +80,10 @@ public class ProjectsConsistencyChecker {
ProjectsConsistencyChecker(
GitRepositoryManager repoManager,
RetryHelper retryHelper,
Provider<InternalChangeQuery> changeQueryProvider,
ChangeJson.Factory changeJsonFactory,
IndexConfig indexConfig) {
this.repoManager = repoManager;
this.retryHelper = retryHelper;
this.changeQueryProvider = changeQueryProvider;
this.changeJsonFactory = changeJsonFactory;
this.indexConfig = indexConfig;
}
@@ -264,14 +259,11 @@ public class ProjectsConsistencyChecker {
try {
List<ChangeData> queryResult =
retryHelper
.indexQuery(
.changeIndexQuery(
"projectsConsistencyCheckerQueryChanges",
() ->
changeQueryProvider
.get()
.setRequestedFields(ChangeField.CHANGE, ChangeField.PATCH_SET)
q ->
q.setRequestedFields(ChangeField.CHANGE, ChangeField.PATCH_SET)
.query(and(basePredicate, or(predicates))))
.retryOn(StorageException.class::isInstance)
.call();
// Result for this query that we want to return to the client.
@@ -308,7 +300,6 @@ public class ProjectsConsistencyChecker {
}
return null;
})
.retryOn(StorageException.class::isInstance)
.call();
}
}

40
java/com/google/gerrit/server/restapi/project/CommitsCollection.java
View File

@@ -16,10 +16,8 @@ package com.google.gerrit.server.restapi.project;
import static com.google.common.collect.ImmutableList.toImmutableList;
import com.google.common.base.Throwables;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.ChildCollection;
import com.google.gerrit.extensions.restapi.IdString;
@@ -35,12 +33,9 @@ import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.Reachable;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.CommitPredicate;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.query.change.ProjectPredicate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryableAction.Action;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Arrays;
@@ -62,7 +57,6 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
private final GitRepositoryManager repoManager;
private final RetryHelper retryHelper;
private final ChangeIndexCollection indexes;
private final Provider<InternalChangeQuery> queryProvider;
private final Reachable reachable;
@Inject
@@ -71,13 +65,11 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
GitRepositoryManager repoManager,
RetryHelper retryHelper,
ChangeIndexCollection indexes,
Provider<InternalChangeQuery> queryProvider,
Reachable reachable) {
this.views = views;
this.repoManager = repoManager;
this.retryHelper = retryHelper;
this.indexes = indexes;
this.queryProvider = queryProvider;
this.reachable = reachable;
}
@@ -130,14 +122,11 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
// Check first if any patchset of any change references the commit in question. This is much
// cheaper than ref visibility filtering and reachability computation.
List<ChangeData> changes =
executeIndexQuery(
"queryChangesByProjectCommitWithLimit1",
() ->
queryProvider
.get()
.enforceVisibility(true)
.setLimit(1)
.byProjectCommit(project, commit));
retryHelper
.changeIndexQuery(
"queryChangesByProjectCommitWithLimit1",
q -> q.enforceVisibility(true).setLimit(1).byProjectCommit(project, commit))
.call();
if (!changes.isEmpty()) {
return true;
}
@@ -152,9 +141,10 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
.map(parent -> new CommitPredicate(parent.getId().getName()))
.collect(toImmutableList())));
changes =
executeIndexQuery(
"queryChangesByProjectCommit",
() -> queryProvider.get().enforceVisibility(true).query(pred));
retryHelper
.changeIndexQuery(
"queryChangesByProjectCommit", q -> q.enforceVisibility(true).query(pred))
.call();
Set<Ref> branchesForCommitParents = new HashSet<>(changes.size());
for (ChangeData cd : changes) {
@@ -177,16 +167,4 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
.collect(toImmutableList());
return reachable.fromRefs(project, repo, commit, refs);
}
private <T> T executeIndexQuery(String actionName, Action<T> action) {
try {
return retryHelper
.indexQuery(actionName, action)
.retryOn(StorageException.class::isInstance)
.call();
} catch (Exception e) {
Throwables.throwIfUnchecked(e);
throw new StorageException(e);
}
}
}

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

@@ -28,9 +28,9 @@ import com.github.rholder.retry.WaitStrategies;
import com.github.rholder.retry.WaitStrategy;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Maps;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.UsedAt;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.metrics.Counter3;
import com.google.gerrit.metrics.Description;
@@ -42,13 +42,18 @@ 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.gerrit.server.query.account.InternalAccountQuery;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.update.RetryableAction.Action;
import com.google.gerrit.server.update.RetryableAction.ActionType;
import com.google.gerrit.server.update.RetryableChangeAction.ChangeAction;
import com.google.gerrit.server.update.RetryableIndexQueryAction.IndexQueryAction;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.time.Duration;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ExecutionException;
@@ -107,15 +112,15 @@ public class RetryHelper {
@VisibleForTesting
@Singleton
public static class Metrics {
final Counter3<ActionType, String, String> attemptCounts;
final Counter3<ActionType, String, String> timeoutCount;
final Counter3<ActionType, String, String> autoRetryCount;
final Counter3<ActionType, String, String> failuresOnAutoRetryCount;
final Counter3<String, String, String> attemptCounts;
final Counter3<String, String, String> timeoutCount;
final Counter3<String, String, String> autoRetryCount;
final Counter3<String, String, String> failuresOnAutoRetryCount;
@Inject
Metrics(MetricMaker metricMaker) {
Field<ActionType> actionTypeField =
Field.ofEnum(ActionType.class, "action_type", Metadata.Builder::actionType).build();
Field<String> actionTypeField =
Field.ofString("action_type", Metadata.Builder::actionType).build();
Field<String> operationNameField =
Field.ofString("operation_name", Metadata.Builder::operationName)
.description("The name of the operation that was retried.")
@@ -175,10 +180,14 @@ public class RetryHelper {
return new AutoValue_RetryHelper_Options.Builder();
}
private final Config cfg;
private final Metrics metrics;
private final BatchUpdate.Factory updateFactory;
private final Provider<InternalAccountQuery> internalAccountQuery;
private final Provider<InternalChangeQuery> internalChangeQuery;
private final PluginSetContext<ExceptionHook> exceptionHooks;
private final Map<ActionType, Duration> defaultTimeouts;
private final Duration defaultTimeout;
private final Map<String, Duration> defaultTimeouts;
private final WaitStrategy waitStrategy;
@Nullable private final Consumer<RetryerBuilder<?>> overwriteDefaultRetryerStrategySetup;
private final boolean retryWithTraceOnFailure;
@@ -188,8 +197,17 @@ public class RetryHelper {
@GerritServerConfig Config cfg,
Metrics metrics,
PluginSetContext<ExceptionHook> exceptionHooks,
BatchUpdate.Factory updateFactory) {
this(cfg, metrics, updateFactory, exceptionHooks, null);
BatchUpdate.Factory updateFactory,
Provider<InternalAccountQuery> internalAccountQuery,
Provider<InternalChangeQuery> internalChangeQuery) {
this(
cfg,
metrics,
updateFactory,
internalAccountQuery,
internalChangeQuery,
exceptionHooks,
null);
}
@VisibleForTesting
@@ -197,21 +215,25 @@ public class RetryHelper {
@GerritServerConfig Config cfg,
Metrics metrics,
BatchUpdate.Factory updateFactory,
Provider<InternalAccountQuery> internalAccountQuery,
Provider<InternalChangeQuery> internalChangeQuery,
PluginSetContext<ExceptionHook> exceptionHooks,
@Nullable Consumer<RetryerBuilder<?>> overwriteDefaultRetryerStrategySetup) {
this.cfg = cfg;
this.metrics = metrics;
this.updateFactory = updateFactory;
this.internalAccountQuery = internalAccountQuery;
this.internalChangeQuery = internalChangeQuery;
this.exceptionHooks = exceptionHooks;
Duration defaultTimeout =
this.defaultTimeout =
Duration.ofMillis(
cfg.getTimeUnit("retry", null, "timeout", SECONDS.toMillis(20), MILLISECONDS));
this.defaultTimeouts = Maps.newEnumMap(ActionType.class);
this.defaultTimeouts = new HashMap<>();
Arrays.stream(ActionType.values())
.forEach(
at ->
defaultTimeouts.put(
at,
at.name(),
Duration.ofMillis(
cfg.getTimeUnit(
"retry",
@@ -230,6 +252,25 @@ public class RetryHelper {
this.retryWithTraceOnFailure = cfg.getBoolean("retry", "retryWithTraceOnFailure", false);
}
/**
* Creates an action that is executed with retrying when called.
*
* <p>This method allows to use a custom action type. If the action type is one of {@link
* ActionType} the usage of {@link #action(ActionType, String, Action)} is preferred.
*
* <p>The action type is used as metric bucket and decides which default timeout is used.
*
* @param actionType the type of the action, used as metric bucket
* @param actionName the name of the action, used as metric bucket
* @param action the action that should be executed
* @return the retryable action, callers need to call {@link RetryableAction#call()} to execute
* the action
*/
@UsedAt(UsedAt.Project.GOOGLE)
public <T> RetryableAction<T> action(String actionType, String actionName, Action<T> action) {
return new RetryableAction<>(this, actionType, actionName, action);
}
/**
* Creates an action that is executed with retrying when called.
*
@@ -309,19 +350,85 @@ public class RetryHelper {
}
/**
* Creates an action for querying an index that is executed with retrying when called.
* Creates an action for querying the account index that is executed with retrying when called.
*
* <p>The index query action gets a {@link InternalAccountQuery} provided that can be used to
* query the account index.
*
* @param actionName the name of the action, used as metric bucket
* @param action the action that should be executed
* @return the retryable action, callers need to call {@link RetryableAction#call()} to execute
* the action
* @param indexQueryAction the action that should be executed
* @return the retryable action, callers need to call {@link RetryableIndexQueryAction#call()} to
* execute the action
*/
public <T> RetryableAction<T> indexQuery(String actionName, Action<T> action) {
return new RetryableAction<>(this, ActionType.INDEX_QUERY, actionName, action);
public <T> RetryableIndexQueryAction<InternalAccountQuery, T> accountIndexQuery(
String actionName, IndexQueryAction<T, InternalAccountQuery> indexQueryAction) {
return new RetryableIndexQueryAction<>(
this, internalAccountQuery.get(), actionName, indexQueryAction);
}
Duration getDefaultTimeout(ActionType actionType) {
return defaultTimeouts.get(actionType);
/**
* Creates an action for querying the change index that is executed with retrying when called.
*
* <p>The index query action gets a {@link InternalChangeQuery} provided that can be used to query
* the change index.
*
* @param actionName the name of the action, used as metric bucket
* @param indexQueryAction the action that should be executed
* @return the retryable action, callers need to call {@link RetryableIndexQueryAction#call()} to
* execute the action
*/
public <T> RetryableIndexQueryAction<InternalChangeQuery, T> changeIndexQuery(
String actionName, IndexQueryAction<T, InternalChangeQuery> indexQueryAction) {
return new RetryableIndexQueryAction<>(
this, internalChangeQuery.get(), actionName, indexQueryAction);
}
/**
* Returns the default timeout for an action type.
*
* <p>The default timeout for an action type is defined by the 'retry.<action-type>.timeout'
* parameter in gerrit.config. If this parameter is not set the value from the 'retry.timeout'
* parameter is used (if this is also not set we fall back to to a hard-coded timeout of 20s).
*
* <p>Callers can overwrite the default timeout by setting another timeout in the {@link Options},
* see {@link Options#timeout()}.
*
* @param actionType the action type for which the default timeout should be retrieved
* @return the default timeout for the given action type
*/
Duration getDefaultTimeout(String actionType) {
Duration timeout = defaultTimeouts.get(actionType);
if (timeout != null) {
return timeout;
}
return readDefaultTimeoutFromConfig(actionType);
}
/**
* Thread-safe method to read and cache a default timeout from gerrit.config.
*
* <p>After reading the default timeout from gerrit.config it is cached in the {@link
* #defaultTimeouts} map, so that it's read only once.
*
* @param actionType the action type for which the default timeout should be retrieved
* @return the default timeout for the given action type
*/
private synchronized Duration readDefaultTimeoutFromConfig(String actionType) {
Duration timeout = defaultTimeouts.get(actionType);
if (timeout != null) {
// some other thread has read the default timeout from the config in the meantime
return timeout;
}
timeout =
Duration.ofMillis(
cfg.getTimeUnit(
"retry",
actionType,
"timeout",
SECONDS.toMillis(defaultTimeout.getSeconds()),
MILLISECONDS));
defaultTimeouts.put(actionType, timeout);
return timeout;
}
/**
@@ -336,10 +443,7 @@ public class RetryHelper {
* catch and inspect this Throwable to decide carefully whether it should be re-thrown
*/
<T> T execute(
ActionType actionType,
Action<T> action,
Options opts,
Predicate<Throwable> exceptionPredicate)
String actionType, Action<T> action, Options opts, Predicate<Throwable> exceptionPredicate)
throws Throwable {
MetricListener listener = new MetricListener();
try (TraceContext traceContext = TraceContext.open()) {
@@ -354,8 +458,11 @@ public class RetryHelper {
return true;
}
String actionName = opts.caller().orElse("N/A");
// Exception hooks may identify additional exceptions for retry.
if (exceptionHooks.stream().anyMatch(h -> h.shouldRetry(t))) {
if (exceptionHooks.stream()
.anyMatch(h -> h.shouldRetry(actionType, actionName, t))) {
return true;
}
@@ -366,19 +473,19 @@ public class RetryHelper {
&& opts.retryWithTrace().get().test(t)) {
// Exception hooks may identify exceptions for which retrying with trace should be
// skipped.
if (exceptionHooks.stream().anyMatch(h -> h.skipRetryWithTrace(t))) {
if (exceptionHooks.stream()
.anyMatch(h -> h.skipRetryWithTrace(actionType, actionName, t))) {
return false;
}
String caller = opts.caller().orElse("N/A");
String cause = formatCause(t);
if (!traceContext.isTracing()) {
String traceId = "retry-on-failure-" + new RequestId();
traceContext.addTag(RequestId.Type.TRACE_ID, traceId).forceLogging();
opts.onAutoTrace().ifPresent(c -> c.accept(traceId));
logger.atFine().withCause(t).log(
"AutoRetry: %s failed, retry with tracing enabled", caller);
metrics.autoRetryCount.increment(actionType, caller, cause);
"AutoRetry: %s failed, retry with tracing enabled", actionName);
metrics.autoRetryCount.increment(actionType, actionName, cause);
return true;
}
@@ -386,8 +493,8 @@ public class RetryHelper {
// enabled and it failed again. Log the failure so that admin can see if it
// differs from the failure that triggered the retry.
logger.atFine().withCause(t).log(
"AutoRetry: auto-retry of %s has failed", caller);
metrics.failuresOnAutoRetryCount.increment(actionType, caller, cause);
"AutoRetry: auto-retry of %s has failed", actionName);
metrics.failuresOnAutoRetryCount.increment(actionType, actionName, cause);
return false;
}
@@ -443,7 +550,7 @@ public class RetryHelper {
* catch and inspect this Throwable to decide carefully whether it should be re-thrown
*/
private <T> T executeWithTimeoutCount(
ActionType actionType, Action<T> action, Options opts, Retryer<T> retryer) throws Throwable {
String actionType, Action<T> action, Options opts, Retryer<T> retryer) throws Throwable {
try {
return retryer.call(action::call);
} catch (ExecutionException | RetryException e) {
@@ -461,7 +568,7 @@ public class RetryHelper {
}
private <O> RetryerBuilder<O> createRetryerBuilder(
ActionType actionType, Options opts, Predicate<Throwable> exceptionPredicate) {
String actionType, Options opts, Predicate<Throwable> exceptionPredicate) {
RetryerBuilder<O> retryerBuilder =
RetryerBuilder.<O>newBuilder().retryIfException(exceptionPredicate::test);
if (opts.listener() != null) {

19
java/com/google/gerrit/server/update/RetryableAction.java
View File

@@ -30,12 +30,11 @@ import java.util.function.Predicate;
* <p>Instances of this class are created via {@link RetryHelper} (see {@link
* RetryHelper#action(ActionType, String, Action)}, {@link RetryHelper#accountUpdate(String,
* Action)}, {@link RetryHelper#changeUpdate(String, Action)}, {@link
* RetryHelper#groupUpdate(String, Action)}, {@link RetryHelper#pluginUpdate(String, Action)},
* {@link RetryHelper#indexQuery(String, Action)}).
* RetryHelper#groupUpdate(String, Action)}, {@link RetryHelper#pluginUpdate(String, Action)}).
*
* <p>Which exceptions cause a retry is controlled by {@link ExceptionHook#shouldRetry(Throwable)}.
* In addition callers can specify additional exception that should cause a retry via {@link
* #retryOn(Predicate)}.
* <p>Which exceptions cause a retry is controlled by {@link ExceptionHook#shouldRetry(String,
* String, Throwable)}. In addition callers can specify additional exception that should cause a
* retry via {@link #retryOn(Predicate)}.
*/
public class RetryableAction<T> {
public enum ActionType {
@@ -54,13 +53,17 @@ public class RetryableAction<T> {
}
private final RetryHelper retryHelper;
private final ActionType actionType;
private final String actionType;
private final Action<T> action;
private final RetryHelper.Options.Builder options = RetryHelper.options();
private final List<Predicate<Throwable>> exceptionPredicates = new ArrayList<>();
RetryableAction(
RetryHelper retryHelper, ActionType actionType, String actionName, Action<T> action) {
this(retryHelper, requireNonNull(actionType, "actionType").name(), actionName, action);
}
RetryableAction(RetryHelper retryHelper, String actionType, String actionName, Action<T> action) {
this.retryHelper = requireNonNull(retryHelper, "retryHelper");
this.actionType = requireNonNull(actionType, "actionType");
this.action = requireNonNull(action, "action");
@@ -71,8 +74,8 @@ public class RetryableAction<T> {
* Adds an additional condition that should trigger retries.
*
* <p>For some exceptions retrying is enabled globally (see {@link
* ExceptionHook#shouldRetry(Throwable)}). Conditions for those exceptions do not need to be
* specified here again.
* ExceptionHook#shouldRetry(String, String, Throwable)}). Conditions for those exceptions do not
* need to be specified here again.
*
* <p>This method can be invoked multiple times to add further conditions that should trigger
* retries.

90
java/com/google/gerrit/server/update/RetryableIndexQueryAction.java Normal file
View File

@@ -0,0 +1,90 @@
// 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.update;
import com.github.rholder.retry.RetryListener;
import com.google.common.base.Throwables;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.index.query.InternalQuery;
import java.util.function.Consumer;
import java.util.function.Predicate;
/**
* An action to query an index that is executed with retrying.
*
* <p>Instances of this class are created via {@link RetryHelper#accountIndexQuery(String,
* IndexQueryAction)} and {@link RetryHelper#changeIndexQuery(String, IndexQueryAction)}.
*
* <p>In contrast to normal {@link RetryableAction.Action}s that are called via {@link
* RetryableAction} {@link IndexQueryAction}s get a {@link InternalQuery} provided.
*
* <p>In addition when an index query action is called any exception that is not an unchecked
* exception gets wrapped into an {@link StorageException}.
*/
public class RetryableIndexQueryAction<Q extends InternalQuery<?, Q>, T>
extends RetryableAction<T> {
@FunctionalInterface
public interface IndexQueryAction<T, Q> {
T call(Q internalQuery) throws Exception;
}
RetryableIndexQueryAction(
RetryHelper retryHelper,
Q internalQuery,
String actionName,
IndexQueryAction<T, Q> indexQuery) {
super(retryHelper, ActionType.INDEX_QUERY, actionName, () -> indexQuery.call(internalQuery));
}
@Override
public RetryableIndexQueryAction<Q, T> retryOn(Predicate<Throwable> exceptionPredicate) {
super.retryOn(exceptionPredicate);
return this;
}
@Override
public RetryableIndexQueryAction<Q, T> retryWithTrace(Predicate<Throwable> exceptionPredicate) {
super.retryWithTrace(exceptionPredicate);
return this;
}
@Override
public RetryableIndexQueryAction<Q, T> onAutoTrace(Consumer<String> traceIdConsumer) {
super.onAutoTrace(traceIdConsumer);
return this;
}
@Override
public RetryableIndexQueryAction<Q, T> listener(RetryListener retryListener) {
super.listener(retryListener);
return this;
}
@Override
public RetryableIndexQueryAction<Q, T> defaultTimeoutMultiplier(int multiplier) {
super.defaultTimeoutMultiplier(multiplier);
return this;
}
@Override
public T call() {
try {
return super.call();
} catch (Throwable t) {
Throwables.throwIfUnchecked(t);
throw new StorageException(t);
}
}
}

20
javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -105,7 +105,6 @@ import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.SshKeyInfo;
import com.google.gerrit.extensions.events.AccountIndexedListener;
import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -117,6 +116,7 @@ import com.google.gerrit.gpg.Fingerprint;
import com.google.gerrit.gpg.PublicKeyStore;
import com.google.gerrit.gpg.testing.TestKey;
import com.google.gerrit.mail.Address;
import com.google.gerrit.server.ExceptionHook;
import com.google.gerrit.server.ServerInitiated;
import com.google.gerrit.server.account.AccountProperties;
import com.google.gerrit.server.account.AccountState;
@@ -135,7 +135,6 @@ import com.google.gerrit.server.index.account.StalenessChecker;
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;
@@ -221,6 +220,7 @@ public class AccountIT extends AbstractDaemonTest {
@Inject private VersionedAuthorizedKeys.Accessor authorizedKeys;
@Inject private PermissionBackend permissionBackend;
@Inject private ExtensionRegistry extensionRegistry;
@Inject private PluginSetContext<ExceptionHook> exceptionHooks;
@Inject protected Emails emails;
@@ -2761,7 +2761,9 @@ public class AccountIT extends AbstractDaemonTest {
cfg,
retryMetrics,
null,
new PluginSetContext<>(DynamicSet.emptySet(), PluginMetrics.DISABLED_INSTANCE),
null,
null,
exceptionHooks,
r -> r.withBlockStrategy(noSleepBlockStrategy)),
extIdNotesFactory,
ident,
@@ -2815,7 +2817,9 @@ public class AccountIT extends AbstractDaemonTest {
cfg,
retryMetrics,
null,
new PluginSetContext<>(DynamicSet.emptySet(), PluginMetrics.DISABLED_INSTANCE),
null,
null,
exceptionHooks,
r ->
r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size()))
.withBlockStrategy(noSleepBlockStrategy)),
@@ -2873,7 +2877,9 @@ public class AccountIT extends AbstractDaemonTest {
cfg,
retryMetrics,
null,
new PluginSetContext<>(DynamicSet.emptySet(), PluginMetrics.DISABLED_INSTANCE),
null,
null,
exceptionHooks,
r -> r.withBlockStrategy(noSleepBlockStrategy)),
extIdNotesFactory,
ident,
@@ -2946,7 +2952,9 @@ public class AccountIT extends AbstractDaemonTest {
cfg,
retryMetrics,
null,
new PluginSetContext<>(DynamicSet.emptySet(), PluginMetrics.DISABLED_INSTANCE),
null,
null,
exceptionHooks,
r -> r.withBlockStrategy(noSleepBlockStrategy)),
extIdNotesFactory,
ident,

2
javatests/com/google/gerrit/acceptance/rest/TraceIT.java
View File

@@ -743,7 +743,7 @@ public class TraceIT extends AbstractDaemonTest {
.add(
new ExceptionHook() {
@Override
public boolean shouldRetry(Throwable t) {
public boolean shouldRetry(String actionType, String actionName, Throwable t) {
return true;
}
})) {