Notedb: Fix reviewedBy computation in ChangeData
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>
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
index 9677f5f..1b7713b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -21,6 +21,7 @@
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 @@
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 @@
}
@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 abstract Account.Id author();
public abstract Timestamp ts();
-
- @Override
- public int compareTo(ReviewedByEvent other) {
- return ts().compareTo(other.ts());
- }
}
@Override
