Rewrite MergeOp to use BatchUpdate
There are a number of major architectural changes here. At a high level, instances of SubmitStrategy no longer actually touch the repo or changes directly; the only public method is now: public void addOps(BatchUpdate bu, Set<CodeReviewCommit> toMerge) throws IntegrationException; Thus the main loop in MergeOp#integrateIntoHistory creates a submit strategy for each branch, adds its ops to a BatchUpdate, and then executes the update. Some of the validation steps (which are read-only) as well as submodule operations still happen outside the main BatchUpdate. The next major change is moving all of the post-submit-strategy validation and change updating logic out of MergeOp. This has been split across two other basic locations. The first location is SubmitStrategyOp, which is a common abstract base class of ops created by SubmitStrategy implementations. This contains the logic to update the branch and change status based on the actual meat of the submit strategy implementation. It handles things like updating refs, writing out ChangeMessages, firing hooks, etc. SubmitStrategy guarantees that there is exactly one SubmitStrategyOp per input CodeReviewCommit that gets merged. Implementations may create a smaller set of ops, for example a single FastForwardOp to merge the first few commits in a series. SubmitStrategy "fills in the gaps" and adds an ImplicitIntegrateOp to the update for each change that needs to be validated even if it was not set up initially by the SubmitStrategy. The second location for validation uses the newly added BatchUpdate.Listener interface. The SubmitStrategyListener implementation uses the problem map stored in the new CommitStatus class to abort the process after updating the repo or updating changes. It also does some checks that are more appropriate after running _all_ submit strategies in the batch, like checking the CommitMergeStatus bit on all the input CodeReviewCommits. One goal of this rewrite is to make fully implementing the notedb part of MergeOp more feasible, but apart from that, there are some behavioral and organizational differences resulting from this change that I believe are overall beneficial. First, the main feature of BatchUpdate is that it follows a well-defined order of operations: all ObjectInserters are flushed before any ref updates are written; all ref updates are written as a (per-repo) batch; and each change is updated in a transaction. Since the merge process does all of these things, it will receive these performance and consistency improvements as a result. Second, this change deletes several hundred lines from MergeOp, making it somewhat less of a kitchen sink. Instead most of the behavior is decomposed into batch update operations. There is some cognitive cost to this in terms of communicating between phases using shared state, but I hope that overall the decomposition is worth it. Change-Id: I1d9883a515729f9085709dd0ec85f9ba3c5a6152
This commit is contained in:
@@ -117,7 +117,6 @@ import com.google.gerrit.server.notedb.ChangeUpdate;
|
||||
import com.google.gerrit.server.notedb.NotesMigration;
|
||||
import com.google.gerrit.server.patch.PatchSetInfoFactory;
|
||||
import com.google.gerrit.server.project.ChangeControl;
|
||||
import com.google.gerrit.server.project.NoSuchChangeException;
|
||||
import com.google.gerrit.server.project.ProjectCache;
|
||||
import com.google.gerrit.server.project.ProjectControl;
|
||||
import com.google.gerrit.server.project.ProjectState;
|
||||
@@ -1828,7 +1827,7 @@ public class ReceiveCommits {
|
||||
}
|
||||
|
||||
private void submit(ChangeControl changeCtl, PatchSet ps)
|
||||
throws OrmException, ResourceConflictException {
|
||||
throws OrmException, RestApiException {
|
||||
Submit submit = submitProvider.get();
|
||||
RevisionResource rsrc = new RevisionResource(changes.parse(changeCtl), ps);
|
||||
try (MergeOp op = mergeOpProvider.get()) {
|
||||
@@ -2159,7 +2158,7 @@ public class ReceiveCommits {
|
||||
requestScopePropagator.wrap(new Callable<PatchSet.Id>() {
|
||||
@Override
|
||||
public PatchSet.Id call() throws OrmException, IOException,
|
||||
NoSuchChangeException, ResourceConflictException {
|
||||
RestApiException {
|
||||
try {
|
||||
if (magicBranch != null && magicBranch.edit) {
|
||||
return upsertEdit();
|
||||
@@ -2233,7 +2232,7 @@ public class ReceiveCommits {
|
||||
}
|
||||
|
||||
PatchSet.Id insertPatchSet(ReviewDb db) throws OrmException, IOException,
|
||||
ResourceConflictException {
|
||||
RestApiException {
|
||||
final Account.Id me = user.getAccountId();
|
||||
final List<FooterLine> footerLines = newCommit.getFooterLines();
|
||||
final MailRecipients recipients = new MailRecipients();
|
||||
|
Reference in New Issue
Block a user