From aece3ffe751a351bd00c1cf748a054341982cd12 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 6 Dec 2019 16:25:51 +0100 Subject: [PATCH] Add fluent interface for retrying actions RetryHelper accumulated a lot of functionality and using it was not straight-forward for callers. Clean-up the class and add fluent interface for calling actions with retry: Object result = retryHelper.changeUpdate( "myActionName", batchUpdateFactory -> { try (BatchUpdate bu = newBatchUpdate(batchUpdateFactory)) { ... } return result; }) .retryOn(LockFailureException.class::isInstance) ... .call(); With the fluent interface providing an action name is now mandatory which makes the retry metrics more useful. Signed-off-by: Edwin Kempin Change-Id: Iecdfa5b153ab17f31c8ec1d2dca82b428fcf5800 --- .../google/gerrit/gpg/server/PostGpgKeys.java | 9 +- .../gerrit/httpd/restapi/RestApiServlet.java | 30 +-- .../gerrit/server/account/AccountsUpdate.java | 9 +- .../google/gerrit/server/account/Emails.java | 16 +- .../server/change/ChangeCleanupRunner.java | 13 +- .../server/change/ConsistencyChecker.java | 43 ++-- .../server/git/receive/ReceiveCommits.java | 223 +++++++++--------- .../gerrit/server/group/db/GroupsUpdate.java | 16 +- .../server/mail/receive/MailProcessor.java | 13 +- .../project/ProjectsConsistencyChecker.java | 74 +++--- .../restapi/project/CommitsCollection.java | 17 +- .../google/gerrit/server/submit/MergeOp.java | 69 +++--- .../gerrit/server/update/RetryHelper.java | 161 +++++++------ .../gerrit/server/update/RetryableAction.java | 168 +++++++++++++ .../server/update/RetryableChangeAction.java | 92 ++++++++ .../server/notedb/NoteDbOnlyIT.java | 24 +- plugins/reviewnotes | 2 +- 17 files changed, 647 insertions(+), 332 deletions(-) create mode 100644 java/com/google/gerrit/server/update/RetryableAction.java create mode 100644 java/com/google/gerrit/server/update/RetryableChangeAction.java diff --git a/java/com/google/gerrit/gpg/server/PostGpgKeys.java b/java/com/google/gerrit/gpg/server/PostGpgKeys.java index 5396e1c127..a36a923803 100644 --- a/java/com/google/gerrit/gpg/server/PostGpgKeys.java +++ b/java/com/google/gerrit/gpg/server/PostGpgKeys.java @@ -59,7 +59,6 @@ import com.google.gerrit.server.mail.send.AddKeySender; import com.google.gerrit.server.mail.send.DeleteKeySender; import com.google.gerrit.server.query.account.InternalAccountQuery; import com.google.gerrit.server.update.RetryHelper; -import com.google.gerrit.server.update.RetryHelper.ActionType; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -207,10 +206,10 @@ public class PostGpgKeys implements RestModifyView keyRings, Collection toRemove) throws RestApiException, PGPException, IOException { try { - retryHelper.execute( - ActionType.ACCOUNT_UPDATE, - () -> tryStoreKeys(rsrc, keyRings, toRemove), - LockFailureException.class::isInstance); + retryHelper + .accountUpdate("storeGpgKeys", () -> tryStoreKeys(rsrc, keyRings, toRemove)) + .retryOn(LockFailureException.class::isInstance) + .call(); } catch (Exception e) { Throwables.throwIfUnchecked(e); Throwables.throwIfInstanceOf(e, RestApiException.class); diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java index bd32c0b17b..b8ecb23438 100644 --- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java @@ -126,8 +126,9 @@ import com.google.gerrit.server.quota.QuotaException; import com.google.gerrit.server.restapi.change.ChangesCollection; import com.google.gerrit.server.restapi.project.ProjectsCollection; 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.RetryableAction; +import com.google.gerrit.server.update.RetryableAction.Action; +import com.google.gerrit.server.update.RetryableAction.ActionType; import com.google.gerrit.server.update.UpdateException; import com.google.gerrit.server.util.time.TimeUtil; import com.google.gerrit.util.http.CacheHeaders; @@ -814,27 +815,22 @@ public class RestApiServlet extends HttpServlet { ActionType actionType, Action action) throws Exception { + RetryableAction retryableAction = globals.retryHelper.action(actionType, caller, action); AtomicReference> traceId = new AtomicReference<>(Optional.empty()); - RetryHelper.Options.Builder retryOptionsBuilder = RetryHelper.options().caller(caller); if (!traceContext.isTracing()) { // enable automatic retry with tracing in case of non-recoverable failure - retryOptionsBuilder = - retryOptionsBuilder - .retryWithTrace(t -> !(t instanceof RestApiException)) - .onAutoTrace( - autoTraceId -> { - traceId.set(Optional.of(autoTraceId)); + retryableAction + .retryWithTrace(t -> !(t instanceof RestApiException)) + .onAutoTrace( + autoTraceId -> { + traceId.set(Optional.of(autoTraceId)); - // Include details of the request into the trace. - traceRequestData(req); - }); + // Include details of the request into the trace. + traceRequestData(req); + }); } try { - // ExceptionHookImpl controls on which exceptions we retry. - // The passed in exceptionPredicate allows to define additional exceptions on which retry - // should happen, but here we have none (hence pass in "t -> false" as exceptionPredicate). - return globals.retryHelper.execute( - actionType, action, retryOptionsBuilder.build(), t -> false); + return retryableAction.call(); } finally { // If auto-tracing got triggered due to a non-recoverable failure, also trace the rest of // this request. This means logging is forced for all further log statements and the logs are diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java index 1caee58937..272389f088 100644 --- a/java/com/google/gerrit/server/account/AccountsUpdate.java +++ b/java/com/google/gerrit/server/account/AccountsUpdate.java @@ -41,8 +41,7 @@ import com.google.gerrit.server.git.meta.MetaDataUpdate; import com.google.gerrit.server.index.change.ReindexAfterRefUpdate; import com.google.gerrit.server.notedb.Sequences; 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.RetryableAction.Action; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.AssistedInject; @@ -421,8 +420,10 @@ public class AccountsUpdate { private Optional executeAccountUpdate(Action> action) throws IOException, ConfigInvalidException { try { - return retryHelper.execute( - ActionType.ACCOUNT_UPDATE, action, LockFailureException.class::isInstance); + return retryHelper + .accountUpdate("updateAccount", action) + .retryOn(LockFailureException.class::isInstance) + .call(); } catch (Exception e) { Throwables.throwIfUnchecked(e); Throwables.throwIfInstanceOf(e, IOException.class); diff --git a/java/com/google/gerrit/server/account/Emails.java b/java/com/google/gerrit/server/account/Emails.java index 76c22cf75a..78cc59f38f 100644 --- a/java/com/google/gerrit/server/account/Emails.java +++ b/java/com/google/gerrit/server/account/Emails.java @@ -29,8 +29,7 @@ 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.RetryHelper.Action; -import com.google.gerrit.server.update.RetryHelper.ActionType; +import com.google.gerrit.server.update.RetryableAction.Action; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -85,7 +84,9 @@ public class Emails { return accounts; } - return executeIndexQuery(() -> queryProvider.get().byPreferredEmail(email).stream()) + return executeIndexQuery( + "queryAccountsByPreferredEmail", + () -> queryProvider.get().byPreferredEmail(email).stream()) .map(a -> a.account().id()) .collect(toImmutableSet()); } @@ -105,6 +106,7 @@ public class Emails { Arrays.stream(emails).filter(e -> !result.containsKey(e)).collect(toImmutableList()); if (!emailsToBackfill.isEmpty()) { executeIndexQuery( + "queryAccountsByPreferredEmails", () -> queryProvider.get().byPreferredEmail(emailsToBackfill).entries().stream()) .forEach(e -> result.put(e.getKey(), e.getValue().account().id())); } @@ -139,10 +141,12 @@ public class Emails { return u; } - private T executeIndexQuery(Action action) { + private T executeIndexQuery(String actionName, Action action) { try { - return retryHelper.execute( - ActionType.INDEX_QUERY, action, StorageException.class::isInstance); + return retryHelper + .indexQuery(actionName, action) + .retryOn(StorageException.class::isInstance) + .call(); } catch (Exception e) { Throwables.throwIfUnchecked(e); throw new StorageException(e); diff --git a/java/com/google/gerrit/server/change/ChangeCleanupRunner.java b/java/com/google/gerrit/server/change/ChangeCleanupRunner.java index 0299f1096e..6cf7a8f120 100644 --- a/java/com/google/gerrit/server/change/ChangeCleanupRunner.java +++ b/java/com/google/gerrit/server/change/ChangeCleanupRunner.java @@ -79,11 +79,14 @@ public class ChangeCleanupRunner implements Runnable { // abandonInactiveOpenChanges skips failures instead of throwing, so retrying will never // actually happen. For the purposes of this class that is fine: they'll get tried again the // next time the scheduled task is run. - retryHelper.execute( - updateFactory -> { - abandonUtil.abandonInactiveOpenChanges(updateFactory); - return null; - }); + retryHelper + .changeUpdate( + "abandonInactiveOpenChanges", + updateFactory -> { + abandonUtil.abandonInactiveOpenChanges(updateFactory); + return null; + }) + .call(); } catch (RestApiException | UpdateException e) { logger.atSevere().withCause(e).log("Failed to cleanup changes."); } diff --git a/java/com/google/gerrit/server/change/ConsistencyChecker.java b/java/com/google/gerrit/server/change/ConsistencyChecker.java index 513f5ca95c..0f03c8809f 100644 --- a/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -170,26 +170,29 @@ public class ConsistencyChecker { public Result check(ChangeNotes notes, @Nullable FixInput f) { requireNonNull(notes); try { - return retryHelper.execute( - buf -> { - try { - reset(); - this.updateFactory = buf; - this.notes = notes; - fix = f; - checkImpl(); - return result(); - } finally { - if (rw != null) { - rw.getObjectReader().close(); - rw.close(); - oi.close(); - } - if (repo != null) { - repo.close(); - } - } - }); + return retryHelper + .changeUpdate( + "checkChangeConsistency", + buf -> { + try { + reset(); + this.updateFactory = buf; + this.notes = notes; + fix = f; + checkImpl(); + return result(); + } finally { + if (rw != null) { + rw.getObjectReader().close(); + rw.close(); + oi.close(); + } + if (repo != null) { + repo.close(); + } + } + }) + .call(); } catch (RestApiException e) { return logAndReturnOneProblem(e, notes, "Error checking change: " + e.getMessage()); } catch (UpdateException e) { diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 5155925ce2..3791de8d08 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -167,8 +167,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.RetryableAction.Action; import com.google.gerrit.server.update.UpdateException; import com.google.gerrit.server.util.LabelVote; import com.google.gerrit.server.util.MagicBranch; @@ -3258,122 +3257,130 @@ class ReceiveCommits { // TODO(dborowitz): Combine this BatchUpdate with the main one in // handleRegularCommands try { - retryHelper.execute( - updateFactory -> { - try (BatchUpdate bu = - updateFactory.create(projectState.getNameKey(), user, TimeUtil.nowTs()); - ObjectInserter ins = repo.newObjectInserter(); - ObjectReader reader = ins.newReader(); - RevWalk rw = new RevWalk(reader)) { - bu.setRepository(repo, rw, ins); - // TODO(dborowitz): Teach BatchUpdate to ignore missing changes. + retryHelper + .changeUpdate( + "autoCloseChanges", + updateFactory -> { + try (BatchUpdate bu = + updateFactory.create(projectState.getNameKey(), user, TimeUtil.nowTs()); + ObjectInserter ins = repo.newObjectInserter(); + ObjectReader reader = ins.newReader(); + RevWalk rw = new RevWalk(reader)) { + bu.setRepository(repo, rw, ins); + // TODO(dborowitz): Teach BatchUpdate to ignore missing changes. - RevCommit newTip = rw.parseCommit(cmd.getNewId()); - BranchNameKey branch = BranchNameKey.create(project.getNameKey(), refName); + RevCommit newTip = rw.parseCommit(cmd.getNewId()); + BranchNameKey branch = BranchNameKey.create(project.getNameKey(), refName); - rw.reset(); - rw.sort(RevSort.REVERSE); - rw.markStart(newTip); - if (!ObjectId.zeroId().equals(cmd.getOldId())) { - rw.markUninteresting(rw.parseCommit(cmd.getOldId())); - } + rw.reset(); + rw.sort(RevSort.REVERSE); + rw.markStart(newTip); + if (!ObjectId.zeroId().equals(cmd.getOldId())) { + rw.markUninteresting(rw.parseCommit(cmd.getOldId())); + } - Map byKey = null; - List replaceAndClose = new ArrayList<>(); + Map byKey = null; + List replaceAndClose = new ArrayList<>(); - int existingPatchSets = 0; - int newPatchSets = 0; - SubmissionId submissionId = null; - COMMIT: - for (RevCommit c; (c = rw.next()) != null; ) { - rw.parseBody(c); + int existingPatchSets = 0; + int newPatchSets = 0; + SubmissionId submissionId = null; + COMMIT: + for (RevCommit c; (c = rw.next()) != null; ) { + rw.parseBody(c); - for (Ref ref : - receivePackRefCache.tipsFromObjectId(c.copy(), RefNames.REFS_CHANGES)) { - PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName()); - Optional notes = getChangeNotes(psId.changeId()); - if (notes.isPresent() && notes.get().getChange().getDest().equals(branch)) { - if (submissionId == null) { - submissionId = new SubmissionId(notes.get().getChange()); + for (Ref ref : + receivePackRefCache.tipsFromObjectId(c.copy(), RefNames.REFS_CHANGES)) { + PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName()); + Optional notes = getChangeNotes(psId.changeId()); + if (notes.isPresent() && notes.get().getChange().getDest().equals(branch)) { + if (submissionId == null) { + submissionId = new SubmissionId(notes.get().getChange()); + } + existingPatchSets++; + bu.addOp( + notes.get().getChangeId(), setPrivateOpFactory.create(false, null)); + bu.addOp( + psId.changeId(), + mergedByPushOpFactory.create( + requestScopePropagator, + psId, + submissionId, + refName, + newTip.getId().getName())); + continue COMMIT; + } } - existingPatchSets++; - bu.addOp(notes.get().getChangeId(), setPrivateOpFactory.create(false, null)); + + for (String changeId : c.getFooterLines(FooterConstants.CHANGE_ID)) { + if (byKey == null) { + byKey = + executeIndexQuery( + "queryOpenChangesByKeyByBranch", + () -> openChangesByKeyByBranch(branch)); + } + + ChangeNotes onto = byKey.get(Change.key(changeId.trim())); + if (onto != null) { + newPatchSets++; + // Hold onto this until we're done with the walk, as the call to + // req.validate below calls isMergedInto which resets the walk. + ReplaceRequest req = + new ReplaceRequest(onto.getChangeId(), c, cmd, false); + req.notes = onto; + replaceAndClose.add(req); + continue COMMIT; + } + } + } + + for (ReplaceRequest req : replaceAndClose) { + Change.Id id = req.notes.getChangeId(); + if (!req.validateNewPatchSetForAutoClose()) { + logger.atFine().log("Not closing %s because validation failed", id); + continue; + } + if (submissionId == null) { + submissionId = new SubmissionId(req.notes.getChange()); + } + req.addOps(bu, null); + bu.addOp(id, setPrivateOpFactory.create(false, null)); bu.addOp( - psId.changeId(), - mergedByPushOpFactory.create( - requestScopePropagator, - psId, - submissionId, - refName, - newTip.getId().getName())); - continue COMMIT; - } - } - - for (String changeId : c.getFooterLines(FooterConstants.CHANGE_ID)) { - if (byKey == null) { - byKey = executeIndexQuery(() -> openChangesByKeyByBranch(branch)); + id, + mergedByPushOpFactory + .create( + requestScopePropagator, + req.psId, + submissionId, + refName, + newTip.getId().getName()) + .setPatchSetProvider(req.replaceOp::getPatchSet)); + bu.addOp(id, new ChangeProgressOp(progress)); + ids.add(id); } - ChangeNotes onto = byKey.get(Change.key(changeId.trim())); - if (onto != null) { - newPatchSets++; - // Hold onto this until we're done with the walk, as the call to - // req.validate below calls isMergedInto which resets the walk. - ReplaceRequest req = new ReplaceRequest(onto.getChangeId(), c, cmd, false); - req.notes = onto; - replaceAndClose.add(req); - continue COMMIT; - } + logger.atFine().log( + "Auto-closing %d changes with existing patch sets and %d with new patch sets", + existingPatchSets, newPatchSets); + bu.execute(); + } catch (IOException | StorageException | PermissionBackendException e) { + logger.atSevere().withCause(e).log("Failed to auto-close changes"); + return null; } - } - for (ReplaceRequest req : replaceAndClose) { - Change.Id id = req.notes.getChangeId(); - if (!req.validateNewPatchSetForAutoClose()) { - logger.atFine().log("Not closing %s because validation failed", id); - continue; - } - if (submissionId == null) { - submissionId = new SubmissionId(req.notes.getChange()); - } - req.addOps(bu, null); - bu.addOp(id, setPrivateOpFactory.create(false, null)); - bu.addOp( - id, - mergedByPushOpFactory - .create( - requestScopePropagator, - req.psId, - submissionId, - refName, - newTip.getId().getName()) - .setPatchSetProvider(req.replaceOp::getPatchSet)); - bu.addOp(id, new ChangeProgressOp(progress)); - ids.add(id); - } + // If we are here, we didn't throw UpdateException. Record the result. + // The ordering is indeterminate due to the HashSet; unfortunately, Change.Id + // doesn't + // fit into TreeSet. + ids.stream() + .forEach(id -> resultChangeIds.add(ResultChangeIds.Key.AUTOCLOSED, id)); - logger.atFine().log( - "Auto-closing %d changes with existing patch sets and %d with new patch sets", - existingPatchSets, newPatchSets); - bu.execute(); - } catch (IOException | StorageException | PermissionBackendException e) { - logger.atSevere().withCause(e).log("Failed to auto-close changes"); - return null; - } - - // If we are here, we didn't throw UpdateException. Record the result. - // The ordering is indeterminate due to the HashSet; unfortunately, Change.Id doesn't - // fit into TreeSet. - ids.stream().forEach(id -> resultChangeIds.add(ResultChangeIds.Key.AUTOCLOSED, id)); - - return null; - }, + return null; + }) // Use a multiple of the default timeout to account for inner retries that may otherwise // eat up the whole timeout so that no time is left to retry this outer action. - RetryHelper.options() - .timeout(retryHelper.getDefaultTimeout(ActionType.CHANGE_UPDATE).multipliedBy(5)) - .build()); + .defaultTimeoutMultiplier(5) + .call(); } catch (RestApiException e) { logger.atSevere().withCause(e).log("Can't insert patchset"); } catch (UpdateException e) { @@ -3390,10 +3397,12 @@ class ReceiveCommits { } } - private T executeIndexQuery(Action action) { + private T executeIndexQuery(String actionName, Action action) { try (TraceTimer traceTimer = newTimer("executeIndexQuery")) { - return retryHelper.execute( - ActionType.INDEX_QUERY, action, StorageException.class::isInstance); + return retryHelper + .indexQuery(actionName, action) + .retryOn(StorageException.class::isInstance) + .call(); } catch (Exception e) { Throwables.throwIfUnchecked(e); throw new StorageException(e); diff --git a/java/com/google/gerrit/server/group/db/GroupsUpdate.java b/java/com/google/gerrit/server/group/db/GroupsUpdate.java index 7f1ba6a178..5069331635 100644 --- a/java/com/google/gerrit/server/group/db/GroupsUpdate.java +++ b/java/com/google/gerrit/server/group/db/GroupsUpdate.java @@ -309,10 +309,10 @@ public class GroupsUpdate { InternalGroupCreation groupCreation, InternalGroupUpdate groupUpdate) throws IOException, ConfigInvalidException, DuplicateKeyException { try { - return retryHelper.execute( - RetryHelper.ActionType.GROUP_UPDATE, - () -> createGroupInNoteDb(groupCreation, groupUpdate), - LockFailureException.class::isInstance); + return retryHelper + .groupUpdate("createGroup", () -> createGroupInNoteDb(groupCreation, groupUpdate)) + .retryOn(LockFailureException.class::isInstance) + .call(); } catch (Exception e) { Throwables.throwIfUnchecked(e); Throwables.throwIfInstanceOf(e, IOException.class); @@ -349,10 +349,10 @@ public class GroupsUpdate { AccountGroup.UUID groupUuid, InternalGroupUpdate groupUpdate) throws IOException, ConfigInvalidException, DuplicateKeyException, NoSuchGroupException { try { - return retryHelper.execute( - RetryHelper.ActionType.GROUP_UPDATE, - () -> updateGroupInNoteDb(groupUuid, groupUpdate), - LockFailureException.class::isInstance); + return retryHelper + .groupUpdate("updateGroup", () -> updateGroupInNoteDb(groupUuid, groupUpdate)) + .retryOn(LockFailureException.class::isInstance) + .call(); } catch (Exception e) { Throwables.throwIfUnchecked(e); Throwables.throwIfInstanceOf(e, IOException.class); diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java index 158db1c4a0..71d8c15592 100644 --- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java +++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java @@ -156,11 +156,14 @@ public class MailProcessor { * @param message {@link MailMessage} to process */ public void process(MailMessage message) throws RestApiException, UpdateException { - retryHelper.execute( - buf -> { - processImpl(buf, message); - return null; - }); + retryHelper + .changeUpdate( + "processCommentsReceivedByEmail", + buf -> { + processImpl(buf, message); + return null; + }) + .call(); } private void processImpl(BatchUpdate.Factory buf, MailMessage message) diff --git a/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java b/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java index f6aba34c46..a70f9021db 100644 --- a/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java +++ b/java/com/google/gerrit/server/project/ProjectsConsistencyChecker.java @@ -52,7 +52,6 @@ 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.gerrit.server.update.RetryHelper.ActionType; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -264,16 +263,16 @@ public class ProjectsConsistencyChecker { try { List queryResult = - retryHelper.execute( - ActionType.INDEX_QUERY, - () -> { - // Execute the query. - return changeQueryProvider - .get() - .setRequestedFields(ChangeField.CHANGE, ChangeField.PATCH_SET) - .query(and(basePredicate, or(predicates))); - }, - StorageException.class::isInstance); + retryHelper + .indexQuery( + "projectsConsistencyCheckerQueryChanges", + () -> + changeQueryProvider + .get() + .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. List autoCloseableChangesByBranch = new ArrayList<>(); @@ -282,32 +281,35 @@ public class ProjectsConsistencyChecker { // Skip changes that we have already processed, either by this query or by // earlier queries. if (seenChanges.add(autoCloseableChange.getId())) { - retryHelper.execute( - ActionType.CHANGE_UPDATE, - () -> { - // Auto-close by change - if (changeIdToMergedSha1.containsKey(autoCloseableChange.change().getKey())) { - autoCloseableChangesByBranch.add( - changeJson( - fix, changeIdToMergedSha1.get(autoCloseableChange.change().getKey())) - .format(autoCloseableChange)); - return null; - } + retryHelper + .changeUpdate( + "projectsConsistencyCheckerAutoCloseChanges", + () -> { + // Auto-close by change + if (changeIdToMergedSha1.containsKey(autoCloseableChange.change().getKey())) { + autoCloseableChangesByBranch.add( + changeJson( + fix, + changeIdToMergedSha1.get(autoCloseableChange.change().getKey())) + .format(autoCloseableChange)); + return null; + } - // Auto-close by commit - for (ObjectId patchSetSha1 : - autoCloseableChange.patchSets().stream() - .map(PatchSet::commitId) - .collect(toSet())) { - if (mergedSha1s.contains(patchSetSha1)) { - autoCloseableChangesByBranch.add( - changeJson(fix, patchSetSha1).format(autoCloseableChange)); - break; - } - } - return null; - }, - StorageException.class::isInstance); + // Auto-close by commit + for (ObjectId patchSetSha1 : + autoCloseableChange.patchSets().stream() + .map(PatchSet::commitId) + .collect(toSet())) { + if (mergedSha1s.contains(patchSetSha1)) { + autoCloseableChangesByBranch.add( + changeJson(fix, patchSetSha1).format(autoCloseableChange)); + break; + } + } + return null; + }) + .retryOn(StorageException.class::isInstance) + .call(); } } diff --git a/java/com/google/gerrit/server/restapi/project/CommitsCollection.java b/java/com/google/gerrit/server/restapi/project/CommitsCollection.java index 7c88ab37fb..213fcd56fd 100644 --- a/java/com/google/gerrit/server/restapi/project/CommitsCollection.java +++ b/java/com/google/gerrit/server/restapi/project/CommitsCollection.java @@ -38,8 +38,7 @@ 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.RetryHelper.Action; -import com.google.gerrit.server.update.RetryHelper.ActionType; +import com.google.gerrit.server.update.RetryableAction.Action; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -132,6 +131,7 @@ public class CommitsCollection implements ChildCollection changes = executeIndexQuery( + "queryChangesByProjectCommitWithLimit1", () -> queryProvider .get() @@ -151,7 +151,10 @@ public class CommitsCollection implements ChildCollection new CommitPredicate(parent.getId().getName())) .collect(toImmutableList()))); - changes = executeIndexQuery(() -> queryProvider.get().enforceVisibility(true).query(pred)); + changes = + executeIndexQuery( + "queryChangesByProjectCommit", + () -> queryProvider.get().enforceVisibility(true).query(pred)); Set branchesForCommitParents = new HashSet<>(changes.size()); for (ChangeData cd : changes) { @@ -175,10 +178,12 @@ public class CommitsCollection implements ChildCollection T executeIndexQuery(Action action) { + private T executeIndexQuery(String actionName, Action action) { try { - return retryHelper.execute( - ActionType.INDEX_QUERY, action, StorageException.class::isInstance); + return retryHelper + .indexQuery(actionName, action) + .retryOn(StorageException.class::isInstance) + .call(); } catch (Exception e) { Throwables.throwIfUnchecked(e); throw new StorageException(e); diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 8bdd98e2da..3af55e6fb2 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -77,7 +77,6 @@ import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; import com.google.gerrit.server.update.RetryHelper; -import com.google.gerrit.server.update.RetryHelper.ActionType; import com.google.gerrit.server.update.UpdateException; import com.google.gerrit.server.util.time.TimeUtil; import com.google.inject.Inject; @@ -483,41 +482,39 @@ public class MergeOp implements AutoCloseable { } RetryTracker retryTracker = new RetryTracker(); - retryHelper.execute( - updateFactory -> { - long attempt = retryTracker.lastAttemptNumber + 1; - boolean isRetry = attempt > 1; - if (isRetry) { - logger.atFine().log("Retrying, attempt #%d; skipping merged changes", attempt); - this.ts = TimeUtil.nowTs(); - openRepoManager(); - } - this.commitStatus = new CommitStatus(cs, isRetry); - if (checkSubmitRules) { - logger.atFine().log("Checking submit rules and state"); - checkSubmitRulesAndState(cs, isRetry); - } else { - logger.atFine().log("Bypassing submit rules"); - bypassSubmitRules(cs, isRetry); - } - try { - integrateIntoHistory(cs); - } catch (IntegrationException e) { - logger.atWarning().withCause(e).log("Error from integrateIntoHistory"); - throw new ResourceConflictException(e.getMessage(), e); - } - return null; - }, - RetryHelper.options() - .listener(retryTracker) - // Up to the entire submit operation is retried, including possibly many projects. - // Multiply the timeout by the number of projects we're actually attempting to - // submit. - .timeout( - retryHelper - .getDefaultTimeout(ActionType.CHANGE_UPDATE) - .multipliedBy(cs.projects().size())) - .build()); + retryHelper + .changeUpdate( + "integrateIntoHistory", + updateFactory -> { + long attempt = retryTracker.lastAttemptNumber + 1; + boolean isRetry = attempt > 1; + if (isRetry) { + logger.atFine().log("Retrying, attempt #%d; skipping merged changes", attempt); + this.ts = TimeUtil.nowTs(); + openRepoManager(); + } + this.commitStatus = new CommitStatus(cs, isRetry); + if (checkSubmitRules) { + logger.atFine().log("Checking submit rules and state"); + checkSubmitRulesAndState(cs, isRetry); + } else { + logger.atFine().log("Bypassing submit rules"); + bypassSubmitRules(cs, isRetry); + } + try { + integrateIntoHistory(cs); + } catch (IntegrationException e) { + logger.atWarning().withCause(e).log("Error from integrateIntoHistory"); + throw new ResourceConflictException(e.getMessage(), e); + } + return null; + }) + .listener(retryTracker) + // Up to the entire submit operation is retried, including possibly many projects. + // Multiply the timeout by the number of projects we're actually attempting to + // submit. + .defaultTimeoutMultiplier(cs.projects().size()) + .call(); if (projects > 1) { topicMetrics.topicSubmissionsCompleted.increment(); diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java index f3636c99e9..65fd97e819 100644 --- a/java/com/google/gerrit/server/update/RetryHelper.java +++ b/java/com/google/gerrit/server/update/RetryHelper.java @@ -28,12 +28,10 @@ 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.base.Throwables; import com.google.common.collect.Maps; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.Nullable; import com.google.gerrit.exceptions.StorageException; -import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.metrics.Counter3; import com.google.gerrit.metrics.Description; import com.google.gerrit.metrics.Field; @@ -44,6 +42,9 @@ 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.update.RetryableAction.Action; +import com.google.gerrit.server.update.RetryableAction.ActionType; +import com.google.gerrit.server.update.RetryableChangeAction.ChangeAction; import com.google.inject.Inject; import com.google.inject.Singleton; import java.time.Duration; @@ -59,26 +60,6 @@ import org.eclipse.jgit.lib.Config; public class RetryHelper { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - @FunctionalInterface - public interface ChangeAction { - T call(BatchUpdate.Factory batchUpdateFactory) throws Exception; - } - - @FunctionalInterface - public interface Action { - T call() throws Exception; - } - - public enum ActionType { - ACCOUNT_UPDATE, - CHANGE_UPDATE, - GROUP_UPDATE, - INDEX_QUERY, - PLUGIN_UPDATE, - REST_READ_REQUEST, - REST_WRITE_REQUEST, - } - /** * Options for retrying a single operation. * @@ -194,10 +175,6 @@ public class RetryHelper { return new AutoValue_RetryHelper_Options.Builder(); } - private static Options defaults() { - return options().build(); - } - private final Metrics metrics; private final BatchUpdate.Factory updateFactory; private final PluginSetContext exceptionHooks; @@ -253,48 +230,100 @@ public class RetryHelper { this.retryWithTraceOnFailure = cfg.getBoolean("retry", "retryWithTraceOnFailure", false); } - public Duration getDefaultTimeout(ActionType actionType) { + /** + * Creates an action that is executed with retrying when called. + * + * @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 + */ + public RetryableAction action(ActionType actionType, String actionName, Action action) { + return new RetryableAction<>(this, actionType, actionName, action); + } + + /** + * Creates an action for updating an account that is executed with retrying when called. + * + * @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 + */ + public RetryableAction accountUpdate(String actionName, Action action) { + return new RetryableAction<>(this, ActionType.ACCOUNT_UPDATE, actionName, action); + } + + /** + * Creates an action for updating a change that is executed with retrying when called. + * + * @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 + */ + public RetryableAction changeUpdate(String actionName, Action action) { + return new RetryableAction<>(this, ActionType.CHANGE_UPDATE, actionName, action); + } + + /** + * Creates an action for updating a change that is executed with retrying when called. + * + *

The change action gets a {@link BatchUpdate.Factory} provided that can be used to update the + * change. + * + * @param actionName the name of the action, used as metric bucket + * @param changeAction the action that should be executed + * @return the retryable action, callers need to call {@link RetryableChangeAction#call()} to + * execute the action + */ + public RetryableChangeAction changeUpdate( + String actionName, ChangeAction changeAction) { + return new RetryableChangeAction<>(this, updateFactory, actionName, changeAction); + } + + /** + * Creates an action for updating a group that is executed with retrying when called. + * + * @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 + */ + public RetryableAction groupUpdate(String actionName, Action action) { + return new RetryableAction<>(this, ActionType.GROUP_UPDATE, actionName, action); + } + + /** + * Creates an action for updating of plugin-specific data that is executed with retrying when + * called. + * + * @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 + */ + public RetryableAction pluginUpdate(String actionName, Action action) { + return new RetryableAction<>(this, ActionType.PLUGIN_UPDATE, actionName, action); + } + + /** + * Creates an action for querying an index that is executed with retrying when called. + * + * @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 + */ + public RetryableAction indexQuery(String actionName, Action action) { + return new RetryableAction<>(this, ActionType.INDEX_QUERY, actionName, action); + } + + Duration getDefaultTimeout(ActionType actionType) { return defaultTimeouts.get(actionType); } - public T execute( - ActionType actionType, Action action, Predicate exceptionPredicate) - throws Exception { - return execute(actionType, action, defaults(), exceptionPredicate); - } - - public T execute( - ActionType actionType, - Action action, - Options opts, - Predicate exceptionPredicate) - throws Exception { - try { - return executeWithAttemptAndTimeoutCount(actionType, action, opts, exceptionPredicate); - } catch (Throwable t) { - Throwables.throwIfUnchecked(t); - Throwables.throwIfInstanceOf(t, Exception.class); - throw new IllegalStateException(t); - } - } - - public T execute(ChangeAction changeAction) throws RestApiException, UpdateException { - return execute(changeAction, defaults()); - } - - public T execute(ChangeAction changeAction, Options opts) - throws RestApiException, UpdateException { - try { - return execute( - ActionType.CHANGE_UPDATE, () -> changeAction.call(updateFactory), opts, t -> false); - } catch (Throwable t) { - Throwables.throwIfUnchecked(t); - Throwables.throwIfInstanceOf(t, UpdateException.class); - Throwables.throwIfInstanceOf(t, RestApiException.class); - throw new UpdateException(t); - } - } - /** * Executes an action and records the number of attempts and the timeout as metrics. * @@ -306,7 +335,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 executeWithAttemptAndTimeoutCount( + T execute( ActionType actionType, Action action, Options opts, diff --git a/java/com/google/gerrit/server/update/RetryableAction.java b/java/com/google/gerrit/server/update/RetryableAction.java new file mode 100644 index 0000000000..3ee079c6ef --- /dev/null +++ b/java/com/google/gerrit/server/update/RetryableAction.java @@ -0,0 +1,168 @@ +// 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 static java.util.Objects.requireNonNull; + +import com.github.rholder.retry.RetryListener; +import com.google.common.base.Throwables; +import com.google.gerrit.server.ExceptionHook; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Consumer; +import java.util.function.Predicate; + +/** + * An action that is executed with retrying. + * + *

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

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)}. + */ +public class RetryableAction { + public enum ActionType { + ACCOUNT_UPDATE, + CHANGE_UPDATE, + GROUP_UPDATE, + INDEX_QUERY, + PLUGIN_UPDATE, + REST_READ_REQUEST, + REST_WRITE_REQUEST, + } + + @FunctionalInterface + public interface Action { + T call() throws Exception; + } + + private final RetryHelper retryHelper; + private final ActionType actionType; + private final Action action; + private final RetryHelper.Options.Builder options = RetryHelper.options(); + private final List> exceptionPredicates = new ArrayList<>(); + + RetryableAction( + RetryHelper retryHelper, ActionType actionType, String actionName, Action action) { + this.retryHelper = requireNonNull(retryHelper, "retryHelper"); + this.actionType = requireNonNull(actionType, "actionType"); + this.action = requireNonNull(action, "action"); + options.caller(requireNonNull(actionName, "actionName")); + } + + /** + * Adds an additional condition that should trigger retries. + * + *

For some exceptions retrying is enabled globally (see {@link + * ExceptionHook#shouldRetry(Throwable)}). Conditions for those exceptions do not need to be + * specified here again. + * + *

This method can be invoked multiple times to add further conditions that should trigger + * retries. + * + * @param exceptionPredicate predicate that decides if the action should be retried for a given + * exception + * @return this instance to enable chaining of calls + */ + public RetryableAction retryOn(Predicate exceptionPredicate) { + exceptionPredicates.add(exceptionPredicate); + return this; + } + + /** + * Sets a condition that should trigger auto-retry with tracing. + * + *

This condition is only relevant if an exception occurs that doesn't trigger (normal) retry. + * + *

Auto-retry with tracing automatically captures traces for unexpected exceptions so that they + * can be investigated. + * + *

Every call of this method overwrites any previously set condition for auto-retry with + * tracing. + * + * @param exceptionPredicate predicate that decides if the action should be retried with tracing + * for a given exception + * @return this instance to enable chaining of calls + */ + public RetryableAction retryWithTrace(Predicate exceptionPredicate) { + options.retryWithTrace(exceptionPredicate); + return this; + } + + /** + * Sets a callback that is invoked when auto-retry with tracing is triggered. + * + *

Via the callback callers can find out with trace ID was used for the retry. + * + *

Every call of this method overwrites any previously set trace ID consumer. + * + * @param traceIdConsumer trace ID consumer + * @return this instance to enable chaining of calls + */ + public RetryableAction onAutoTrace(Consumer traceIdConsumer) { + options.onAutoTrace(traceIdConsumer); + return this; + } + + /** + * Sets a listener that is invoked when the action is retried. + * + *

Every call of this method overwrites any previously set listener. + * + * @param retryListener retry listener + * @return this instance to enable chaining of calls + */ + public RetryableAction listener(RetryListener retryListener) { + options.listener(retryListener); + return this; + } + + /** + * Increases the default timeout by the given multiplier. + * + *

Every call of this method overwrites any previously set timeout. + * + * @param multiplier multiplier for the default timeout + * @return this instance to enable chaining of calls + */ + public RetryableAction defaultTimeoutMultiplier(int multiplier) { + options.timeout(retryHelper.getDefaultTimeout(actionType).multipliedBy(multiplier)); + return this; + } + + /** + * Executes this action with retry. + * + * @return the result of the action + */ + public T call() throws Exception { + try { + return retryHelper.execute( + actionType, + action, + options.build(), + t -> exceptionPredicates.stream().anyMatch(p -> p.test(t))); + } catch (Throwable t) { + Throwables.throwIfUnchecked(t); + Throwables.throwIfInstanceOf(t, Exception.class); + throw new IllegalStateException(t); + } + } +} diff --git a/java/com/google/gerrit/server/update/RetryableChangeAction.java b/java/com/google/gerrit/server/update/RetryableChangeAction.java new file mode 100644 index 0000000000..152db2c1bc --- /dev/null +++ b/java/com/google/gerrit/server/update/RetryableChangeAction.java @@ -0,0 +1,92 @@ +// 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.extensions.restapi.RestApiException; +import java.util.function.Consumer; +import java.util.function.Predicate; + +/** + * A change action that is executed with retrying. + * + *

Instances of this class are created via {@link RetryHelper#changeUpdate(String, + * ChangeAction)}. + * + *

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

In addition when a change action is called any exception that is not an unchecked exception + * and neither {@link UpdateException} nor {@link RestApiException} get wrapped into an {@link + * UpdateException}. + */ +public class RetryableChangeAction extends RetryableAction { + @FunctionalInterface + public interface ChangeAction { + T call(BatchUpdate.Factory batchUpdateFactory) throws Exception; + } + + RetryableChangeAction( + RetryHelper retryHelper, + BatchUpdate.Factory updateFactory, + String actionName, + ChangeAction changeAction) { + super( + retryHelper, ActionType.CHANGE_UPDATE, actionName, () -> changeAction.call(updateFactory)); + } + + @Override + public RetryableChangeAction retryOn(Predicate exceptionPredicate) { + super.retryOn(exceptionPredicate); + return this; + } + + @Override + public RetryableChangeAction retryWithTrace(Predicate exceptionPredicate) { + super.retryWithTrace(exceptionPredicate); + return this; + } + + @Override + public RetryableChangeAction onAutoTrace(Consumer traceIdConsumer) { + super.onAutoTrace(traceIdConsumer); + return this; + } + + @Override + public RetryableChangeAction listener(RetryListener retryListener) { + super.listener(retryListener); + return this; + } + + @Override + public RetryableChangeAction defaultTimeoutMultiplier(int multiplier) { + super.defaultTimeoutMultiplier(multiplier); + return this; + } + + @Override + public T call() throws UpdateException, RestApiException { + try { + return super.call(); + } catch (Throwable t) { + Throwables.throwIfUnchecked(t); + Throwables.throwIfInstanceOf(t, UpdateException.class); + Throwables.throwIfInstanceOf(t, RestApiException.class); + throw new UpdateException(t); + } + } +} diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java index c502c798ae..3c066a394e 100644 --- a/javatests/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java +++ b/javatests/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java @@ -154,16 +154,20 @@ public class NoteDbOnlyIT extends AbstractDaemonTest { AtomicInteger afterUpdateReposCalledCount = new AtomicInteger(); String result = - retryHelper.execute( - batchUpdateFactory -> { - try (BatchUpdate bu = newBatchUpdate(batchUpdateFactory)) { - bu.addOp( - id, - new UpdateRefAndAddMessageOp(updateRepoCalledCount, updateChangeCalledCount)); - bu.execute(new ConcurrentWritingListener(afterUpdateReposCalledCount)); - } - return "Done"; - }); + retryHelper + .changeUpdate( + "testUpdateRefAndAddMessageOp", + batchUpdateFactory -> { + try (BatchUpdate bu = newBatchUpdate(batchUpdateFactory)) { + bu.addOp( + id, + new UpdateRefAndAddMessageOp( + updateRepoCalledCount, updateChangeCalledCount)); + bu.execute(new ConcurrentWritingListener(afterUpdateReposCalledCount)); + } + return "Done"; + }) + .call(); assertThat(result).isEqualTo("Done"); assertThat(updateRepoCalledCount.get()).isEqualTo(2); diff --git a/plugins/reviewnotes b/plugins/reviewnotes index f1a36220e0..eac4cd97cb 160000 --- a/plugins/reviewnotes +++ b/plugins/reviewnotes @@ -1 +1 @@ -Subproject commit f1a36220e0ef31fb024de9ad589dfdfdf301c295 +Subproject commit eac4cd97cb5818ff471c64914fb4e342baf28c05