ChangeData: Filter out zombie draft refs

Since I622dbbb3 the implementation of ChangeData#draftsByUser just
took the set of account IDs that have corresponding draft refs.
However, this is not actually the same as the set of users who have
drafts, because some drafts may be zombies: drafts where the published
comment was added to the change meta ref, but the later write to
All-Users to delete the draft failed. Since there is no atomicity
between the change repo and the All-Users repo, this can be expected
from time to time in normal operation. For this reason,
ChangeNotes#getDraftComments already knows how to filter out zombies.

Tweak the implementation of ChangeData#draftRefs to filter out refs
where all the comments on the ref are zombies. This restores the old
behavior of draftsByUser, which fixes the draftby search operator,
which is the main thing I'm concerned about. This is not strictly
correct because we will still return an over-populated ref from
draftRefs in the case where some but not all comments are zombies.
However, trying harder to fix this corner case is not worth the
effort. First of all, this case should be quite rare in the first
place, because the default behavior of PostReview is to publish all
comments on all revisions, deleting the ref. Second of all, as
mentioned, the main thing I'm concerned about is the set of users who
have drafts, and this implementation preserves that.

Change the implementation of ChangeNotes#getDraftComments to cache the
set of Comment.Keys in the published map. The linear search was more
ok when we weren't calling getDraftComments in a loop over all users.
Plus this new implementation results in slightly cleaner code.

Change-Id: I93ac69db418a8ad068d1919fd6e0fb39d290df5f
This commit is contained in:
Dave Borowitz
2017-01-05 17:48:48 -05:00
parent a82fdd0fde
commit 9d0236f88e
6 changed files with 137 additions and 29 deletions

View File

@@ -14,20 +14,22 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gerrit.reviewdb.client.RefNames.refsDraftComments;
import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.Multimap;
import com.google.common.collect.MultimapBuilder;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Comment;
import com.google.gerrit.reviewdb.client.PatchLineComment;
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.reviewdb.server.ReviewDb;
import com.google.gerrit.server.git.RepoRefCache;
@@ -42,6 +44,7 @@ import com.google.inject.assistedinject.AssistedInject;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -69,6 +72,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
private final Change change;
private final Account.Id author;
private final NoteDbUpdateManager.Result rebuildResult;
private final Ref ref;
private ImmutableListMultimap<RevId, Comment> comments;
private RevisionNoteMap<ChangeRevisionNote> revisionNoteMap;
@@ -78,7 +82,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
Args args,
@Assisted Change change,
@Assisted Account.Id author) {
this(args, change, author, true, null);
this(args, change, author, true, null, null);
}
@AssistedInject
@@ -92,6 +96,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
this.change = null;
this.author = author;
this.rebuildResult = null;
this.ref = null;
}
DraftCommentNotes(
@@ -99,11 +104,19 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
Change change,
Account.Id author,
boolean autoRebuild,
NoteDbUpdateManager.Result rebuildResult) {
@Nullable NoteDbUpdateManager.Result rebuildResult,
@Nullable Ref ref) {
super(args, change.getId(), PrimaryStorage.of(change), autoRebuild);
this.change = change;
this.author = author;
this.rebuildResult = rebuildResult;
this.ref = ref;
if (ref != null) {
checkArgument(
ref.getName().equals(getRefName()),
"draft ref not for change %s and account %s: %s",
getChangeId(), author, ref.getName());
}
}
RevisionNoteMap<ChangeRevisionNote> getRevisionNoteMap() {
@@ -129,7 +142,15 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
@Override
protected String getRefName() {
return RefNames.refsDraftComments(getChangeId(), author);
return refsDraftComments(getChangeId(), author);
}
@Override
protected ObjectId readRef(Repository repo) throws IOException {
if (ref != null) {
return ref.getObjectId();
}
return super.readRef(repo);
}
@Override