From 96586f59d826961b3f58502699938a6dcfbe885e Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 16 Sep 2016 13:27:36 -0400 Subject: [PATCH] ChangeRebuilder: Add dependencies on PatchSetEvent This ensures events for patch sets don't get sorted before the PatchSetEvent that created them. This can happen due to timestamp skew, or the corner case described in If8ef0726. Change-Id: I45b16437518fb396e1d42d4aedb21a42258364ac --- .../server/notedb/ChangeRebuilderIT.java | 2 + .../gerrit/server/notedb/ChangeBundle.java | 37 ++++++++++++++++++- .../notedb/rebuild/ChangeRebuilderImpl.java | 31 +++++++++++----- .../gerrit/server/notedb/rebuild/Event.java | 3 +- .../server/notedb/ChangeBundleTest.java | 4 +- 5 files changed, 65 insertions(+), 12 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java index 4f14bb17be..8606ce77e3 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java @@ -715,6 +715,8 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { rin.message = "comment"; Timestamp ts = new Timestamp(c.getCreatedOn().getTime() + 2000); + assertThat(ts).isGreaterThan(c.getCreatedOn()); + assertThat(ts).isLessThan(db.patchSets().get(psId).getCreatedOn()); RevisionResource revRsrc = parseCurrentRevisionResource(r.getChangeId()); postReview.get().apply(revRsrc, rin, ts); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java index 00726bac13..fd931f5544 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java @@ -624,17 +624,31 @@ public class ChangeBundle { List tempDiffs = new ArrayList<>(); String temp = "temp"; + // ReviewDb allows timestamps before patch set was created, but NoteDb + // truncates this to the patch set creation timestamp. + Timestamp ta = a.getWrittenOn(); + Timestamp tb = b.getWrittenOn(); + PatchSet psa = bundleA.patchSets.get(a.getPatchSetId()); + PatchSet psb = bundleB.patchSets.get(b.getPatchSetId()); boolean excludePatchSet = false; + boolean excludeWrittenOn = false; if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) { excludePatchSet = a.getPatchSetId() == null; + excludeWrittenOn = psa != null && psb != null + && ta.before(psa.getCreatedOn()) && tb.equals(psb.getCreatedOn()); } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) { excludePatchSet = b.getPatchSetId() == null; + excludeWrittenOn = psa != null && psb != null + && tb.before(psb.getCreatedOn()) && ta.equals(psa.getCreatedOn()); } List exclude = Lists.newArrayList("key"); if (excludePatchSet) { exclude.add("patchset"); } + if (excludeWrittenOn) { + exclude.add("writtenOn"); + } diffColumnsExcluding( tempDiffs, ChangeMessage.class, temp, bundleA, a, bundleB, b, exclude); @@ -673,7 +687,28 @@ public class ChangeBundle { PatchSetApproval a = as.get(k); PatchSetApproval b = bs.get(k); String desc = describe(k); - diffColumns(diffs, PatchSetApproval.class, desc, bundleA, a, bundleB, b); + + // ReviewDb allows timestamps before patch set was created, but NoteDb + // truncates this to the patch set creation timestamp. + Timestamp ta = a.getGranted(); + Timestamp tb = b.getGranted(); + PatchSet psa = checkNotNull(bundleA.patchSets.get(a.getPatchSetId())); + PatchSet psb = checkNotNull(bundleB.patchSets.get(b.getPatchSetId())); + boolean excludeGranted = false; + List exclude = new ArrayList<>(1); + if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) { + excludeGranted = + ta.before(psa.getCreatedOn()) && tb.equals(psb.getCreatedOn()); + } else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) { + excludeGranted = + tb.before(psb.getCreatedOn()) && ta.equals(psa.getCreatedOn()); + } + if (excludeGranted) { + exclude.add("granted"); + } + + diffColumnsExcluding( + diffs, PatchSetApproval.class, desc, bundleA, a, bundleB, b, exclude); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java index 3389e2bb2f..ef2fdacaf4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java @@ -29,6 +29,7 @@ import com.google.common.base.Splitter; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Maps; import com.google.common.collect.Multimap; import com.google.common.collect.Ordering; import com.google.common.collect.Sets; @@ -92,6 +93,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Set; @@ -309,8 +311,8 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { deleteDraftRefs(change, manager.getAllUsersRepo()); Integer minPsNum = getMinPatchSetNum(bundle); - Set psIds = - Sets.newHashSetWithExpectedSize(bundle.getPatchSets().size()); + Map patchSetEvents = + Maps.newHashMapWithExpectedSize(bundle.getPatchSets().size()); for (PatchSet ps : bundle.getPatchSets()) { if (ps.getId().get() > currPsId.get()) { @@ -319,14 +321,15 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { ps.getId(), currPsId); continue; } - psIds.add(ps.getId()); - events.add(new PatchSetEvent( - change, ps, manager.getChangeRepo().rw)); + PatchSetEvent pse = + new PatchSetEvent(change, ps, manager.getChangeRepo().rw); + patchSetEvents.put(ps.getId(), pse); + events.add(pse); for (PatchLineComment c : getPatchLineComments(bundle, ps)) { PatchLineCommentEvent e = new PatchLineCommentEvent(c, change, ps, patchListCache); if (c.getStatus() == Status.PUBLISHED) { - events.add(e); + events.add(e.addDep(pse)); } else { draftCommentEvents.put(c.getAuthor(), e); } @@ -334,8 +337,9 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { } for (PatchSetApproval psa : bundle.getPatchSetApprovals()) { - if (psIds.contains(psa.getPatchSetId())) { - events.add(new ApprovalEvent(psa, change.getCreatedOn())); + PatchSetEvent pse = patchSetEvents.get(psa.getPatchSetId()); + if (pse != null) { + events.add(new ApprovalEvent(psa, change.getCreatedOn()).addDep(pse)); } } @@ -346,9 +350,18 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { Change noteDbChange = new Change(null, null, null, null, null); for (ChangeMessage msg : bundle.getChangeMessages()) { - if (msg.getPatchSetId() == null || psIds.contains(msg.getPatchSetId())) { + if (msg.getPatchSetId() == null) { + // No dependency necessary; will get assigned to most recent patch set + // in sortAndFillEvents. events.add( new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn())); + continue; + } + PatchSetEvent pse = patchSetEvents.get(msg.getPatchSetId()); + if (pse != null) { + events.add( + new ChangeMessageEvent(msg, noteDbChange, change.getCreatedOn()) + .addDep(pse)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java index a72e4fa43e..d04d1b5a2c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java @@ -66,8 +66,9 @@ abstract class Event implements Comparable { who, update.getNullableAccountId()); } - void addDep(Event e) { + Event addDep(Event e) { deps.add(e); + return this; } /** diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java index 054a82bc10..97bf864d76 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java @@ -1260,7 +1260,9 @@ public class ChangeBundleTest { } private static List latest(Change c) { - return ImmutableList.of(new PatchSet(c.currentPatchSetId())); + PatchSet ps = new PatchSet(c.currentPatchSetId()); + ps.setCreatedOn(c.getLastUpdatedOn()); + return ImmutableList.of(ps); } private static List approvals(PatchSetApproval... ents) {