opendev/gerrit
Code Issues Proposed changes

Revert changes to MergeOp to batch object inserts and ref updates

Browse Source 
The failure modes for these are problematic, making the cure worse
than the disease. Unlike a missing PatchSet record in the database, a
missing PatchSet ref causes the change screen to fail to load, and
even once abandoned, causes reindexing to fail.

A longer-term rewrite of MergeOp begins with I0771f5e8, which is
designed to have less problematic failure modes, but until then, just
revert.

This mostly reverts the following commits (except for some cleanups
and new helper methods that are not used now but will be eventually):
  c20148c8e2
  5dea1c0fc0

Change-Id: Id691318a6e28f18860b34a9a39f6e834161ff0dd
This commit is contained in:
Dave Borowitz
2015-01-30 07:31:15 -08:00
parent 02fc027d12
commit 875fc8e8a3
12 changed files with 221 additions and 212 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
View File

@@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CommitMergeStatus;
import com.google.gerrit.server.git.MergeConflictException;
@@ -36,8 +37,8 @@ import com.google.gwtorm.server.OrmException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.ReceiveCommand;
import java.io.IOException;
import java.util.ArrayList;
@@ -49,13 +50,16 @@ import java.util.Map;
public class CherryPick extends SubmitStrategy {
private final PatchSetInfoFactory patchSetInfoFactory;
private final GitReferenceUpdated gitRefUpdated;
private final Map<Change.Id, CodeReviewCommit> newCommits;
CherryPick(SubmitStrategy.Arguments args,
PatchSetInfoFactory patchSetInfoFactory) {
PatchSetInfoFactory patchSetInfoFactory,
GitReferenceUpdated gitRefUpdated) {
super(args);
this.patchSetInfoFactory = patchSetInfoFactory;
this.gitRefUpdated = gitRefUpdated;
this.newCommits = new HashMap<>();
}
@@ -170,6 +174,8 @@ public class CherryPick extends SubmitStrategy {
ps.setUploader(cherryPickUser.getAccountId());
ps.setRevision(new RevId(newCommit.getId().getName()));
RefUpdate ru;
args.db.changes().beginTransaction(n.change().getId());
try {
insertAncestors(args.db, ps.getId(), newCommit);
@@ -185,14 +191,23 @@ public class CherryPick extends SubmitStrategy {
}
args.db.patchSetApprovals().insert(approvals);
args.batchRefUpdate.addCommand(
new ReceiveCommand(ObjectId.zeroId(), newCommit, ps.getRefName()));
ru = args.repo.updateRef(ps.getRefName());
ru.setExpectedOldObjectId(ObjectId.zeroId());
ru.setNewObjectId(newCommit);
ru.disableRefLog();
if (ru.update(args.rw) != RefUpdate.Result.NEW) {
throw new IOException(String.format(
"Failed to create ref %s in %s: %s", ps.getRefName(), n.change()
.getDest().getParentKey().get(), ru.getResult()));
}
args.db.commit();
} finally {
args.db.rollback();
}
gitRefUpdated.fire(n.change().getProject(), ru);
newCommit.copyFrom(n);
newCommit.setStatusCode(CommitMergeStatus.CLEAN_PICK);
newCommit.setControl(args.changeControlFactory.controlFor(n.change(), cherryPickUser));

10
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
View File

@@ -87,11 +87,11 @@ public class RebaseIfNecessary extends SubmitStrategy {
IdentifiedUser uploader =
args.identifiedUserFactory.create(args.mergeUtil
.getSubmitter(n).getAccountId());
PatchSet newPatchSet = rebaseChange.rebase(args.repo, args.rw,
args.inserter, n.getPatchsetId(), n.change(), uploader,
mergeTip.getCurrentTip(), args.mergeUtil,
args.serverIdent.get(), false, false, ValidatePolicy.NONE,
args.batchRefUpdate);
PatchSet newPatchSet =
rebaseChange.rebase(args.repo, args.rw, args.inserter,
n.getPatchsetId(), n.change(), uploader,
mergeTip.getCurrentTip(), args.mergeUtil,
args.serverIdent.get(), false, false, ValidatePolicy.NONE);
List<PatchSetApproval> approvals = Lists.newArrayList();
for (PatchSetApproval a : args.approvalsUtil.byPatchSet(args.db,

9
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
View File

@@ -30,7 +30,6 @@ import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.project.ChangeControl;
import com.google.inject.Provider;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.RefUpdate.Result;
@@ -61,7 +60,6 @@ public abstract class SubmitStrategy {
protected final RevWalk rw;
protected final ObjectInserter inserter;
protected final RevFlag canMergeFlag;
protected final BatchRefUpdate batchRefUpdate;
protected final Set<RevCommit> alreadyAccepted;
protected final Branch.NameKey destBranch;
protected final ApprovalsUtil approvalsUtil;
@@ -73,9 +71,9 @@ public abstract class SubmitStrategy {
Provider<PersonIdent> serverIdent, ReviewDb db,
ChangeControl.GenericFactory changeControlFactory, Repository repo,
RevWalk rw, ObjectInserter inserter, RevFlag canMergeFlag,
BatchRefUpdate batchRefUpdate, Set<RevCommit> alreadyAccepted,
Branch.NameKey destBranch, ApprovalsUtil approvalsUtil,
MergeUtil mergeUtil, ChangeIndexer indexer) {
Set<RevCommit> alreadyAccepted, Branch.NameKey destBranch,
ApprovalsUtil approvalsUtil, MergeUtil mergeUtil,
ChangeIndexer indexer) {
this.identifiedUserFactory = identifiedUserFactory;
this.serverIdent = serverIdent;
this.db = db;
@@ -84,7 +82,6 @@ public abstract class SubmitStrategy {
this.repo = repo;
this.rw = rw;
this.inserter = inserter;
this.batchRefUpdate = batchRefUpdate;
this.canMergeFlag = canMergeFlag;
this.alreadyAccepted = alreadyAccepted;
this.destBranch = destBranch;

25
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java
View File

@@ -21,6 +21,7 @@ import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.changedetail.RebaseChange;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.MergeException;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.index.ChangeIndexer;
@@ -33,7 +34,6 @@ import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
@@ -55,6 +55,7 @@ public class SubmitStrategyFactory {
private final Provider<PersonIdent> myIdent;
private final ChangeControl.GenericFactory changeControlFactory;
private final PatchSetInfoFactory patchSetInfoFactory;
private final GitReferenceUpdated gitRefUpdated;
private final RebaseChange rebaseChange;
private final ProjectCache projectCache;
private final ApprovalsUtil approvalsUtil;
@@ -67,7 +68,7 @@ public class SubmitStrategyFactory {
@GerritPersonIdent Provider<PersonIdent> myIdent,
final ChangeControl.GenericFactory changeControlFactory,
final PatchSetInfoFactory patchSetInfoFactory,
final RebaseChange rebaseChange,
final GitReferenceUpdated gitRefUpdated, final RebaseChange rebaseChange,
final ProjectCache projectCache,
final ApprovalsUtil approvalsUtil,
final MergeUtil.Factory mergeUtilFactory,
@@ -76,6 +77,7 @@ public class SubmitStrategyFactory {
this.myIdent = myIdent;
this.changeControlFactory = changeControlFactory;
this.patchSetInfoFactory = patchSetInfoFactory;
this.gitRefUpdated = gitRefUpdated;
this.rebaseChange = rebaseChange;
this.projectCache = projectCache;
this.approvalsUtil = approvalsUtil;
@@ -83,19 +85,20 @@ public class SubmitStrategyFactory {
this.indexer = indexer;
}
public SubmitStrategy create(SubmitType submitType, ReviewDb db,
Repository repo, RevWalk rw, ObjectInserter inserter,
RevFlag canMergeFlag, BatchRefUpdate batchRefUpdated,
Set<RevCommit> alreadyAccepted, Branch.NameKey destBranch)
public SubmitStrategy create(final SubmitType submitType, final ReviewDb db,
final Repository repo, final RevWalk rw, final ObjectInserter inserter,
final RevFlag canMergeFlag, final Set<RevCommit> alreadyAccepted,
final Branch.NameKey destBranch)
throws MergeException, NoSuchProjectException {
ProjectState project = getProject(destBranch);
SubmitStrategy.Arguments args = new SubmitStrategy.Arguments(
identifiedUserFactory, myIdent, db, changeControlFactory, repo, rw,
inserter, canMergeFlag, batchRefUpdated, alreadyAccepted, destBranch,
approvalsUtil, mergeUtilFactory.create(project), indexer);
final SubmitStrategy.Arguments args =
new SubmitStrategy.Arguments(identifiedUserFactory, myIdent, db,
changeControlFactory, repo, rw, inserter, canMergeFlag,
alreadyAccepted, destBranch,approvalsUtil,
mergeUtilFactory.create(project), indexer);
switch (submitType) {
case CHERRY_PICK:
return new CherryPick(args, patchSetInfoFactory);
return new CherryPick(args, patchSetInfoFactory, gitRefUpdated);
case FAST_FORWARD_ONLY:
return new FastForwardOnly(args);
case MERGE_ALWAYS: