diff --git a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java index ca97a1ac5f..2a4deafb80 100644 --- a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java +++ b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java @@ -16,9 +16,11 @@ package com.google.gerrit.server.notedb; import static com.google.common.base.MoreObjects.firstNonNull; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.server.logging.TraceContext.newTimer; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ListMultimap; import com.google.common.collect.MultimapBuilder; import com.google.gerrit.common.Nullable; @@ -34,6 +36,7 @@ import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.logging.Metadata; import com.google.gerrit.server.logging.TraceContext; +import com.google.gerrit.server.update.BatchUpdateListener; import com.google.gerrit.server.update.ChainedReceiveCommands; import com.google.inject.Inject; import com.google.inject.Provider; @@ -94,6 +97,7 @@ public class NoteDbUpdateManager implements AutoCloseable { private String refLogMessage; private PersonIdent refLogIdent; private PushCertificate pushCert; + private ImmutableList batchUpdateListeners; @Inject NoteDbUpdateManager( @@ -117,6 +121,7 @@ public class NoteDbUpdateManager implements AutoCloseable { robotCommentUpdates = MultimapBuilder.hashKeys().arrayListValues().build(); rewriters = MultimapBuilder.hashKeys().arrayListValues().build(); changesToDelete = new HashSet<>(); + batchUpdateListeners = ImmutableList.of(); } @Override @@ -172,6 +177,13 @@ public class NoteDbUpdateManager implements AutoCloseable { return this; } + public NoteDbUpdateManager setBatchUpdateListeners( + ImmutableList batchUpdateListeners) { + checkNotNull(batchUpdateListeners); + this.batchUpdateListeners = batchUpdateListeners; + return this; + } + private void initChangeRepo() throws IOException { if (changeRepo == null) { changeRepo = OpenRepo.open(repoManager, projectName); @@ -358,6 +370,9 @@ public class NoteDbUpdateManager implements AutoCloseable { bru.setAtomic(true); or.cmds.addTo(bru); bru.setAllowNonFastForwards(true); + for (BatchUpdateListener listener : batchUpdateListeners) { + bru = listener.beforeUpdateRefs(bru); + } if (!dryrun) { RefUpdateUtil.executeChecked(bru, or.rw); diff --git a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java index 70d1eaf28c..ec82e1aace 100644 --- a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java +++ b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java @@ -48,7 +48,6 @@ import com.google.gerrit.server.restapi.change.CommentJson; import com.google.gerrit.server.restapi.change.CommentJson.HumanCommentFormatter; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdate.Factory; -import com.google.gerrit.server.update.BatchUpdateListener; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; import com.google.gerrit.server.update.UpdateException; @@ -140,7 +139,7 @@ public class DeleteDraftComments // Currently there's no way to let some updates succeed even if others fail. Even if there 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(updates.values(), BatchUpdateListener.NONE, false); + BatchUpdate.execute(updates.values(), ImmutableList.of(), false); return Response.ok( ops.stream().map(Op::getResult).filter(Objects::nonNull).collect(toImmutableList())); diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 5ce7c0329e..c53402a879 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -24,6 +24,7 @@ import com.github.rholder.retry.Attempt; import com.github.rholder.retry.RetryListener; import com.google.auto.value.AutoValue; import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSetMultimap; @@ -97,6 +98,7 @@ import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.transport.ReceiveCommand; /** * Merges changes in submission order into a single branch. @@ -228,6 +230,7 @@ public class MergeOp implements AutoCloseable { private final SubmitStrategyFactory submitStrategyFactory; private final SubscriptionGraph.Factory subscriptionGraphFactory; private final SubmoduleCommits.Factory submoduleCommitsFactory; + private final SubmoduleOp.Factory submoduleOpFactory; private final Provider ormProvider; private final NotifyResolver notifyResolver; private final RetryHelper retryHelper; @@ -235,6 +238,8 @@ public class MergeOp implements AutoCloseable { // Changes that were updated by this MergeOp. private final Map updatedChanges; + // Refs that were updated by this MergeOp. + private final Map updatedRefs; private Timestamp ts; private SubmissionId submissionId; @@ -259,6 +264,7 @@ public class MergeOp implements AutoCloseable { SubmitStrategyFactory submitStrategyFactory, SubmoduleCommits.Factory submoduleCommitsFactory, SubscriptionGraph.Factory subscriptionGraphFactory, + SubmoduleOp.Factory submoduleOpFactory, Provider ormProvider, NotifyResolver notifyResolver, TopicMetrics topicMetrics, @@ -273,12 +279,14 @@ public class MergeOp implements AutoCloseable { this.submitStrategyFactory = submitStrategyFactory; this.submoduleCommitsFactory = submoduleCommitsFactory; this.subscriptionGraphFactory = subscriptionGraphFactory; + this.submoduleOpFactory = submoduleOpFactory; this.ormProvider = ormProvider; this.notifyResolver = notifyResolver; this.retryHelper = retryHelper; this.topicMetrics = topicMetrics; this.changeDataFactory = changeDataFactory; this.updatedChanges = new HashMap<>(); + this.updatedRefs = new HashMap<>(); } @Override @@ -520,6 +528,9 @@ public class MergeOp implements AutoCloseable { .defaultTimeoutMultiplier(cs.projects().size()) .call(); + SubmoduleOp submoduleOp = submoduleOpFactory.create(updatedRefs, orm); + submoduleOp.updateSuperProjects(dryrun); + if (projects > 1) { topicMetrics.topicSubmissionsCompleted.increment(); } @@ -619,13 +630,14 @@ public class MergeOp implements AutoCloseable { try { BatchUpdate.execute( batchUpdates, - new SubmitStrategyListener(submitInput, strategies, commitStatus), + ImmutableList.of(new SubmitStrategyListener(submitInput, strategies, commitStatus)), dryrun); } finally { // If the BatchUpdate fails it can be that merging some of the changes was actually - // successful. This is why we must to collect the updated changes also when an exception was - // thrown. + // successful. This is why we must to collect the updated changes and refs also when an + // exception was thrown. strategies.forEach(s -> updatedChanges.putAll(s.getUpdatedChanges())); + batchUpdates.forEach(bu -> updatedRefs.putAll(bu.getSuccessfullyUpdatedBranches(dryrun))); // Do not leave executed BatchUpdates in the OpenRepos if (!dryrun) { @@ -682,8 +694,6 @@ public class MergeOp implements AutoCloseable { Set allCommits = toSubmit.values().stream().map(BranchBatch::commits).flatMap(Set::stream).collect(toSet()); - GitlinkOp.Factory gitlinkOpFactory = new GitlinkOp.Factory(submoduleCommits, subscriptionGraph); - for (BranchNameKey branch : allBranches) { OpenRepo or = orm.getRepo(branch.project()); if (toSubmit.containsKey(branch)) { @@ -713,16 +723,9 @@ public class MergeOp implements AutoCloseable { dryrun); strategies.add(strategy); strategy.addOps(or.getUpdate(), commitsToSubmit); - if (submitting.submitType().equals(SubmitType.FAST_FORWARD_ONLY) - && subscriptionGraph.hasSubscription(branch)) { - or.getUpdate().addRepoOnlyOp(gitlinkOpFactory.create(branch)); - } - } else { - // no open change for this branch - // add submodule triggered op into BatchUpdate - or.getUpdate().addRepoOnlyOp(gitlinkOpFactory.create(branch)); } } + return strategies; } diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java index 64710c3ed0..69d76e23d0 100644 --- a/java/com/google/gerrit/server/submit/SubmoduleOp.java +++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.submit; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.Project; @@ -23,7 +24,6 @@ import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.submit.MergeOpRepoManager.OpenRepo; import com.google.gerrit.server.update.BatchUpdate; -import com.google.gerrit.server.update.BatchUpdateListener; import com.google.gerrit.server.update.UpdateException; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -103,7 +103,7 @@ public class SubmoduleOp { } } } - BatchUpdate.execute(orm.batchUpdates(superProjects), BatchUpdateListener.NONE, dryrun); + BatchUpdate.execute(orm.batchUpdates(superProjects), ImmutableList.of(), dryrun); } catch (UpdateException | IOException | NoSuchProjectException e) { throw new StorageException("Cannot update gitlinks", e); } diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java index eb4962dc0e..2b7dd5c2d9 100644 --- a/java/com/google/gerrit/server/update/BatchUpdate.java +++ b/java/com/google/gerrit/server/update/BatchUpdate.java @@ -124,9 +124,9 @@ public class BatchUpdate implements AutoCloseable { } public static void execute( - Collection updates, BatchUpdateListener listener, boolean dryrun) + Collection updates, ImmutableList listeners, boolean dryrun) throws UpdateException, RestApiException { - requireNonNull(listener); + requireNonNull(listeners); if (updates.isEmpty()) { return; } @@ -140,16 +140,16 @@ public class BatchUpdate implements AutoCloseable { for (BatchUpdate u : updates) { u.executeUpdateRepo(); } - listener.afterUpdateRepos(); + notifyAfterUpdateRepo(listeners); for (BatchUpdate u : updates) { - changesHandles.add(u.executeChangeOps(dryrun)); + changesHandles.add(u.executeChangeOps(listeners, dryrun)); } for (ChangesHandle h : changesHandles) { h.execute(); indexFutures.addAll(h.startIndexFutures()); } - listener.afterUpdateRefs(); - listener.afterUpdateChanges(); + notifyAfterUpdateRefs(listeners); + notifyAfterUpdateChanges(listeners); } finally { for (ChangesHandle h : changesHandles) { h.close(); @@ -161,10 +161,7 @@ public class BatchUpdate implements AutoCloseable { // 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.stream() - .filter(u -> u.batchRefUpdate != null) - .forEach( - u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null))); + updates.forEach(BatchUpdate::fireRefChangeEvent); if (!dryrun) { for (BatchUpdate u : updates) { @@ -176,6 +173,27 @@ public class BatchUpdate implements AutoCloseable { } } + private static void notifyAfterUpdateRepo(ImmutableList listeners) + throws Exception { + for (BatchUpdateListener listener : listeners) { + listener.afterUpdateRepos(); + } + } + + private static void notifyAfterUpdateRefs(ImmutableList listeners) + throws Exception { + for (BatchUpdateListener listener : listeners) { + listener.afterUpdateRefs(); + } + } + + private static void notifyAfterUpdateChanges(ImmutableList listeners) + throws Exception { + for (BatchUpdateListener listener : listeners) { + listener.afterUpdateChanges(); + } + } + private static void checkDifferentProject(Collection updates) { Multiset projectCounts = updates.stream().map(u -> u.project).collect(toImmutableMultiset()); @@ -386,11 +404,11 @@ public class BatchUpdate implements AutoCloseable { } public void execute(BatchUpdateListener listener) throws UpdateException, RestApiException { - execute(ImmutableList.of(this), listener, false); + execute(ImmutableList.of(this), ImmutableList.of(listener), false); } public void execute() throws UpdateException, RestApiException { - execute(BatchUpdateListener.NONE); + execute(ImmutableList.of(this), ImmutableList.of(), false); } public BatchUpdate setRepository(Repository repo, RevWalk revWalk, ObjectInserter inserter) { @@ -530,6 +548,12 @@ public class BatchUpdate implements AutoCloseable { } } + private void fireRefChangeEvent() { + if (batchRefUpdate != null) { + gitRefUpdated.fire(project, batchRefUpdate, getAccount().orElse(null)); + } + } + private class ChangesHandle implements AutoCloseable { private final NoteDbUpdateManager manager; private final boolean dryrun; @@ -580,7 +604,8 @@ public class BatchUpdate implements AutoCloseable { } } - private ChangesHandle executeChangeOps(boolean dryrun) throws Exception { + private ChangesHandle executeChangeOps( + ImmutableList batchUpdateListeners, boolean dryrun) throws Exception { logDebug("Executing change ops"); initRepository(); Repository repo = repoView.getRepository(); @@ -593,6 +618,7 @@ public class BatchUpdate implements AutoCloseable { new ChangesHandle( updateManagerFactory .create(project) + .setBatchUpdateListeners(batchUpdateListeners) .setChangeRepo( repo, repoView.getRevWalk(), repoView.getInserter(), repoView.getCommands()), dryrun); diff --git a/java/com/google/gerrit/server/update/BatchUpdateListener.java b/java/com/google/gerrit/server/update/BatchUpdateListener.java index 765bba1ca7..d286e84136 100644 --- a/java/com/google/gerrit/server/update/BatchUpdateListener.java +++ b/java/com/google/gerrit/server/update/BatchUpdateListener.java @@ -14,6 +14,8 @@ package com.google.gerrit.server.update; +import org.eclipse.jgit.lib.BatchRefUpdate; + /** * Interface for listening during batch update execution. * @@ -21,11 +23,25 @@ package com.google.gerrit.server.update; * after that phase has been completed for all updates. */ public interface BatchUpdateListener { - public static final BatchUpdateListener NONE = new BatchUpdateListener() {}; + BatchUpdateListener NONE = new BatchUpdateListener() {}; /** Called after updating all repositories and flushing objects but before updating any refs. */ default void afterUpdateRepos() throws Exception {} + /** + * Optional setup of the {@link BatchRefUpdate} that is going to be executed. + * + *

Called after {@link #afterUpdateRepos()}, before {@link #afterUpdateRefs()} and {@link + * #afterUpdateChanges()} + * + * @param bru a batch ref update, ready but not executed yet + * @return a new {@link BatchRefUpdate}. Implementations can decide to modify and return the + * incoming instance, but callers must not rely on that. + */ + default BatchRefUpdate beforeUpdateRefs(BatchRefUpdate bru) { + return bru; + } + /** Called after updating all refs. */ default void afterUpdateRefs() throws Exception {}