opendev
/
gerrit
Code Issues Proposed changes

MergeOp: Pre-open all OpenRepos 

The openRepo method may throw NoSuchProjectException, which should
fail the merge process and abandon all open changes. Better to do this
in advance rather than lazily after some side effects may have already
happened.

Change-Id: Iba219f83de65f19a329088ff60366e34eebe569e
This commit is contained in:
Dave Borowitz 2016-01-12 15:28:50 -05:00 committed by David Pursehouse
parent 82253c6e51
commit c5b1712ae8
5 changed files with 49 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
View File

@ -186,8 +186,6 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
ReviewDb db = dbProvider.get();
op.merge(db, change, caller, true);
change = db.changes().get(change.getId());
} catch (NoSuchChangeException e) {
throw new OrmException("Submission failed", e);
}
if (change == null) {

79
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@ -326,22 +326,27 @@ public class MergeOp implements AutoCloseable {
commits = new CommitStatus();
}
private OpenRepo openRepo(Project.NameKey project)
private OpenRepo getRepo(Project.NameKey project) {
OpenRepo or = openRepos.get(project);
checkState(or != null, "repo not yet opened: %s", project);
return or;
}
private void openRepo(Project.NameKey project)
throws NoSuchProjectException, IOException {
OpenRepo repo = openRepos.get(project);
if (repo == null) {
ProjectState projectState = projectCache.get(project);
if (projectState == null) {
throw new NoSuchProjectException(project);
}
try {
repo = new OpenRepo(repoManager.openRepository(project), projectState);
} catch (RepositoryNotFoundException e) {
throw new NoSuchProjectException(project);
}
openRepos.put(project, repo);
checkState(!openRepos.containsKey(project),
"repo already opened: %s", project);
ProjectState projectState = projectCache.get(project);
if (projectState == null) {
throw new NoSuchProjectException(project);
}
try {
OpenRepo or =
new OpenRepo(repoManager.openRepository(project), projectState);
openRepos.put(project, or);
} catch (RepositoryNotFoundException e) {
throw new NoSuchProjectException(project);
}
return repo;
}
@Override
@ -474,8 +479,7 @@ public class MergeOp implements AutoCloseable {
}
public void merge(ReviewDb db, Change change, IdentifiedUser caller,
boolean checkSubmitRules) throws NoSuchChangeException,
OrmException, ResourceConflictException {
boolean checkSubmitRules) throws OrmException, ResourceConflictException {
this.caller = caller;
updateSubmissionId(change);
this.db = db;
@ -523,22 +527,22 @@ public class MergeOp implements AutoCloseable {
}
private void integrateIntoHistory(ChangeSet cs)
throws IntegrationException, NoSuchChangeException,
ResourceConflictException {
throws IntegrationException, ResourceConflictException {
logDebug("Beginning merge attempt on {}", cs);
Map<Branch.NameKey, BranchBatch> toSubmit = new HashMap<>();
logDebug("Perform the merges");
try {
Multimap<Project.NameKey, Branch.NameKey> br = cs.branchesByProject();
Multimap<Branch.NameKey, ChangeData> cbb = cs.changesByBranch();
openRepos(br.keySet());
for (Branch.NameKey branch : cbb.keySet()) {
OpenRepo or = openRepo(branch.getParentKey());
OpenRepo or = getRepo(branch.getParentKey());
toSubmit.put(branch, validateChangeList(or, cbb.get(branch)));
}
failFast(cs); // Done checks that don't involve running submit strategies.
for (Branch.NameKey branch : cbb.keySet()) {
OpenRepo or = openRepo(branch.getParentKey());
OpenRepo or = getRepo(branch.getParentKey());
OpenBranch ob = or.getBranch(branch);
BranchBatch submitting = toSubmit.get(branch);
SubmitStrategy strategy = createStrategy(or, branch,
@ -547,7 +551,7 @@ public class MergeOp implements AutoCloseable {
}
checkMergeStrategyResults(cs, toSubmit.values());
for (Project.NameKey project : br.keySet()) {
openRepo(project).ins.flush();
getRepo(project).ins.flush();
}
Set<Branch.NameKey> done =
@ -555,7 +559,7 @@ public class MergeOp implements AutoCloseable {
logDebug("Write out the new branch tips");
SubmoduleOp subOp = subOpProvider.get();
for (Project.NameKey project : br.keySet()) {
OpenRepo or = openRepo(project);
OpenRepo or = getRepo(project);
for (Branch.NameKey branch : br.get(project)) {
OpenBranch ob = or.getBranch(branch);
boolean updated = updateBranch(or, branch);
@ -574,10 +578,6 @@ public class MergeOp implements AutoCloseable {
checkState(done.equals(cbb.keySet()), "programmer error: did not process"
+ " all branches in input set.\nExpected: %s\nActual: %s",
done, cbb.keySet());
} catch (NoSuchProjectException noProject) {
logWarn("Project " + noProject.project() + " no longer exists, "
+ "abandoning open changes");
abandonAllOpenChanges(noProject.project());
} catch (OrmException e) {
throw new IntegrationException("Cannot query the database", e);
} catch (IOException e) {
@ -602,8 +602,7 @@ public class MergeOp implements AutoCloseable {
private SubmitStrategy createStrategy(OpenRepo or,
Branch.NameKey destBranch, SubmitType submitType,
CodeReviewCommit branchTip)
throws IntegrationException, NoSuchProjectException {
CodeReviewCommit branchTip) throws IntegrationException {
return submitStrategyFactory.create(submitType, db, or.repo, or.rw, or.ins,
or.canMergeFlag, getAlreadyAccepted(or, branchTip), destBranch, caller,
commits);
@ -1209,19 +1208,33 @@ public class MergeOp implements AutoCloseable {
});
}
private void abandonAllOpenChanges(Project.NameKey destProject)
throws NoSuchChangeException {
private void openRepos(Collection<Project.NameKey> projects)
throws IntegrationException {
for (Project.NameKey project : projects) {
try {
openRepo(project);
} catch (NoSuchProjectException noProject) {
logWarn("Project " + noProject.project() + " no longer exists, "
+ "abandoning open changes");
abandonAllOpenChanges(noProject.project());
} catch (IOException e) {
throw new IntegrationException("Error opening project " + project, e);
}
}
}
private void abandonAllOpenChanges(Project.NameKey destProject) {
try {
for (ChangeData cd : internalChangeQuery.byProjectOpen(destProject)) {
abandonOneChange(cd.change());
}
} catch (IOException | OrmException e) {
logWarn("Cannot abandon changes for deleted project ", e);
} catch (NoSuchChangeException | IOException | OrmException e) {
logWarn("Cannot abandon changes for deleted project " + destProject, e);
}
}
private void abandonOneChange(Change change) throws OrmException,
NoSuchChangeException, IOException {
NoSuchChangeException, IOException {
db.changes().beginTransaction(change.getId());
//TODO(dborowitz): support InternalUser in ChangeUpdate

2
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@ -1830,8 +1830,6 @@ public class ReceiveCommits {
try (MergeOp op = mergeOpProvider.get()) {
op.merge(db, rsrc.getChange(),
changeCtl.getUser().asIdentifiedUser(), false);
} catch (NoSuchChangeException e) {
throw new OrmException(e);
}
addMessage("");
Change c = db.changes().get(rsrc.getChange().getId());

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

@ -139,7 +139,8 @@ public abstract class SubmitStrategy {
this.db = db;
this.alreadyAccepted = alreadyAccepted;
this.project = projectCache.get(destBranch.getParentKey());
this.project = checkNotNull(projectCache.get(destBranch.getParentKey()),
"project not found: %s", destBranch.getParentKey());
this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag);
this.mergeUtil = mergeUtilFactory.create(project);
}

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

@ -21,7 +21,6 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.git.IntegrationException;
import com.google.gerrit.server.git.MergeOp.CommitStatus;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@ -51,12 +50,9 @@ public class SubmitStrategyFactory {
Repository repo, CodeReviewRevWalk rw, ObjectInserter inserter,
RevFlag canMergeFlag, Set<RevCommit> alreadyAccepted,
Branch.NameKey destBranch, IdentifiedUser caller, CommitStatus commits)
throws IntegrationException, NoSuchProjectException {
throws IntegrationException {
SubmitStrategy.Arguments args = argsFactory.create(destBranch, commits, rw,
caller, inserter, repo, canMergeFlag, db, alreadyAccepted);
if (args.project == null) {
throw new NoSuchProjectException(destBranch.getParentKey());
}
switch (submitType) {
case CHERRY_PICK:
return new CherryPick(args);