diff options
author | Dave Borowitz <dborowitz@google.com> | 2016-10-27 21:41:42 -0400 |
---|---|---|
committer | Dave Borowitz <dborowitz@google.com> | 2016-10-28 08:26:00 -0400 |
commit | 577d7cf664cc584cd808e6e4a621287b6b81feaa (patch) | |
tree | 44e0657f09ee320aceb0b4ca7b37f55e524627fa /gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java | |
parent | add796c082676b49e2557eba4c327c783debac44 (diff) |
EventSorter: Don't eagerly process events after satisfying deps
The previous EventSorter incorrectly handled a much later event having
a dependency on a much earlier event. For example, with a set of events
in the following natural order:
E1, E2, ..., E50
Say E50 depends on E1. The old sort algorithm would emit E1, see that
E50 now has no outstanding dependencies, and immediately emit E50,
before emitting E2. This is topologically correct but really screws
with the natural order.
This interacts very poorly with the later code that ensures commits in
the NoteDb graph have monotonic timestamps. One realistic scenario,
using the example above, is if E1 and E2 are PatchSetEvents and E50 is a
ChangeMessageEvent on PS1. The sorted result was [E1, E50, E2, ...],
and E2's timestamp would be squashed to be after E50's, which is way
off from the original timestamp of the ReviewDb PatchSet.
Fix this by not eagerly processing dependant events (E50) eagerly.
Instead, just insert them back in the queue for later processing.
Additionally, use a PriorityQueue to preserve the original order as
much as possible.
In addition to more comprehensive small tests in EventSorterTest, add a
real test in ChangeRebuilderIT with a scenario like the one described
above with data very much like we've observed in the wild.
Change-Id: Ia57fa7a72c3f48c1c87b43d0de45bc73f11f8fba
Diffstat (limited to 'gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java')
-rw-r--r-- | gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java | 17 |
1 files changed, 17 insertions, 0 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 dcdce9286f..29375fef37 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 @@ -72,6 +72,7 @@ import com.google.gerrit.server.notedb.NoteDbUpdateManager; import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException; import com.google.gerrit.server.project.Util; +import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.testutil.ConfigSuite; import com.google.gerrit.testutil.NoteDbChecker; import com.google.gerrit.testutil.NoteDbMode; @@ -1071,6 +1072,22 @@ public class ChangeRebuilderIT extends AbstractDaemonTest { assertThat(pub.get(1).getRealAuthor()).isEqualTo(admin.id); } + @Test + public void laterEventsDependingOnEarlierPatchSetDontIntefereWithOtherPatchSets() + throws Exception { + PushOneCommit.Result r1 = createChange(); + ChangeData cd = r1.getChange(); + Change.Id id = cd.getId(); + amendChange(cd.change().getKey().get()); + TestTimeUtil.incrementClock(90, TimeUnit.DAYS); + + ReviewInput rin = ReviewInput.approve(); + rin.message = "Some very late message on PS1"; + gApi.changes().id(id.get()).revision(1).review(rin); + + checker.rebuildAndCheckChanges(id); + } + private void assertChangesReadOnly(RestApiException e) throws Exception { Throwable cause = e.getCause(); assertThat(cause).isInstanceOf(UpdateException.class); |