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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebuild.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebuild.java index c27fa53d8c..39820b84e1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebuild.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebuild.java @@ -14,12 +14,19 @@ package com.google.gerrit.server.change; +import static java.util.stream.Collectors.joining; + +import com.google.gerrit.extensions.restapi.BinaryResult; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; -import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.reviewdb.server.ReviewDbUtil; +import com.google.gerrit.server.CommentsUtil; import com.google.gerrit.server.change.Rebuild.Input; +import com.google.gerrit.server.notedb.ChangeBundle; +import com.google.gerrit.server.notedb.ChangeBundleReader; +import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder; import com.google.gerrit.server.project.NoSuchChangeException; @@ -31,6 +38,7 @@ import com.google.inject.Singleton; import org.eclipse.jgit.errors.ConfigInvalidException; import java.io.IOException; +import java.util.List; @Singleton public class Rebuild implements RestModifyView { @@ -40,29 +48,63 @@ public class Rebuild implements RestModifyView { private final Provider db; private final NotesMigration migration; private final ChangeRebuilder rebuilder; + private final ChangeBundleReader bundleReader; + private final CommentsUtil commentsUtil; + private final ChangeNotes.Factory notesFactory; @Inject Rebuild(Provider db, NotesMigration migration, - ChangeRebuilder rebuilder) { + ChangeRebuilder rebuilder, + ChangeBundleReader bundleReader, + CommentsUtil commentsUtil, + ChangeNotes.Factory notesFactory) { this.db = db; this.migration = migration; this.rebuilder = rebuilder; + this.bundleReader = bundleReader; + this.commentsUtil = commentsUtil; + this.notesFactory = notesFactory; } @Override - public Response apply(ChangeResource rsrc, Input input) + public BinaryResult apply(ChangeResource rsrc, Input input) throws ResourceNotFoundException, IOException, OrmException, ConfigInvalidException { if (!migration.commitChangeWrites()) { throw new ResourceNotFoundException(); + } if (!migration.readChanges()) { + // ChangeBundle#fromNotes currently doesn't work if reading isn't enabled, + // so don't attempt a diff. + rebuild(rsrc); + return BinaryResult.create("Rebuilt change successfully"); } + + // Not the same transaction as the rebuild, so may result in spurious diffs + // in the case of races. This should be easy enough to detect by rerunning. + ChangeBundle reviewDbBundle = bundleReader.fromReviewDb( + ReviewDbUtil.unwrapDb(db.get()), rsrc.getId()); + rebuild(rsrc); + ChangeNotes notes = notesFactory.create( + db.get(), rsrc.getChange().getProject(), rsrc.getId()); + ChangeBundle noteDbBundle = ChangeBundle.fromNotes(commentsUtil, notes); + List diffs = reviewDbBundle.differencesFrom(noteDbBundle); + if (diffs.isEmpty()) { + return BinaryResult.create("No differences between ReviewDb and NoteDb"); + } + return BinaryResult.create( + diffs.stream() + .collect(joining( + "\n", "Differences between ReviewDb and NoteDb:\n", "\n"))); + } + + private void rebuild(ChangeResource rsrc) throws ResourceNotFoundException, + ConfigInvalidException, OrmException, IOException { try { rebuilder.rebuild(db.get(), rsrc.getId()); } catch (NoSuchChangeException e) { throw new ResourceNotFoundException( IdString.fromDecoded(rsrc.getId().toString())); } - return Response.none(); } } 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 4dcdec67ee..147a4671c7 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 @@ -40,7 +40,13 @@ abstract class Event implements Comparable { final Account.Id realUser; final String tag; final boolean predatesChange; + + /** + * Dependencies of this event; other events that must happen before this + * one. + */ final List deps; + Timestamp when; PatchSet.Id psId; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/EventSorter.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/EventSorter.java index 2ab4c00fbb..69587f4705 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/EventSorter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/EventSorter.java @@ -25,6 +25,7 @@ import com.google.common.collect.SetMultimap; import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; +import java.util.PriorityQueue; /** * Helper to sort a list of events. @@ -63,12 +64,12 @@ class EventSorter { void sort() { // First pass: sort by natural order. - Collections.sort(out); + PriorityQueue todo = new PriorityQueue<>(out); // Populate waiting map after initial sort to preserve natural order. waiting = ArrayListMultimap.create(); deps = HashMultimap.create(); - for (Event e : out) { + for (Event e : todo) { for (Event d : e.deps) { deps.put(e, d); waiting.put(d, e); @@ -77,8 +78,8 @@ class EventSorter { // Second pass: enforce dependencies. int size = out.size(); - for (Event e : out) { - process(e); + while (!todo.isEmpty()) { + process(todo.remove(), todo); } checkState(sorted.size() == size, "event sort expected %s elements, got %s", size, sorted.size()); @@ -88,20 +89,26 @@ class EventSorter { out.addAll(sorted); } - void process(Event e) { + void process(Event e, PriorityQueue todo) { if (sorted.contains(e)) { + return; // Already emitted. + } + if (!deps.get(e).isEmpty()) { + // Not all events that e depends on have been emitted yet. Ignore e for + // now; it will get added back to the queue in the block below once its + // last dependency is processed. return; } - // If all events that e depends on have been emitted: - // - e can be emitted. - // - remove e from the dependency set of all events waiting on e, and then - // re-process those events in case they can now be emitted. - if (deps.get(e).isEmpty()) { - sorted.add(e); - for (Event w : waiting.get(e)) { - deps.get(w).remove(e); - process(w); - } + + // All events that e depends on have been emitted, so e can be emitted. + sorted.add(e); + + // Remove e from the dependency set of all events waiting on e, and add + // those events back to the queue in the original priority order for + // reconsideration. + for (Event w : waiting.get(e)) { + deps.get(w).remove(e); + todo.add(w); } } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/rebuild/EventSorterTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/rebuild/EventSorterTest.java index f5fdf13e86..1db59c58d6 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/rebuild/EventSorterTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/rebuild/EventSorterTest.java @@ -15,8 +15,10 @@ package com.google.gerrit.server.notedb.rebuild; import static com.google.common.truth.Truth.assertThat; +import static java.util.stream.Collectors.toList; import static org.junit.Assert.fail; +import com.google.common.collect.Collections2; import com.google.common.collect.Lists; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.reviewdb.client.Account; @@ -29,8 +31,10 @@ import org.junit.Before; import org.junit.Test; import java.sql.Timestamp; +import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; +import java.util.stream.Stream; public class EventSorterTest { private class TestEvent extends Event { @@ -50,6 +54,12 @@ public class EventSorterTest { void apply(ChangeUpdate update) { throw new UnsupportedOperationException(); } + + @SuppressWarnings("deprecation") + @Override + public String toString() { + return "E{" + when.getSeconds() + '}'; + } } private Timestamp changeCreatedOn; @@ -66,33 +76,73 @@ public class EventSorterTest { Event e2 = new TestEvent(TimeUtil.nowTs()); Event e3 = new TestEvent(TimeUtil.nowTs()); - List events = events(e2, e1, e3); - new EventSorter(events).sort(); - assertThat(events).containsExactly(e1, e2, e3).inOrder(); - } - - @Test - public void topoSortNoChange() { - Event e1 = new TestEvent(TimeUtil.nowTs()); - Event e2 = new TestEvent(TimeUtil.nowTs()); - Event e3 = new TestEvent(TimeUtil.nowTs()); - e2.addDep(e1); - - List events = events(e2, e1, e3); - new EventSorter(events).sort(); - assertThat(events).containsExactly(e1, e2, e3).inOrder(); + for (List events : Collections2.permutations(events(e1, e2, e3))) { + assertSorted(events, events(e1, e2, e3)); + } } @Test public void topoSortOneDep() { - Event e1 = new TestEvent(TimeUtil.nowTs()); - Event e2 = new TestEvent(TimeUtil.nowTs()); - Event e3 = new TestEvent(TimeUtil.nowTs()); - e1.addDep(e2); + List es; - List events = events(e2, e3, e1); - new EventSorter(events).sort(); - assertThat(events).containsExactly(e2, e1, e3).inOrder(); + // Input list is 0,1,2 + + // 0 depends on 1 => 1,0,2 + es = threeEventsOneDep(0, 1); + assertSorted(es, events(es, 1, 0, 2)); + + // 1 depends on 0 => 0,1,2 + es = threeEventsOneDep(1, 0); + assertSorted(es, events(es, 0, 1, 2)); + + // 0 depends on 2 => 1,2,0 + es = threeEventsOneDep(0, 2); + assertSorted(es, events(es, 1, 2, 0)); + + // 2 depends on 0 => 0,1,2 + es = threeEventsOneDep(2, 0); + assertSorted(es, events(es, 0, 1, 2)); + + // 1 depends on 2 => 0,2,1 + es = threeEventsOneDep(1, 2); + assertSorted(es, events(es, 0, 2, 1)); + + // 2 depends on 1 => 0,1,2 + es = threeEventsOneDep(2, 1); + assertSorted(es, events(es, 0, 1, 2)); + } + + private List threeEventsOneDep(int depFromIdx, int depOnIdx) { + List events = Lists.newArrayList( + new TestEvent(TimeUtil.nowTs()), + new TestEvent(TimeUtil.nowTs()), + new TestEvent(TimeUtil.nowTs())); + events.get(depFromIdx).addDep(events.get(depOnIdx)); + return events; + } + + @Test + public void lastEventDependsOnFirstEvent() { + List events = new ArrayList<>(); + for (int i = 0; i < 20; i++) { + events.add(new TestEvent(TimeUtil.nowTs())); + } + events.get(events.size() - 1).addDep(events.get(0)); + assertSorted(events, events); + } + + @Test + public void firstEventDependsOnLastEvent() { + List events = new ArrayList<>(); + for (int i = 0; i < 20; i++) { + events.add(new TestEvent(TimeUtil.nowTs())); + } + events.get(0).addDep(events.get(events.size() - 1)); + + List expected = new ArrayList<>(); + expected.addAll(events.subList(1, events.size())); + expected.add(events.get(0)); + assertSorted(events, expected); } @Test @@ -105,9 +155,9 @@ public class EventSorterTest { e2.addDep(e3); e3.addDep(e4); - List events = events(e1, e2, e3, e4); - new EventSorter(events).sort(); - assertThat(events).containsExactly(e4, e3, e2, e1).inOrder(); + assertSorted( + events(e1, e2, e3, e4), + events(e4, e3, e2, e1)); } @Test @@ -121,9 +171,9 @@ public class EventSorterTest { e2.addDep(e3); // Processing 3 pops 2, processing 4 pops 1. - List events = events(e2, e3, e1, e4); - new EventSorter(events).sort(); - assertThat(events).containsExactly(e3, e2, e4, e1).inOrder(); + assertSorted( + events(e2, e3, e1, e4), + events(e3, e2, e4, e1)); } @Test @@ -137,9 +187,9 @@ public class EventSorterTest { e3.addDep(e4); // Processing 4 pops 1, 2, 3 in natural order. - List events = events(e4, e3, e2, e1); - new EventSorter(events).sort(); - assertThat(events).containsExactly(e4, e1, e2, e3).inOrder(); + assertSorted( + events(e4, e3, e2, e1), + events(e4, e1, e2, e3)); } @Test @@ -150,9 +200,9 @@ public class EventSorterTest { // Implementation is not really defined, but infinite looping would be bad. // According to current implementation details, 2 pops 1, 1 pops 2 which was // already seen. - List events = events(e2, e1); - new EventSorter(events).sort(); - assertThat(events).containsExactly(e1, e2).inOrder(); + assertSorted( + events(e2, e1), + events(e1, e2)); } @Test @@ -171,7 +221,19 @@ public class EventSorterTest { } } - private List events(Event... es) { + private static List events(Event... es) { return Lists.newArrayList(es); } + + private static List events(List in, Integer... indexes) { + return Stream.of(indexes).map(in::get).collect(toList()); + } + + private static void assertSorted(List unsorted, List expected) { + List actual = new ArrayList<>(unsorted); + new EventSorter(actual).sort(); + assertThat(actual) + .named("sorted" + unsorted) + .isEqualTo(expected); + } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestTimeUtil.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestTimeUtil.java index 4c71c573fc..efb2b1989b 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestTimeUtil.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestTimeUtil.java @@ -22,6 +22,7 @@ import org.joda.time.DateTimeUtils; import org.joda.time.DateTimeUtils.MillisProvider; import org.joda.time.DateTimeZone; +import java.sql.Timestamp; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; @@ -65,8 +66,31 @@ public class TestTimeUtil { }); } + /** + * Set the clock to a specific timestamp. + * + * @param ts time to set + */ + public static synchronized void setClock(Timestamp ts) { + checkState(clockMs != null, "call resetWithClockStep first"); + clockMs.set(ts.getTime()); + } + + /** + * Increment the clock once by a given amount. + * + * @param clockStep amount to increment clock by. + * @param clockStepUnit time unit for {@code clockStep}. + */ + public static synchronized void incrementClock( + long clockStep, TimeUnit clockStepUnit) { + checkState(clockMs != null, "call resetWithClockStep first"); + clockMs.addAndGet(clockStepUnit.toMillis(clockStep)); + } + /** Reset the clock to use the actual system clock. */ public static synchronized void useSystemTime() { + clockMs = null; DateTimeUtils.setCurrentMillisSystem(); }