Merge changes Ic7f65188,Ib24563d8,Ic4fdfc9e,Icf7afdf3
* changes:
Add test to show how to retry BatchUpdate with RetryHelper on LockFailure
Test LockFailure during BatchUpdate
BatchUpdates: Inject ChangeData.Factory
Move static BatchUpdate#execute method into a new BatchUpdates class
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 7caa988..4399ad4 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -184,6 +184,7 @@
import com.google.gerrit.server.submit.MergeOpRepoManager;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.BatchUpdates;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.PostUpdateContext;
import com.google.gerrit.server.update.RepoContext;
@@ -367,9 +368,9 @@
private final AccountResolver accountResolver;
private final AllProjectsName allProjectsName;
private final BatchUpdate.Factory batchUpdateFactory;
+ private final BatchUpdates batchUpdates;
private final CancellationMetrics cancellationMetrics;
private final ChangeEditUtil editUtil;
- private final ChangeData.Factory changeDataFactory;
private final ChangeIndexer indexer;
private final ChangeInserter.Factory changeInserterFactory;
private final ChangeNotes.Factory notesFactory;
@@ -456,11 +457,11 @@
AccountResolver accountResolver,
AllProjectsName allProjectsName,
BatchUpdate.Factory batchUpdateFactory,
+ BatchUpdates batchUpdates,
CancellationMetrics cancellationMetrics,
ProjectConfig.Factory projectConfigFactory,
@GerritServerConfig Config config,
ChangeEditUtil editUtil,
- ChangeData.Factory changeDataFactory,
ChangeIndexer indexer,
ChangeInserter.Factory changeInserterFactory,
ChangeNotes.Factory notesFactory,
@@ -514,6 +515,7 @@
this.accountResolver = accountResolver;
this.allProjectsName = allProjectsName;
this.batchUpdateFactory = batchUpdateFactory;
+ this.batchUpdates = batchUpdates;
this.cancellationMetrics = cancellationMetrics;
this.changeFormatter = changeFormatterProvider.get();
this.changeUtil = changeUtil;
@@ -527,7 +529,6 @@
this.deadlineCheckerFactory = deadlineCheckerFactory;
this.diffOperationsForCommitValidationFactory = diffOperationsForCommitValidationFactory;
this.editUtil = editUtil;
- this.changeDataFactory = changeDataFactory;
this.hashtagsFactory = hashtagsFactory;
this.setTopicFactory = setTopicFactory;
this.indexer = indexer;
@@ -887,7 +888,7 @@
logger.atFine().log("Added %d additional ref updates", added);
SubmissionExecutor submissionExecutor =
- new SubmissionExecutor(changeDataFactory, false, superprojectUpdateSubmissionListeners);
+ new SubmissionExecutor(batchUpdates, false, superprojectUpdateSubmissionListeners);
submissionExecutor.execute(ImmutableList.of(bu));
diff --git a/java/com/google/gerrit/server/restapi/account/DeleteDraftCommentsUtil.java b/java/com/google/gerrit/server/restapi/account/DeleteDraftCommentsUtil.java
index 6fe464d..79038af 100644
--- a/java/com/google/gerrit/server/restapi/account/DeleteDraftCommentsUtil.java
+++ b/java/com/google/gerrit/server/restapi/account/DeleteDraftCommentsUtil.java
@@ -46,6 +46,7 @@
import com.google.gerrit.server.restapi.change.CommentJson;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.BatchUpdates;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.update.context.RefUpdateContext;
@@ -64,6 +65,7 @@
@Singleton
public class DeleteDraftCommentsUtil {
private final BatchUpdate.Factory batchUpdateFactory;
+ private final BatchUpdates batchUpdates;
private final Supplier<ChangeQueryBuilder> queryBuilderSupplier;
private final Provider<InternalChangeQuery> queryProvider;
private final ChangeData.Factory changeDataFactory;
@@ -77,6 +79,7 @@
@Inject
public DeleteDraftCommentsUtil(
BatchUpdate.Factory batchUpdateFactory,
+ BatchUpdates batchUpdates,
Provider<ChangeQueryBuilder> queryBuilderProvider,
Provider<InternalChangeQuery> queryProvider,
ChangeData.Factory changeDataFactory,
@@ -86,6 +89,7 @@
DraftCommentsReader draftCommentsReader,
PatchSetUtil psUtil) {
this.batchUpdateFactory = batchUpdateFactory;
+ this.batchUpdates = batchUpdates;
this.queryBuilderSupplier = Suppliers.memoize(queryBuilderProvider::get);
this.queryProvider = queryProvider;
this.changeDataFactory = changeDataFactory;
@@ -122,7 +126,7 @@
// were,
// all updates from this operation only happen in All-Users and thus are fully atomic, so
// allowing partial failure would have little value.
- BatchUpdate.execute(changeDataFactory, updates.values(), ImmutableList.of(), false);
+ batchUpdates.execute(updates.values(), ImmutableList.of(), false);
}
return ops.stream().map(Op::getResult).filter(Objects::nonNull).collect(toImmutableList());
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 709d6a7..e8c7049 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -94,6 +94,7 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.BatchUpdates;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.update.context.RefUpdateContext;
import com.google.gerrit.server.util.time.TimeUtil;
@@ -290,7 +291,7 @@
}
output.labels = input.labels;
- BatchUpdate.Result batchUpdateResult;
+ BatchUpdates.Result batchUpdateResult;
// Notify based on ReviewInput, ignoring the notify settings from any ReviewerInputs.
NotifyResolver.Result notify = notifyResolver.resolve(input.notify, input.notifyDetails);
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index bb1f25f..3fa4157 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -94,6 +94,7 @@
import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.BatchUpdates;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.SubmissionExecutor;
@@ -277,6 +278,7 @@
private final ChangeMessagesUtil cmUtil;
private final BatchUpdate.Factory batchUpdateFactory;
+ private final BatchUpdates batchUpdates;
private final InternalUser.Factory internalUserFactory;
private final MergeSuperSet mergeSuperSet;
private final MergeValidators.Factory mergeValidatorsFactory;
@@ -316,6 +318,7 @@
MergeOp(
ChangeMessagesUtil cmUtil,
BatchUpdate.Factory batchUpdateFactory,
+ BatchUpdates batchUpdates,
InternalUser.Factory internalUserFactory,
MergeSuperSet mergeSuperSet,
MergeValidators.Factory mergeValidatorsFactory,
@@ -337,6 +340,7 @@
@GerritServerConfig Config config) {
this.cmUtil = cmUtil;
this.batchUpdateFactory = batchUpdateFactory;
+ this.batchUpdates = batchUpdates;
this.internalUserFactory = internalUserFactory;
this.mergeSuperSet = mergeSuperSet;
this.mergeValidatorsFactory = mergeValidatorsFactory;
@@ -558,8 +562,7 @@
}
SubmissionExecutor submissionExecutor =
- new SubmissionExecutor(
- changeDataFactory, dryrun, superprojectUpdateSubmissionListeners);
+ new SubmissionExecutor(batchUpdates, dryrun, superprojectUpdateSubmissionListeners);
RetryTracker retryTracker = new RetryTracker();
@SuppressWarnings("unused")
var unused =
diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java
index 02afedb..2402357 100644
--- a/java/com/google/gerrit/server/submit/SubmoduleOp.java
+++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java
@@ -24,9 +24,8 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo;
-import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.BatchUpdates;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.update.context.RefUpdateContext;
import com.google.inject.Inject;
@@ -40,16 +39,16 @@
@Singleton
public static class Factory {
- private final ChangeData.Factory changeDataFactory;
+ private final BatchUpdates batchUpdates;
private final SubscriptionGraph.Factory subscriptionGraphFactory;
private final SubmoduleCommits.Factory submoduleCommitsFactory;
@Inject
Factory(
- ChangeData.Factory changeDataFactory,
+ BatchUpdates batchUpdates,
SubscriptionGraph.Factory subscriptionGraphFactory,
SubmoduleCommits.Factory submoduleCommitsFactory) {
- this.changeDataFactory = changeDataFactory;
+ this.batchUpdates = batchUpdates;
this.subscriptionGraphFactory = subscriptionGraphFactory;
this.submoduleCommitsFactory = submoduleCommitsFactory;
}
@@ -58,7 +57,7 @@
Map<BranchNameKey, ReceiveCommand> updatedBranches, MergeOpRepoManager orm)
throws SubmoduleConflictException {
return new SubmoduleOp(
- changeDataFactory,
+ batchUpdates,
updatedBranches,
orm,
subscriptionGraphFactory.compute(updatedBranches.keySet(), orm),
@@ -66,7 +65,7 @@
}
}
- private final ChangeData.Factory changeDataFactory;
+ private final BatchUpdates batchUpdates;
private final Map<BranchNameKey, ReceiveCommand> updatedBranches;
private final MergeOpRepoManager orm;
private final SubscriptionGraph subscriptionGraph;
@@ -74,12 +73,12 @@
private final UpdateOrderCalculator updateOrderCalculator;
private SubmoduleOp(
- ChangeData.Factory changeDataFactory,
+ BatchUpdates batchUpdates,
Map<BranchNameKey, ReceiveCommand> updatedBranches,
MergeOpRepoManager orm,
SubscriptionGraph subscriptionGraph,
SubmoduleCommits submoduleCommits) {
- this.changeDataFactory = changeDataFactory;
+ this.batchUpdates = batchUpdates;
this.updatedBranches = updatedBranches;
this.orm = orm;
this.subscriptionGraph = subscriptionGraph;
@@ -115,8 +114,7 @@
}
}
try (RefUpdateContext ctx = RefUpdateContext.open(UPDATE_SUPERPROJECT)) {
- BatchUpdate.execute(
- changeDataFactory,
+ batchUpdates.execute(
orm.batchUpdates(superProjects, /* refLogMessage= */ "merged"),
ImmutableList.of(),
dryrun);
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 6b5c5bc..813246c 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -17,7 +17,6 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.ImmutableMultiset.toImmutableMultiset;
import static com.google.common.flogger.LazyArgs.lazy;
import static com.google.gerrit.common.UsedAt.Project.GOOGLE;
import static java.util.Comparator.comparing;
@@ -35,7 +34,6 @@
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder;
-import com.google.common.collect.Multiset;
import com.google.common.flogger.FluentLogger;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
@@ -51,9 +49,6 @@
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.config.FactoryModule;
-import com.google.gerrit.extensions.restapi.BadRequestException;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.CurrentUser;
@@ -73,12 +68,7 @@
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.notedb.LimitExceededException;
import com.google.gerrit.server.notedb.NoteDbUpdateManager;
-import com.google.gerrit.server.project.InvalidChangeOperationException;
-import com.google.gerrit.server.project.NoSuchChangeException;
-import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.project.NoSuchRefException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -91,10 +81,8 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.Objects;
import java.util.Optional;
import java.util.TreeMap;
-import java.util.function.Function;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectInserter;
@@ -141,151 +129,6 @@
BatchUpdate create(Project.NameKey project, CurrentUser user, Instant when);
}
- public static class Result {
- private final ChangeData.Factory changeDataFactory;
- private final Map<Change.Id, ChangeData> changeDatas;
-
- private Result(ChangeData.Factory changeDataFactory) {
- this(changeDataFactory, new HashMap<>());
- }
-
- private Result(ChangeData.Factory changeDataFactory, Map<Change.Id, ChangeData> changeDatas) {
- this.changeDataFactory = changeDataFactory;
- this.changeDatas = changeDatas;
- }
-
- /**
- * Returns the updated {@link ChangeData} for the given project and change ID.
- *
- * <p>If the requested {@link ChangeData} was already loaded after the {@link BatchUpdate} has
- * been executed the cached {@link ChangeData} instance is returned, otherwise the requested
- * {@link ChangeData} is loaded and put into the cache.
- */
- public ChangeData getChangeData(Project.NameKey projectName, Change.Id changeId) {
- return changeDatas.computeIfAbsent(
- changeId, id -> changeDataFactory.create(projectName, changeId));
- }
- }
-
- @CanIgnoreReturnValue
- public static Result execute(
- ChangeData.Factory changeDataFactory,
- Collection<BatchUpdate> updates,
- ImmutableList<BatchUpdateListener> listeners,
- boolean dryrun)
- throws UpdateException, RestApiException {
- requireNonNull(listeners);
- if (updates.isEmpty()) {
- return new Result(changeDataFactory);
- }
-
- checkDifferentProject(updates);
-
- try {
- List<ListenableFuture<ChangeData>> indexFutures = new ArrayList<>();
- List<ChangesHandle> changesHandles = new ArrayList<>(updates.size());
- try {
- for (BatchUpdate u : updates) {
- u.executeUpdateRepo();
- }
- notifyAfterUpdateRepo(listeners);
- for (BatchUpdate u : updates) {
- changesHandles.add(u.executeChangeOps(listeners, dryrun));
- }
- for (ChangesHandle h : changesHandles) {
- h.execute();
- if (h.requiresReindex()) {
- indexFutures.addAll(h.startIndexFutures());
- }
- }
- notifyAfterUpdateRefs(listeners);
- notifyAfterUpdateChanges(listeners);
- } finally {
- for (ChangesHandle h : changesHandles) {
- h.close();
- }
- }
-
- Map<Change.Id, ChangeData> changeDatas =
- Futures.allAsList(indexFutures).get().stream()
- // filter out null values that were returned for change deletions
- .filter(Objects::nonNull)
- .collect(toMap(cd -> cd.change().getId(), Function.identity()));
-
- // Fire ref update events only after all mutations are finished, since callers may assume a
- // patch set ref being created means the change was created, or a branch advancing meaning
- // some changes were closed.
- updates.forEach(BatchUpdate::fireRefChangeEvents);
-
- if (!dryrun) {
- for (BatchUpdate u : updates) {
- u.executePostOps(changeDatas);
- }
- }
-
- return new Result(changeDataFactory, changeDatas);
- } catch (Exception e) {
- wrapAndThrowException(e);
- return new Result(changeDataFactory);
- }
- }
-
- private static void notifyAfterUpdateRepo(ImmutableList<BatchUpdateListener> listeners)
- throws Exception {
- for (BatchUpdateListener listener : listeners) {
- listener.afterUpdateRepos();
- }
- }
-
- private static void notifyAfterUpdateRefs(ImmutableList<BatchUpdateListener> listeners)
- throws Exception {
- for (BatchUpdateListener listener : listeners) {
- listener.afterUpdateRefs();
- }
- }
-
- private static void notifyAfterUpdateChanges(ImmutableList<BatchUpdateListener> listeners)
- throws Exception {
- for (BatchUpdateListener listener : listeners) {
- listener.afterUpdateChanges();
- }
- }
-
- private static void checkDifferentProject(Collection<BatchUpdate> updates) {
- Multiset<Project.NameKey> projectCounts =
- updates.stream().map(u -> u.project).collect(toImmutableMultiset());
- checkArgument(
- projectCounts.entrySet().size() == updates.size(),
- "updates must all be for different projects, got: %s",
- projectCounts);
- }
-
- private static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException {
- // Convert common non-REST exception types with user-visible messages to corresponding REST
- // exception types.
- if (e instanceof InvalidChangeOperationException || e instanceof LimitExceededException) {
- throw new ResourceConflictException(e.getMessage(), e);
- } else if (e instanceof NoSuchChangeException
- || e instanceof NoSuchRefException
- || e instanceof NoSuchProjectException) {
- throw new ResourceNotFoundException(e.getMessage(), e);
- } else if (e instanceof CommentsRejectedException) {
- // SC_BAD_REQUEST is not ideal because it's not a syntactic error, but there is no better
- // status code and it's isolated in monitoring.
- throw new BadRequestException(e.getMessage(), e);
- }
-
- Throwables.throwIfUnchecked(e);
-
- // Propagate REST API exceptions thrown by operations; they commonly throw exceptions like
- // ResourceConflictException to indicate an atomic update failure.
- Throwables.throwIfInstanceOf(e, UpdateException.class);
- Throwables.throwIfInstanceOf(e, RestApiException.class);
-
- // Otherwise, wrap in a generic UpdateException, which does not include a user-visible message.
- throw new UpdateException(e);
- }
-
class ContextImpl implements Context {
private final CurrentUser contextUser;
@@ -441,6 +284,7 @@
DELETED
}
+ private final BatchUpdates batchUpdates;
private final GitRepositoryManager repoManager;
private final AccountCache accountCache;
private final ChangeData.Factory changeDataFactory;
@@ -477,6 +321,7 @@
@Inject
BatchUpdate(
+ BatchUpdates batchUpdates,
GitRepositoryManager repoManager,
@GerritPersonIdent PersonIdent serverIdent,
AccountCache accountCache,
@@ -493,6 +338,7 @@
@Assisted CurrentUser user,
@Assisted Instant when) {
this.gerritConfig = gerritConfig;
+ this.batchUpdates = batchUpdates;
this.repoManager = repoManager;
this.accountCache = accountCache;
this.changeDataFactory = changeDataFactory;
@@ -517,13 +363,14 @@
}
@CanIgnoreReturnValue
- public Result execute(BatchUpdateListener listener) throws UpdateException, RestApiException {
- return execute(changeDataFactory, ImmutableList.of(this), ImmutableList.of(listener), false);
+ public BatchUpdates.Result execute(BatchUpdateListener listener)
+ throws UpdateException, RestApiException {
+ return batchUpdates.execute(ImmutableList.of(this), ImmutableList.of(listener), false);
}
@CanIgnoreReturnValue
- public Result execute() throws UpdateException, RestApiException {
- return execute(changeDataFactory, ImmutableList.of(this), ImmutableList.of(), false);
+ public BatchUpdates.Result execute() throws UpdateException, RestApiException {
+ return batchUpdates.execute(ImmutableList.of(this), ImmutableList.of(), false);
}
public boolean isExecuted() {
@@ -676,7 +523,7 @@
return this;
}
- private void executeUpdateRepo() throws UpdateException, RestApiException {
+ void executeUpdateRepo() throws UpdateException, RestApiException {
try {
logDebug("Executing updateRepo on %d ops", ops.size());
for (Map.Entry<Change.Id, OpData<BatchUpdateOp>> e : ops.entries()) {
@@ -718,7 +565,7 @@
&& gerritConfig.getBoolean("index", "indexChangesAsync", false);
}
- private void fireRefChangeEvents() {
+ void fireRefChangeEvents() {
batchRefUpdate.forEach(
(projectName, bru) -> gitRefUpdated.fire(projectName, bru, getAccount().orElse(null)));
}
@@ -735,7 +582,7 @@
}
}
- private class ChangesHandle implements AutoCloseable {
+ class ChangesHandle implements AutoCloseable {
private final NoteDbUpdateManager manager;
private final boolean dryrun;
private final Map<Change.Id, ChangeResult> results;
@@ -820,7 +667,7 @@
}
}
- private ChangesHandle executeChangeOps(
+ ChangesHandle executeChangeOps(
ImmutableList<BatchUpdateListener> batchUpdateListeners, boolean dryrun) throws Exception {
logDebug("Executing change ops");
initRepository();
@@ -950,7 +797,7 @@
return new ChangeContextImpl(contextUser, notes);
}
- private void executePostOps(Map<Change.Id, ChangeData> changeDatas) throws Exception {
+ void executePostOps(Map<Change.Id, ChangeData> changeDatas) throws Exception {
for (OpData<BatchUpdateOp> opData : ops.values()) {
PostUpdateContextImpl ctx = new PostUpdateContextImpl(opData.user(), changeDatas);
try (TraceContext.TraceTimer ignored =
diff --git a/java/com/google/gerrit/server/update/BatchUpdates.java b/java/com/google/gerrit/server/update/BatchUpdates.java
new file mode 100644
index 0000000..2f9ef84
--- /dev/null
+++ b/java/com/google/gerrit/server/update/BatchUpdates.java
@@ -0,0 +1,199 @@
+// Copyright (C) 2024 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 com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.collect.ImmutableMultiset.toImmutableMultiset;
+import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toMap;
+
+import com.google.common.base.Throwables;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Multiset;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.notedb.LimitExceededException;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.NoSuchRefException;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.update.BatchUpdate.ChangesHandle;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.Function;
+
+@Singleton
+public class BatchUpdates {
+ public class Result {
+ private final Map<Change.Id, ChangeData> changeDatas;
+
+ private Result() {
+ this(new HashMap<>());
+ }
+
+ private Result(Map<Change.Id, ChangeData> changeDatas) {
+ this.changeDatas = changeDatas;
+ }
+
+ /**
+ * Returns the updated {@link ChangeData} for the given project and change ID.
+ *
+ * <p>If the requested {@link ChangeData} was already loaded after the {@link BatchUpdate} has
+ * been executed the cached {@link ChangeData} instance is returned, otherwise the requested
+ * {@link ChangeData} is loaded and put into the cache.
+ */
+ public ChangeData getChangeData(Project.NameKey projectName, Change.Id changeId) {
+ return changeDatas.computeIfAbsent(
+ changeId, id -> changeDataFactory.create(projectName, changeId));
+ }
+ }
+
+ private final ChangeData.Factory changeDataFactory;
+
+ @Inject
+ BatchUpdates(ChangeData.Factory changeDataFactory) {
+ this.changeDataFactory = changeDataFactory;
+ }
+
+ @CanIgnoreReturnValue
+ public Result execute(
+ Collection<BatchUpdate> updates, ImmutableList<BatchUpdateListener> listeners, boolean dryrun)
+ throws UpdateException, RestApiException {
+ requireNonNull(listeners);
+ if (updates.isEmpty()) {
+ return new Result();
+ }
+
+ checkDifferentProject(updates);
+
+ try {
+ List<ListenableFuture<ChangeData>> indexFutures = new ArrayList<>();
+ List<ChangesHandle> changesHandles = new ArrayList<>(updates.size());
+ try {
+ for (BatchUpdate u : updates) {
+ u.executeUpdateRepo();
+ }
+ notifyAfterUpdateRepo(listeners);
+ for (BatchUpdate u : updates) {
+ changesHandles.add(u.executeChangeOps(listeners, dryrun));
+ }
+ for (ChangesHandle h : changesHandles) {
+ h.execute();
+ if (h.requiresReindex()) {
+ indexFutures.addAll(h.startIndexFutures());
+ }
+ }
+ notifyAfterUpdateRefs(listeners);
+ notifyAfterUpdateChanges(listeners);
+ } finally {
+ for (ChangesHandle h : changesHandles) {
+ h.close();
+ }
+ }
+
+ Map<Change.Id, ChangeData> changeDatas =
+ Futures.allAsList(indexFutures).get().stream()
+ // filter out null values that were returned for change deletions
+ .filter(Objects::nonNull)
+ .collect(toMap(cd -> cd.change().getId(), Function.identity()));
+
+ // Fire ref update events only after all mutations are finished, since callers may assume a
+ // patch set ref being created means the change was created, or a branch advancing meaning
+ // some changes were closed.
+ updates.forEach(BatchUpdate::fireRefChangeEvents);
+
+ if (!dryrun) {
+ for (BatchUpdate u : updates) {
+ u.executePostOps(changeDatas);
+ }
+ }
+
+ return new Result(changeDatas);
+ } catch (Exception e) {
+ wrapAndThrowException(e);
+ return new Result();
+ }
+ }
+
+ private static void notifyAfterUpdateRepo(ImmutableList<BatchUpdateListener> listeners)
+ throws Exception {
+ for (BatchUpdateListener listener : listeners) {
+ listener.afterUpdateRepos();
+ }
+ }
+
+ private static void notifyAfterUpdateRefs(ImmutableList<BatchUpdateListener> listeners)
+ throws Exception {
+ for (BatchUpdateListener listener : listeners) {
+ listener.afterUpdateRefs();
+ }
+ }
+
+ private static void notifyAfterUpdateChanges(ImmutableList<BatchUpdateListener> listeners)
+ throws Exception {
+ for (BatchUpdateListener listener : listeners) {
+ listener.afterUpdateChanges();
+ }
+ }
+
+ private static void checkDifferentProject(Collection<BatchUpdate> updates) {
+ Multiset<Project.NameKey> projectCounts =
+ updates.stream().map(u -> u.getProject()).collect(toImmutableMultiset());
+ checkArgument(
+ projectCounts.entrySet().size() == updates.size(),
+ "updates must all be for different projects, got: %s",
+ projectCounts);
+ }
+
+ private static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException {
+ // Convert common non-REST exception types with user-visible messages to corresponding REST
+ // exception types.
+ if (e instanceof InvalidChangeOperationException || e instanceof LimitExceededException) {
+ throw new ResourceConflictException(e.getMessage(), e);
+ } else if (e instanceof NoSuchChangeException
+ || e instanceof NoSuchRefException
+ || e instanceof NoSuchProjectException) {
+ throw new ResourceNotFoundException(e.getMessage(), e);
+ } else if (e instanceof CommentsRejectedException) {
+ // SC_BAD_REQUEST is not ideal because it's not a syntactic error, but there is no better
+ // status code and it's isolated in monitoring.
+ throw new BadRequestException(e.getMessage(), e);
+ }
+
+ Throwables.throwIfUnchecked(e);
+
+ // Propagate REST API exceptions thrown by operations; they commonly throw exceptions like
+ // ResourceConflictException to indicate an atomic update failure.
+ Throwables.throwIfInstanceOf(e, UpdateException.class);
+ Throwables.throwIfInstanceOf(e, RestApiException.class);
+
+ // Otherwise, wrap in a generic UpdateException, which does not include a user-visible message.
+ throw new UpdateException(e);
+ }
+}
diff --git a/java/com/google/gerrit/server/update/SubmissionExecutor.java b/java/com/google/gerrit/server/update/SubmissionExecutor.java
index bab6ddd..762de57 100644
--- a/java/com/google/gerrit/server/update/SubmissionExecutor.java
+++ b/java/com/google/gerrit/server/update/SubmissionExecutor.java
@@ -16,23 +16,22 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.submit.MergeOpRepoManager;
import java.util.Collection;
import java.util.Optional;
import java.util.stream.Collectors;
public class SubmissionExecutor {
- private final ChangeData.Factory changeDataFactory;
+ private final BatchUpdates batchUpdates;
private final ImmutableList<SubmissionListener> submissionListeners;
private final boolean dryrun;
private ImmutableList<BatchUpdateListener> additionalListeners = ImmutableList.of();
public SubmissionExecutor(
- ChangeData.Factory changeDataFactory,
+ BatchUpdates batchUpdates,
boolean dryrun,
ImmutableList<SubmissionListener> submissionListeners) {
- this.changeDataFactory = changeDataFactory;
+ this.batchUpdates = batchUpdates;
this.dryrun = dryrun;
this.submissionListeners = submissionListeners;
if (dryrun) {
@@ -63,7 +62,7 @@
.map(Optional::get)
.collect(Collectors.toList()))
.build();
- BatchUpdate.execute(changeDataFactory, updates, listeners, dryrun);
+ batchUpdates.execute(updates, listeners, dryrun);
}
/**
diff --git a/javatests/com/google/gerrit/server/update/BUILD b/javatests/com/google/gerrit/server/update/BUILD
index 345681d..a602ee9 100644
--- a/javatests/com/google/gerrit/server/update/BUILD
+++ b/javatests/com/google/gerrit/server/update/BUILD
@@ -12,11 +12,13 @@
"//java/com/google/gerrit/common:annotations",
"//java/com/google/gerrit/entities",
"//java/com/google/gerrit/extensions:api",
+ "//java/com/google/gerrit/git",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/util/time",
"//java/com/google/gerrit/testing:gerrit-test-util",
"//java/com/google/gerrit/testing:test-ref-update-context",
"//lib:guava",
+ "//lib:guava-retrying",
"//lib:jgit",
"//lib:jgit-junit",
"//lib/guice",
diff --git a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
index f7a2afa..cc49eb7 100644
--- a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
+++ b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
@@ -37,7 +37,10 @@
import com.google.gerrit.extensions.events.AttentionSetListener;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.git.LockFailureException;
+import com.google.gerrit.git.RefUpdateUtil;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.Sequences;
@@ -61,12 +64,16 @@
import com.google.inject.name.Named;
import java.util.ArrayList;
import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
@@ -107,6 +114,8 @@
@Inject private IdentifiedUser.GenericFactory userFactory;
@Inject private InternalUser.Factory internalUserFactory;
@Inject private AbandonOp.Factory abandonOpFactory;
+ @Inject @GerritPersonIdent private PersonIdent serverIdent;
+ @Inject private RetryHelper retryHelper;
@Rule public final MockitoRule mockito = MockitoJUnit.rule();
@@ -566,6 +575,131 @@
assertThat(metaCommit.getParent(0)).isEqualTo(oldHead);
}
+ @Test
+ public void lockFailureOnConcurrentUpdate() throws Exception {
+ Change.Id changeId = createChange();
+ ObjectId metaId = getMetaId(changeId);
+
+ ChangeNotes notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.NEW);
+
+ AtomicBoolean doneBackgroundUpdate = new AtomicBoolean(false);
+
+ // Create a listener that updates the change meta ref concurrently on the first attempt.
+ BatchUpdateListener listener =
+ new BatchUpdateListener() {
+ @Override
+ public BatchRefUpdate beforeUpdateRefs(BatchRefUpdate bru) {
+ try (RevWalk rw = new RevWalk(repo.getRepository())) {
+ RevCommit old = rw.parseCommit(metaId);
+ RevCommit commit =
+ repo.commit()
+ .parent(old)
+ .author(serverIdent)
+ .committer(serverIdent)
+ .setTopLevelTree(old.getTree())
+ .message("Concurrent Update\n\nPatch-Set: 1")
+ .create();
+ RefUpdate ru = repo.getRepository().updateRef(RefNames.changeMetaRef(changeId));
+ ru.setExpectedOldObjectId(metaId);
+ ru.setNewObjectId(commit);
+ ru.update();
+ RefUpdateUtil.checkResult(ru);
+ doneBackgroundUpdate.set(true);
+ } catch (Exception e) {
+ // Ignore. If an exception happens doneBackgroundUpdate is false and we fail later
+ // when doneBackgroundUpdate is checked.
+ }
+ return bru;
+ }
+ };
+
+ // Do a batch update, expect that it fails with LOCK_FAILURE due to the concurrent update.
+ assertThat(doneBackgroundUpdate.get()).isFalse();
+ UpdateException exception =
+ assertThrows(
+ UpdateException.class,
+ () -> {
+ try (BatchUpdate bu =
+ batchUpdateFactory.create(project, user.get(), TimeUtil.now())) {
+ bu.addOp(changeId, abandonOpFactory.create(null, "abandon"));
+ bu.execute(listener);
+ }
+ });
+ assertThat(exception).hasCauseThat().isInstanceOf(LockFailureException.class);
+ assertThat(doneBackgroundUpdate.get()).isTrue();
+
+ // Check that the change was not updated.
+ notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.NEW);
+ }
+
+ @Test
+ public void useRetryHelperToRetryOnLockFailure() throws Exception {
+ Change.Id changeId = createChange();
+ ObjectId metaId = getMetaId(changeId);
+
+ ChangeNotes notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.NEW);
+
+ AtomicBoolean doneBackgroundUpdate = new AtomicBoolean(false);
+
+ // Create a listener that updates the change meta ref concurrently on the first attempt.
+ BatchUpdateListener listener =
+ new BatchUpdateListener() {
+ @Override
+ public BatchRefUpdate beforeUpdateRefs(BatchRefUpdate bru) {
+ if (!doneBackgroundUpdate.getAndSet(true)) {
+ try (RevWalk rw = new RevWalk(repo.getRepository())) {
+ RevCommit old = rw.parseCommit(metaId);
+ RevCommit commit =
+ repo.commit()
+ .parent(old)
+ .author(serverIdent)
+ .committer(serverIdent)
+ .setTopLevelTree(old.getTree())
+ .message("Concurrent Update\n\nPatch-Set: 1")
+ .create();
+ RefUpdate ru = repo.getRepository().updateRef(RefNames.changeMetaRef(changeId));
+ ru.setExpectedOldObjectId(metaId);
+ ru.setNewObjectId(commit);
+ ru.update();
+ RefUpdateUtil.checkResult(ru);
+ } catch (Exception e) {
+ // Ignore. If an exception happens doneBackgroundUpdate is false and we fail later
+ // when doneBackgroundUpdate is checked.
+ }
+ }
+ return bru;
+ }
+ };
+
+ // Do a batch update, expect that it succeeds due to retrying despite the LOCK_FAILURE on the
+ // first attempt.
+ assertThat(doneBackgroundUpdate.get()).isFalse();
+
+ @SuppressWarnings("unused")
+ var unused =
+ retryHelper
+ .changeUpdate(
+ "batchUpdate",
+ updateFactory -> {
+ try (BatchUpdate bu = updateFactory.create(project, user.get(), TimeUtil.now())) {
+ bu.addOp(changeId, abandonOpFactory.create(null, "abandon"));
+ bu.execute(listener);
+ }
+ return null;
+ })
+ .call();
+
+ // Check that the concurrent update was done.
+ assertThat(doneBackgroundUpdate.get()).isTrue();
+
+ // Check that the BatchUpdate updated the change.
+ notes = changeNotesFactory.create(project, changeId);
+ assertThat(notes.getChange().getStatus()).isEqualTo(Change.Status.ABANDONED);
+ }
+
private Change.Id createChange() throws Exception {
Change.Id id = Change.id(sequences.nextChangeId());
try (BatchUpdate bu = batchUpdateFactory.create(project, user.get(), TimeUtil.now())) {