Parse ChangeNotes from staged results if auto-rebuilding fails

Loading the change screen is really pessimal when it comes to
rebuilding out-of-date changes in NoteDb, because it causes 10 or so
request handlers to try to rebuild the change concurrently. Even on a
backend that's more performant than googlesource.com's, this can lead
to contention when trying to update the same NoteDb ref.

A previous fix in I0b80c975 handles some failure modes by rechecking
the change after a failure during rebuilding, but that wasn't enough.
Go one step further and just serve up the ChangeNotes from data the
caller tried but failed to store in NoteDb. For a given state in
ReviewDb (i.e. a given ChangeBundle), the exact set of git objects
produced by rebuilding is completely deterministic, so we can say with
confidence that this is what we should have returned. Of course, it's
still possible that multiple concurrent read-and-rebuild requests will
race with concurrent write requests, so different requests may have
rebuilt from different snapshots of ReviewDb. But this is a completely
normal and acceptable race: if a write happens in the middle of a
sequence of concurrent reads, some reads will observe the old state
and some will observe the new state.

Change-Id: I2797d4fbc59fe9e0507db21ed53c69625df1eb07
This commit is contained in:
Dave Borowitz
2016-06-16 13:17:30 -04:00
parent 1635b5d6a4
commit 531d1d3a2d
9 changed files with 267 additions and 82 deletions

View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkNotNull;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableListMultimap;
@@ -25,6 +27,7 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.git.RepoRefCache;
import com.google.gerrit.server.notedb.NoteDbUpdateManager.StagedResult;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.assistedinject.Assisted;
@@ -36,6 +39,7 @@ import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.ReceiveCommand;
import java.io.IOException;
@@ -52,6 +56,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
private final Change change;
private final Account.Id author;
private final NoteDbUpdateManager.Result rebuildResult;
private ImmutableListMultimap<RevId, PatchLineComment> comments;
private RevisionNoteMap revisionNoteMap;
@@ -61,7 +66,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
Args args,
@Assisted Change change,
@Assisted Account.Id author) {
this(args, change, author, true);
this(args, change, author, true, null);
}
@AssistedInject
@@ -72,16 +77,19 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
super(args, changeId, true);
this.change = null;
this.author = author;
this.rebuildResult = null;
}
DraftCommentNotes(
Args args,
Change change,
Account.Id author,
boolean autoRebuild) {
boolean autoRebuild,
NoteDbUpdateManager.Result rebuildResult) {
super(args, change.getId(), autoRebuild);
this.change = change;
this.author = author;
this.rebuildResult = rebuildResult;
}
RevisionNoteMap getRevisionNoteMap() {
@@ -146,7 +154,12 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
@Override
protected LoadHandle openHandle(Repository repo) throws IOException {
if (change != null && autoRebuild) {
if (rebuildResult != null) {
StagedResult sr = checkNotNull(rebuildResult.staged());
return LoadHandle.create(
ChangeNotesCommit.newStagedRevWalk(repo, sr.allUsersObjects()),
findNewId(sr.allUsersCommands(), getRefName()));
} else if (change != null && autoRebuild) {
NoteDbChangeState state = NoteDbChangeState.parse(change);
// Only check if this particular user's drafts are up to date, to avoid
// reading unnecessary refs.
@@ -158,6 +171,16 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
return super.openHandle(repo);
}
private static ObjectId findNewId(
Iterable<ReceiveCommand> cmds, String refName) {
for (ReceiveCommand cmd : cmds) {
if (cmd.getRefName().equals(refName)) {
return cmd.getNewId();
}
}
return null;
}
private LoadHandle rebuildAndOpen(Repository repo) throws IOException {
try {
NoteDbUpdateManager.Result r =