opendev/gerrit
Code Issues Proposed changes

Skip auto-closing changes when creating a new branch

Browse Source 
The refactoring and exception propagation in I6a4b2731f8 revealed
that we were previously covering up pushes that got stuck in while
attempting to auto-close changes.

After I6a4b2731f8, this now surfaces on the Git wire and users
started to report the error. Previosuly, the push would take
until the timeout was reached and then end gracefully covering
up this failure and contributing to the long-tail latency.

In the case of a new branch being pushed, we would previously
walk all commits loading all change notes from the pushed branch.

That is because we had no parent to mark as uninteresting. This
commit makes the logic return early to skip this behavior.

It's hard to test this because the result - even when walking all
commits - is the same and the issue only surfaces with many commits
being pushed and the timeout getting triggered.

Change-Id: Id4ce93ac499d8fe7063ba1365ebf7f3d6b4d9e0a
This commit is contained in:
Patrick Hiesel
2020-05-07 10:52:03 +02:00
parent acb11c4083
commit 7e0a59a64d
1 changed files with 11 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -737,7 +737,7 @@ class ReceiveCommits {
Set<BranchNameKey> branches = new HashSet<>();
for (ReceiveCommand c : cmds) {
// Most post-update steps should happen in UpdateOneRefOp#postUpdate. The only steps that
// should happen in this loop are things that can't happen within one BatchUpdate because
// should happen in this loops are things that can't happen within one BatchUpdate because
// they involve kicking off an additional BatchUpdate.
if (c.getResult() != OK) {
continue;
@@ -3238,6 +3238,13 @@ class ReceiveCommits {
ObjectInserter ins = repo.newObjectInserter();
ObjectReader reader = ins.newReader();
RevWalk rw = new RevWalk(reader)) {
if (ObjectId.zeroId().equals(cmd.getOldId())) {
// The user is creating a new branch. The branch can't contain any changes, so
// auto-closing doesn't apply. Exiting here early to spare any further,
// potentially expensive computation that loop over all commits.
return null;
}
bu.setRepository(repo, rw, ins);
// TODO(dborowitz): Teach BatchUpdate to ignore missing changes.
@@ -3247,9 +3254,7 @@ class ReceiveCommits {
rw.reset();
rw.sort(RevSort.REVERSE);
rw.markStart(newTip);
if (!ObjectId.zeroId().equals(cmd.getOldId())) {
rw.markUninteresting(rw.parseCommit(cmd.getOldId()));
}
rw.markUninteresting(rw.parseCommit(cmd.getOldId()));
Map<Change.Key, ChangeNotes> byKey = null;
List<ReplaceRequest> replaceAndClose = new ArrayList<>();
@@ -3261,6 +3266,8 @@ class ReceiveCommits {
for (RevCommit c; (c = rw.next()) != null; ) {
rw.parseBody(c);
// Check if change refs point to this commit. Usually there are 0-1 change
// refs pointing to this commit.
for (Ref ref :
receivePackRefCache.tipsFromObjectId(c.copy(), RefNames.REFS_CHANGES)) {
PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName());