From 4da95c042e4c37e4809fb32ed0d94aebdf80455e Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Sun, 27 Sep 2020 18:01:09 -0700 Subject: [PATCH 1/4] MergeOp: Use submoduleOp.updateSuperprojects() instead of direct ops We want to make updateSuperprojects an "after submission" action. ReceiveCommits do so using submoduleOp.updateSuperprojects(), it would simplify code to do the same for MergeOp. Invoke submoduleOp.updateSuperprojects() with the updated branches after the submission (the updated branches are accumulated between retries). Change-Id: I5959aa3537221dca57cfb35e475bd22217a120be --- .../google/gerrit/server/submit/MergeOp.java | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 5ce7c0329e..b126e91a2a 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java @@ -97,6 +97,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 +229,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 +237,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 +263,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 +278,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 +527,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(); } @@ -623,9 +633,10 @@ public class MergeOp implements AutoCloseable { 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 +693,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 +722,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; } From 52ad85c26bdd0303782b98d6e94e04b5e477b5a8 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Thu, 9 Jul 2020 15:36:29 -0700 Subject: [PATCH 2/4] BatchUpdate: Move firing ref change event to BatchUpdate proper The static method BatchUpdate#execute() is accessing instance variables to fire ref update events. Actually all the information to emit those events is in the instance. Make the ref update event emission a method in the BatchUpdate instance. Change-Id: Ie7321d807313fa19a23e5d23e7129b4294db0dc5 --- java/com/google/gerrit/server/update/BatchUpdate.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java index eb4962dc0e..0a5efbf2ac 100644 --- a/java/com/google/gerrit/server/update/BatchUpdate.java +++ b/java/com/google/gerrit/server/update/BatchUpdate.java @@ -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) { @@ -530,6 +527,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; From 688d51ac97f4812ee89381162ccbf6d0af1b5842 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Thu, 24 Sep 2020 10:36:31 -0700 Subject: [PATCH 3/4] BatchUpdateListener: Offer BatchRefUpdate to the listener before exec To implement a reliable submission tracking, we need to add more information to the BatchRefUpdate. Offer to the BatchUpdateListeners the BatchRefUpdate before executing it. For this, pass the listener all the way down to NoteDbUpdateManager where the BRU is created. Change-Id: I2674ac6814d3d0d9c6b9cc95370de00eb31c6f6a --- .../server/notedb/NoteDbUpdateManager.java | 11 +++++++++++ .../gerrit/server/update/BatchUpdate.java | 6 ++++-- .../server/update/BatchUpdateListener.java | 18 +++++++++++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java index ca97a1ac5f..cd8a18ff69 100644 --- a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java +++ b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java @@ -16,6 +16,7 @@ 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; @@ -34,6 +35,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 +96,7 @@ public class NoteDbUpdateManager implements AutoCloseable { private String refLogMessage; private PersonIdent refLogIdent; private PushCertificate pushCert; + private BatchUpdateListener batchUpdateListener; @Inject NoteDbUpdateManager( @@ -117,6 +120,7 @@ public class NoteDbUpdateManager implements AutoCloseable { robotCommentUpdates = MultimapBuilder.hashKeys().arrayListValues().build(); rewriters = MultimapBuilder.hashKeys().arrayListValues().build(); changesToDelete = new HashSet<>(); + batchUpdateListener = BatchUpdateListener.NONE; } @Override @@ -172,6 +176,12 @@ public class NoteDbUpdateManager implements AutoCloseable { return this; } + public NoteDbUpdateManager setBatchUpdateListener(BatchUpdateListener batchUpdateListener) { + checkNotNull(batchUpdateListener); + this.batchUpdateListener = batchUpdateListener; + return this; + } + private void initChangeRepo() throws IOException { if (changeRepo == null) { changeRepo = OpenRepo.open(repoManager, projectName); @@ -358,6 +368,7 @@ public class NoteDbUpdateManager implements AutoCloseable { bru.setAtomic(true); or.cmds.addTo(bru); bru.setAllowNonFastForwards(true); + bru = this.batchUpdateListener.beforeUpdateRefs(bru); if (!dryrun) { RefUpdateUtil.executeChecked(bru, or.rw); diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java index 0a5efbf2ac..4cd3f7c2e3 100644 --- a/java/com/google/gerrit/server/update/BatchUpdate.java +++ b/java/com/google/gerrit/server/update/BatchUpdate.java @@ -142,7 +142,7 @@ public class BatchUpdate implements AutoCloseable { } listener.afterUpdateRepos(); for (BatchUpdate u : updates) { - changesHandles.add(u.executeChangeOps(dryrun)); + changesHandles.add(u.executeChangeOps(listener, dryrun)); } for (ChangesHandle h : changesHandles) { h.execute(); @@ -583,7 +583,8 @@ public class BatchUpdate implements AutoCloseable { } } - private ChangesHandle executeChangeOps(boolean dryrun) throws Exception { + private ChangesHandle executeChangeOps(BatchUpdateListener batchUpdateListener, boolean dryrun) + throws Exception { logDebug("Executing change ops"); initRepository(); Repository repo = repoView.getRepository(); @@ -596,6 +597,7 @@ public class BatchUpdate implements AutoCloseable { new ChangesHandle( updateManagerFactory .create(project) + .setBatchUpdateListener(batchUpdateListener) .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 {} From 23d5aced94449a9e59c5b539724dde5c5d81092a Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Tue, 29 Sep 2020 16:36:47 -0700 Subject: [PATCH 4/4] BatchUpdate.execute: Accept a list of listeners Later, a submission could need to add extra listeners to the BatchUpdate. Support that directly in BatchUpdate. Change-Id: I4d03b91451aeae0d3fd6e20b7bc99a6f39b1f580 --- .../server/notedb/NoteDbUpdateManager.java | 16 ++++--- .../restapi/account/DeleteDraftComments.java | 3 +- .../google/gerrit/server/submit/MergeOp.java | 3 +- .../gerrit/server/submit/SubmoduleOp.java | 4 +- .../gerrit/server/update/BatchUpdate.java | 43 ++++++++++++++----- 5 files changed, 47 insertions(+), 22 deletions(-) diff --git a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java index cd8a18ff69..2a4deafb80 100644 --- a/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java +++ b/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java @@ -20,6 +20,7 @@ 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; @@ -96,7 +97,7 @@ public class NoteDbUpdateManager implements AutoCloseable { private String refLogMessage; private PersonIdent refLogIdent; private PushCertificate pushCert; - private BatchUpdateListener batchUpdateListener; + private ImmutableList batchUpdateListeners; @Inject NoteDbUpdateManager( @@ -120,7 +121,7 @@ public class NoteDbUpdateManager implements AutoCloseable { robotCommentUpdates = MultimapBuilder.hashKeys().arrayListValues().build(); rewriters = MultimapBuilder.hashKeys().arrayListValues().build(); changesToDelete = new HashSet<>(); - batchUpdateListener = BatchUpdateListener.NONE; + batchUpdateListeners = ImmutableList.of(); } @Override @@ -176,9 +177,10 @@ public class NoteDbUpdateManager implements AutoCloseable { return this; } - public NoteDbUpdateManager setBatchUpdateListener(BatchUpdateListener batchUpdateListener) { - checkNotNull(batchUpdateListener); - this.batchUpdateListener = batchUpdateListener; + public NoteDbUpdateManager setBatchUpdateListeners( + ImmutableList batchUpdateListeners) { + checkNotNull(batchUpdateListeners); + this.batchUpdateListeners = batchUpdateListeners; return this; } @@ -368,7 +370,9 @@ public class NoteDbUpdateManager implements AutoCloseable { bru.setAtomic(true); or.cmds.addTo(bru); bru.setAllowNonFastForwards(true); - bru = this.batchUpdateListener.beforeUpdateRefs(bru); + 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 b126e91a2a..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; @@ -629,7 +630,7 @@ 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 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 4cd3f7c2e3..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(listener, 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(); @@ -173,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()); @@ -383,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) { @@ -583,8 +604,8 @@ public class BatchUpdate implements AutoCloseable { } } - private ChangesHandle executeChangeOps(BatchUpdateListener batchUpdateListener, boolean dryrun) - throws Exception { + private ChangesHandle executeChangeOps( + ImmutableList batchUpdateListeners, boolean dryrun) throws Exception { logDebug("Executing change ops"); initRepository(); Repository repo = repoView.getRepository(); @@ -597,7 +618,7 @@ public class BatchUpdate implements AutoCloseable { new ChangesHandle( updateManagerFactory .create(project) - .setBatchUpdateListener(batchUpdateListener) + .setBatchUpdateListeners(batchUpdateListeners) .setChangeRepo( repo, repoView.getRevWalk(), repoView.getInserter(), repoView.getCommands()), dryrun);