opendev/gerrit
Code Issues Proposed changes

MergeOp: Avoid (one instance of) scanning all refs

Browse Source 
We were scanning all refs just to check that each patch set SHA-1 was
not completely orphaned. Instead, tighten this check slightly and
enforce that a patch set is present at its expected ref name. This
allows us to use a limited exactRef(String...) call rather than
scanning all potentially many thousands of refs.

Change-Id: I5308682b111bd69ade0345c51c63ee341226c1e2
This commit is contained in:
Dave Borowitz
2015-12-17 15:04:11 -05:00
parent 8061a65720
commit d650396842
1 changed files with 30 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -16,12 +16,12 @@ package com.google.gerrit.server.git;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static org.eclipse.jgit.lib.RefDatabase.ALL;
import com.google.auto.value.AutoValue;
import com.google.common.base.Optional;
import com.google.common.base.Predicate;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
@@ -581,18 +581,7 @@ public class MergeOp {
Collection<ChangeData> submitted) throws IntegrationException {
logDebug("Validating {} changes", submitted.size());
List<ChangeData> toSubmit = new ArrayList<>(submitted.size());
Map<String, Ref> allRefs;
try {
allRefs = repo.getRefDatabase().getRefs(ALL);
} catch (IOException e) {
throw new IntegrationException(e.getMessage(), e);
}
Set<ObjectId> tips = new HashSet<>();
for (Ref r : allRefs.values()) {
tips.add(r.getObjectId());
}
Multimap<ObjectId, PatchSet.Id> revisions = getRevisions(submitted);
SubmitType submitType = null;
ChangeData choseSubmitTypeFrom = null;
@@ -601,8 +590,7 @@ public class MergeOp {
Change chg;
try {
ctl = cd.changeControl();
// Reload change in case index was stale.
chg = cd.reloadChange();
chg = cd.change();
} catch (OrmException e) {
throw new IntegrationException("Failed to validate changes", e);
}
@@ -641,18 +629,13 @@ public class MergeOp {
continue;
}
if (!tips.contains(id)) {
// TODO Technically the proper way to do this test is to use a
// RevWalk on "$id --not --all" and test for an empty set. But
// that is way slower than looking for a ref directly pointing
// at the desired tip. We should always have a ref available.
//
if (!revisions.containsEntry(id, ps.getId())) {
// TODO this is actually an error, the branch is gone but we
// want to merge the issue. We can't safely do that if the
// tip is not reachable.
//
logError("Revision " + idstr + " of patch set " + ps.getId()
+ " is not contained in any ref");
+ " does not match " + ps.getId().toRefName());
commits.put(changeId, CodeReviewCommit.revisionGone(ctl));
continue;
}
@@ -705,6 +688,31 @@ public class MergeOp {
return new AutoValue_MergeOp_BranchBatch(submitType, toSubmit);
}
private Multimap<ObjectId, PatchSet.Id> getRevisions(
Collection<ChangeData> cds) throws IntegrationException {
try {
List<String> refNames = new ArrayList<>(cds.size());
for (ChangeData cd : cds) {
// Reload change in case index was stale.
cd.reloadChange();
Change c = cd.change();
if (c != null) {
refNames.add(c.currentPatchSetId().toRefName());
}
}
Multimap<ObjectId, PatchSet.Id> revisions =
HashMultimap.create(cds.size(), 1);
for (Map.Entry<String, Ref> e : repo.getRefDatabase().exactRef(
refNames.toArray(new String[refNames.size()])).entrySet()) {
revisions.put(
e.getValue().getObjectId(), PatchSet.Id.fromRef(e.getKey()));
}
return revisions;
} catch (IOException | OrmException e) {
throw new IntegrationException("Failed to validate changes", e);
}
}
private SubmitType getSubmitType(ChangeControl ctl, PatchSet ps) {
try {
ChangeData cd = changeDataFactory.create(db, ctl);