diff --git a/java/com/google/gerrit/server/account/Emails.java b/java/com/google/gerrit/server/account/Emails.java index 8262b4dc37..98d0d5006e 100644 --- a/java/com/google/gerrit/server/account/Emails.java +++ b/java/com/google/gerrit/server/account/Emails.java @@ -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 queryProvider; private final RetryHelper retryHelper; @Inject - public Emails( - ExternalIds externalIds, - Provider 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 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,13 +131,4 @@ public class Emails { return u; } - - private T executeIndexQuery(String actionName, Action action) { - try { - return retryHelper.indexQuery(actionName, action).call(); - } catch (Exception e) { - Throwables.throwIfUnchecked(e); - throw new StorageException(e); - } - } } diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index db159423e5..5f6d4fd36f 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -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,20 +3397,12 @@ class ReceiveCommits { } } - private T executeIndexQuery(String actionName, Action action) { - try (TraceTimer traceTimer = newTimer("executeIndexQuery")) { - return retryHelper.indexQuery(actionName, action).call(); - } catch (Exception e) { - Throwables.throwIfUnchecked(e); - throw new StorageException(e); - } - } - - private Map openChangesByKeyByBranch(BranchNameKey branch) { + private Map openChangesByKeyByBranch( + InternalChangeQuery internalChangeQuery, BranchNameKey branch) { try (TraceTimer traceTimer = newTimer("openChangesByKeyByBranch", Metadata.builder().branchName(branch.branch()))) { Map 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) { diff --git a/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java b/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java index aaa2582f77..110beaf944 100644 --- a/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java +++ b/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java @@ -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 changeQueryProvider; private final ChangeJson.Factory changeJsonFactory; private final IndexConfig indexConfig; @@ -83,12 +80,10 @@ public class ProjectsConsistencyChecker { ProjectsConsistencyChecker( GitRepositoryManager repoManager, RetryHelper retryHelper, - Provider changeQueryProvider, ChangeJson.Factory changeJsonFactory, IndexConfig indexConfig) { this.repoManager = repoManager; this.retryHelper = retryHelper; - this.changeQueryProvider = changeQueryProvider; this.changeJsonFactory = changeJsonFactory; this.indexConfig = indexConfig; } @@ -264,12 +259,10 @@ public class ProjectsConsistencyChecker { try { List 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)))) .call(); diff --git a/java/com/google/gerrit/server/restapi/project/CommitsCollection.java b/java/com/google/gerrit/server/restapi/project/CommitsCollection.java index 8ae04a0dd4..31fa8d3729 100644 --- a/java/com/google/gerrit/server/restapi/project/CommitsCollection.java +++ b/java/com/google/gerrit/server/restapi/project/CommitsCollection.java @@ -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 queryProvider; private final Reachable reachable; @Inject @@ -71,13 +65,11 @@ public class CommitsCollection implements ChildCollection 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 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 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 branchesForCommitParents = new HashSet<>(changes.size()); for (ChangeData cd : changes) { @@ -177,13 +167,4 @@ public class CommitsCollection implements ChildCollection T executeIndexQuery(String actionName, Action action) { - try { - return retryHelper.indexQuery(actionName, action).call(); - } catch (Exception e) { - Throwables.throwIfUnchecked(e); - throw new StorageException(e); - } - } } diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java index 65fd97e819..a2dd32b44e 100644 --- a/java/com/google/gerrit/server/update/RetryHelper.java +++ b/java/com/google/gerrit/server/update/RetryHelper.java @@ -42,10 +42,14 @@ 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; @@ -177,6 +181,8 @@ public class RetryHelper { private final Metrics metrics; private final BatchUpdate.Factory updateFactory; + private final Provider internalAccountQuery; + private final Provider internalChangeQuery; private final PluginSetContext exceptionHooks; private final Map defaultTimeouts; private final WaitStrategy waitStrategy; @@ -188,8 +194,17 @@ public class RetryHelper { @GerritServerConfig Config cfg, Metrics metrics, PluginSetContext exceptionHooks, - BatchUpdate.Factory updateFactory) { - this(cfg, metrics, updateFactory, exceptionHooks, null); + BatchUpdate.Factory updateFactory, + Provider internalAccountQuery, + Provider internalChangeQuery) { + this( + cfg, + metrics, + updateFactory, + internalAccountQuery, + internalChangeQuery, + exceptionHooks, + null); } @VisibleForTesting @@ -197,10 +212,14 @@ public class RetryHelper { @GerritServerConfig Config cfg, Metrics metrics, BatchUpdate.Factory updateFactory, + Provider internalAccountQuery, + Provider internalChangeQuery, PluginSetContext exceptionHooks, @Nullable Consumer> overwriteDefaultRetryerStrategySetup) { this.metrics = metrics; this.updateFactory = updateFactory; + this.internalAccountQuery = internalAccountQuery; + this.internalChangeQuery = internalChangeQuery; this.exceptionHooks = exceptionHooks; Duration defaultTimeout = @@ -309,15 +328,37 @@ 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. + * + *

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 RetryableAction indexQuery(String actionName, Action action) { - return new RetryableAction<>(this, ActionType.INDEX_QUERY, actionName, action); + public RetryableIndexQueryAction accountIndexQuery( + String actionName, IndexQueryAction indexQueryAction) { + return new RetryableIndexQueryAction<>( + this, internalAccountQuery.get(), actionName, indexQueryAction); + } + + /** + * Creates an action for querying the change index that is executed with retrying when called. + * + *

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 RetryableIndexQueryAction changeIndexQuery( + String actionName, IndexQueryAction indexQueryAction) { + return new RetryableIndexQueryAction<>( + this, internalChangeQuery.get(), actionName, indexQueryAction); } Duration getDefaultTimeout(ActionType actionType) { diff --git a/java/com/google/gerrit/server/update/RetryableAction.java b/java/com/google/gerrit/server/update/RetryableAction.java index 3ee079c6ef..bd99e73e20 100644 --- a/java/com/google/gerrit/server/update/RetryableAction.java +++ b/java/com/google/gerrit/server/update/RetryableAction.java @@ -30,8 +30,7 @@ import java.util.function.Predicate; *

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)}). * *

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 diff --git a/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java b/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java new file mode 100644 index 0000000000..6e3d0e974d --- /dev/null +++ b/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java @@ -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. + * + *

Instances of this class are created via {@link RetryHelper#accountIndexQuery(String, + * IndexQueryAction)} and {@link RetryHelper#changeIndexQuery(String, IndexQueryAction)}. + * + *

In contrast to normal {@link RetryableAction.Action}s that are called via {@link + * RetryableAction} {@link IndexQueryAction}s get a {@link InternalQuery} provided. + * + *

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, T> + extends RetryableAction { + @FunctionalInterface + public interface IndexQueryAction { + T call(Q internalQuery) throws Exception; + } + + RetryableIndexQueryAction( + RetryHelper retryHelper, + Q internalQuery, + String actionName, + IndexQueryAction indexQuery) { + super(retryHelper, ActionType.INDEX_QUERY, actionName, () -> indexQuery.call(internalQuery)); + } + + @Override + public RetryableIndexQueryAction retryOn(Predicate exceptionPredicate) { + super.retryOn(exceptionPredicate); + return this; + } + + @Override + public RetryableIndexQueryAction retryWithTrace(Predicate exceptionPredicate) { + super.retryWithTrace(exceptionPredicate); + return this; + } + + @Override + public RetryableIndexQueryAction onAutoTrace(Consumer traceIdConsumer) { + super.onAutoTrace(traceIdConsumer); + return this; + } + + @Override + public RetryableIndexQueryAction listener(RetryListener retryListener) { + super.listener(retryListener); + return this; + } + + @Override + public RetryableIndexQueryAction 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); + } + } +} diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 7514bd952c..203f4a8cd9 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -2761,6 +2761,8 @@ public class AccountIT extends AbstractDaemonTest { cfg, retryMetrics, null, + null, + null, exceptionHooks, r -> r.withBlockStrategy(noSleepBlockStrategy)), extIdNotesFactory, @@ -2815,6 +2817,8 @@ public class AccountIT extends AbstractDaemonTest { cfg, retryMetrics, null, + null, + null, exceptionHooks, r -> r.withStopStrategy(StopStrategies.stopAfterAttempt(status.size())) @@ -2873,6 +2877,8 @@ public class AccountIT extends AbstractDaemonTest { cfg, retryMetrics, null, + null, + null, exceptionHooks, r -> r.withBlockStrategy(noSleepBlockStrategy)), extIdNotesFactory, @@ -2946,6 +2952,8 @@ public class AccountIT extends AbstractDaemonTest { cfg, retryMetrics, null, + null, + null, exceptionHooks, r -> r.withBlockStrategy(noSleepBlockStrategy)), extIdNotesFactory,