opendev/gerrit
Code Issues Proposed changes

Notedb: Fix reviewedBy computation in ChangeData

Browse Source 
To find which users have reviewed a change we find all users that have
posted a message or uploaded a patch set after the change owner has
posted a message or uploaded a patch set. To do this we internally
created ReviewedByEvents based on the changes messages and the patch
sets and sorted them by date. When two actions were done with the same
timestamp this could result in a wrong order. E.g. the owner uploads
patch set 1 and then a user posts a review, we have 2 change messages
and 1 patch set and they all can have the same timestamp. For the
change message the correct order is guaranteed by respecting the order
of the commits on the refs/changes/XX/YYYY/meta branch, but doing a
sorting by timestamp in the reviewedBy computation may result in a
wrong order. Because of this the
ChangeIT#checkReviewedFlagBeforeAndAfterReview() was flaky when
notedb was enabled. To fix the issue the sorting by timestamp is
removed from the reviewedBy computation. This is possible by only
relying on the change messages and not considering the patch sets.
It's OK to ignore the patch set because any patch set creation also
creates a change message.

Change-Id: I959517ad562a0839b588fc2cebc4c2d05ee7a117
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin 2015-11-11 09:33:34 -08:00
parent 12088fc1b5
commit 40d64f6d43
1 changed files with 3 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@ -21,6 +21,7 @@ import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.SetMultimap;
import com.google.gerrit.common.data.SubmitRecord;
@ -808,10 +809,7 @@ public class ChangeData {
events.add(ReviewedByEvent.create(msg));
}
}
for (PatchSet ps : patchSets()) {
events.add(ReviewedByEvent.create(ps));
}
Collections.sort(events, Collections.reverseOrder());
events = Lists.reverse(events);
reviewedBy = new LinkedHashSet<>();
Account.Id owner = c.getOwner();
for (ReviewedByEvent event : events) {
@ -829,12 +827,7 @@ public class ChangeData {
}
@AutoValue
abstract static class ReviewedByEvent implements Comparable<ReviewedByEvent> {
private static ReviewedByEvent create(PatchSet ps) {
return new AutoValue_ChangeData_ReviewedByEvent(
ps.getUploader(), ps.getCreatedOn());
}
abstract static class ReviewedByEvent {
private static ReviewedByEvent create(ChangeMessage msg) {
return new AutoValue_ChangeData_ReviewedByEvent(
msg.getAuthor(), msg.getWrittenOn());
@ -842,11 +835,6 @@ public class ChangeData {
public abstract Account.Id author();
public abstract Timestamp ts();
@Override
public int compareTo(ReviewedByEvent other) {
return ts().compareTo(other.ts());
}
}
@Override