NoteDBMigrator: Introduce `skipAlreadyBuilt` setting

When running a `rebuild()` via the `NoteDBMigrator` it is possible to
specify some filters to only limit the migration to a subset of changes.

For example, it is possible to specify which projects to migrate or
which projects not to, or even yet a specific list of changes.

For all the resulting changes however, the NoteDB migration happens
regardless of whether the change was previously built on NoteDB.

Introduce a new `skipAlreadyBuilt` filter (available for both offline
and online migration), which allows to only rebuild changes whose NoteDB
status is empty, meaning they are stored only in ReviewDB.

Note that because of this, the NoteDB migrator does not check if already
built changes need a NoteDb rebuild action, because of the meta-data
being amended.

Setting the `skipAlreadyBuilt` is useful to allow continuing a partial
migration without having to reprocess already built changes.

Release-Notes: NoteDBMigrator: Introduce `skipAlreadyBuilt` setting
Forward-Compatible: checked
Change-Id: Ic4cb4e5f3b4d2cc0b5ffa89e11532528e66a3e52

5 files changed, 181 insertions, 51 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 6ee6bc9ff1..cdacb0bed4 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -3766,6 +3766,20 @@ of imposing a higher load on the running server.
 +
 By default, 1.
 
+[[notedb.onlineMigrationSkipAlreadyBuilt]] notedb.onlineMigrationSkipAlreadyBuilt::
++
+This configuration parameter controls the behavior of the online migration
+process from reviewDb to noteDb. When enabled, it restricts the migration to
+only those changes that have not yet been previously built on NoteDB.
++
+It proves beneficial in those scenarios where you need to resume a migration
+that was halted midway.
++
+Note that, the NoteDB migrator does not check if already built changes need a
+NoteDb rebuild action, because of the meta-data being amended.
++
+By default, false.
+
 [[oauth]]
 === Section oauth
 
diff --git a/java/com/google/gerrit/pgm/MigrateToNoteDb.java b/java/com/google/gerrit/pgm/MigrateToNoteDb.java
index 83751ccedd..0200c327b1 100644
--- a/java/com/google/gerrit/pgm/MigrateToNoteDb.java
+++ b/java/com/google/gerrit/pgm/MigrateToNoteDb.java
@@ -93,6 +93,11 @@ public class MigrateToNoteDb extends SiteProgram {
   private boolean gc;
 
   @Option(
+      name = "--skip-already-built",
+      usage = "Do not process changes that have previously been built on NoteDb")
+  private boolean skipAlreadyBuilt;
+
+  @Option(
       name = "--shuffle-project-slices",
       usage =
           "Shuffle project slices to reduce memory"
@@ -175,6 +180,7 @@ public class MigrateToNoteDb extends SiteProgram {
               .setSequenceGap(sequenceGap)
               .setVerbose(verbose)
               .setLockLooseRefs(lockLooseRefs)
+              .setSkipAlreadyBuilt(skipAlreadyBuilt)
               .build()) {
         if (!projects.isEmpty()
             || !changes.isEmpty()
diff --git a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
index ea8b9f5259..0781105448 100644
--- a/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
+++ b/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java
@@ -209,6 +209,7 @@ public class NoteDbMigrator implements AutoCloseable {
     private boolean lockLooseRefs;
     private boolean verbose;
     private boolean executorMetrics = true;
+    private boolean skipAlreadyBuilt = false;
 
     @Inject
     Builder(
@@ -357,6 +358,21 @@ public class NoteDbMigrator implements AutoCloseable {
     }
 
     /**
+     * Do not process changes that have already been migrated to NoteDB.
+     *
+     * <p>By default, the NoteDBMigrator rebuilds changes regardless of whether they had already
+     * been migrated to NoteDB.
+     *
+     * @param skipAlreadyBuilt whether to skip changes that that have already been migrated to
+     *     NoteDB.
+     * @return this.
+     */
+    public Builder setSkipAlreadyBuilt(boolean skipAlreadyBuilt) {
+      this.skipAlreadyBuilt = skipAlreadyBuilt;
+      return this;
+    }
+
+    /**
      * Rebuild all changes in NoteDb from ReviewDb, even if Gerrit is currently configured to read
      * from NoteDb.
      *
@@ -526,7 +542,8 @@ public class NoteDbMigrator implements AutoCloseable {
           sequenceGap >= 0 ? sequenceGap : Sequences.getChangeSequenceGap(cfg),
           autoMigrate,
           lockLooseRefs,
-          verbose);
+          verbose,
+          skipAlreadyBuilt);
     }
   }
 
@@ -593,6 +610,7 @@ public class NoteDbMigrator implements AutoCloseable {
   private final boolean autoMigrate;
   private final boolean lockLooseRefs;
   private final boolean verbose;
+  private final boolean skipAlreadyBuilt;
 
   private final AtomicLong globalChangeCounter = new AtomicLong();
   private long totalChangeCount;
@@ -627,7 +645,8 @@ public class NoteDbMigrator implements AutoCloseable {
       int sequenceGap,
       boolean autoMigrate,
       boolean lockLooseRefs,
-      boolean verbose)
+      boolean verbose,
+      boolean skipAlreadyBuilt)
       throws MigrationException {
     if (ImmutableList.of(!changes.isEmpty(), !projects.isEmpty(), !skipProjects.isEmpty()).stream()
             .filter(e -> e)
@@ -667,6 +686,7 @@ public class NoteDbMigrator implements AutoCloseable {
     this.autoMigrate = autoMigrate;
     this.lockLooseRefs = lockLooseRefs;
     this.verbose = verbose;
+    this.skipAlreadyBuilt = skipAlreadyBuilt;
 
     // Stack notedb.config over gerrit.config, in the same way as GerritServerConfigProvider.
     this.gerritConfig = new FileBasedConfig(sitePaths.gerrit_config.toFile(), FS.detect());
@@ -1003,6 +1023,10 @@ public class NoteDbMigrator implements AutoCloseable {
     }
   }
 
+  private boolean shouldProcessChange(Change c) {
+    return !skipAlreadyBuilt || c.getNoteDbState() == null;
+  }
+
   private ImmutableListMultimap<Project.NameKey, Change.Id> getChangesByProject()
       throws OrmException {
     // Memoize all changes so we can close the db connection and allow other threads to use the full
@@ -1013,15 +1037,21 @@ public class NoteDbMigrator implements AutoCloseable {
             .build();
     try (ReviewDb db = unwrapDb(schemaFactory.open())) {
       if (!projects.isEmpty()) {
-        return byProject(db.changes().all(), c -> projects.contains(c.getProject()), out);
+        return byProject(
+            db.changes().all(),
+            c -> shouldProcessChange(c) && projects.contains(c.getProject()),
+            out);
       }
       if (!skipProjects.isEmpty()) {
-        return byProject(db.changes().all(), c -> !skipProjects.contains(c.getProject()), out);
+        return byProject(
+            db.changes().all(),
+            c -> shouldProcessChange(c) && !skipProjects.contains(c.getProject()),
+            out);
       }
       if (!changes.isEmpty()) {
-        return byProject(db.changes().get(changes), c -> true, out);
+        return byProject(db.changes().get(changes), this::shouldProcessChange, out);
       }
-      return byProject(db.changes().all(), c -> true, out);
+      return byProject(db.changes().all(), this::shouldProcessChange, out);
     }
   }
 
diff --git a/java/com/google/gerrit/server/notedb/rebuild/OnlineNoteDbMigrator.java b/java/com/google/gerrit/server/notedb/rebuild/OnlineNoteDbMigrator.java
index 107b2a75c2..40cee88bf1 100644
--- a/java/com/google/gerrit/server/notedb/rebuild/OnlineNoteDbMigrator.java
+++ b/java/com/google/gerrit/server/notedb/rebuild/OnlineNoteDbMigrator.java
@@ -46,6 +46,9 @@ public class OnlineNoteDbMigrator implements LifecycleListener {
 
   public static final String ONLINE_MIGRATION_PROJECTS = "onlineMigrationProjects";
 
+  private static final String ONLINE_MIGRATION_SKIP_ALREADY_BUILT =
+      "onlineMigrationSkipAlreadyBuilt";
+
   public static class Module extends LifecycleModule {
     private final boolean trial;
 
@@ -67,6 +70,7 @@ public class OnlineNoteDbMigrator implements LifecycleListener {
   private final boolean trial;
   private final int threads;
   private final String[] projects;
+  private final boolean skipAlreadyBuilt;
 
   @Inject
   OnlineNoteDbMigrator(
@@ -82,6 +86,8 @@ public class OnlineNoteDbMigrator implements LifecycleListener {
     this.trial = trial || NoteDbMigrator.getTrialMode(cfg);
     this.threads = cfg.getInt(SECTION_NOTE_DB, ONLINE_MIGRATION_THREADS, 1);
     this.projects = cfg.getStringList(SECTION_NOTE_DB, null, ONLINE_MIGRATION_PROJECTS);
+    this.skipAlreadyBuilt =
+        cfg.getBoolean(SECTION_NOTE_DB, null, ONLINE_MIGRATION_SKIP_ALREADY_BUILT, false);
   }
 
   @Override
@@ -102,7 +108,12 @@ public class OnlineNoteDbMigrator implements LifecycleListener {
     Stopwatch sw = Stopwatch.createStarted();
 
     Builder migratorBuilder =
-        migratorBuilderProvider.get().setThreads(threads).setAutoMigrate(true).setTrialMode(trial);
+        migratorBuilderProvider
+            .get()
+            .setThreads(threads)
+            .setAutoMigrate(true)
+            .setTrialMode(trial)
+            .setSkipAlreadyBuilt(skipAlreadyBuilt);
 
     try {
       // TODO(dborowitz): maybe expose a progress monitor somewhere.
diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
index 8bc0f531d0..74fffbddaf 100644
--- a/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java
@@ -79,6 +79,7 @@ import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
+import java.util.function.Consumer;
 import java.util.stream.Stream;
 import org.eclipse.jgit.internal.storage.file.FileRepository;
 import org.eclipse.jgit.junit.TestRepository;
@@ -295,12 +296,8 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
 
   @Test
   public void rebuildSubsetOfChanges() throws Exception {
-    setNotesMigrationState(WRITE);
-
-    PushOneCommit.Result r1 = createChange();
-    PushOneCommit.Result r2 = createChange();
-    Change.Id id1 = r1.getChange().getId();
-    Change.Id id2 = r2.getChange().getId();
+    Change.Id id1 = createChangeOnBothReviewDBAndNoteDb(project);
+    Change.Id id2 = createChangeOnBothReviewDBAndNoteDb(project);
 
     invalidateNoteDbState(id1, id2);
     migrate(b -> b.setChanges(ImmutableList.of(id2)), NoteDbMigrator::rebuild);
@@ -310,15 +307,9 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
 
   @Test
   public void rebuildSubsetOfProjects() throws Exception {
-    setNotesMigrationState(WRITE);
-
     Project.NameKey p2 = createProject("project2");
-    TestRepository<?> tr2 = cloneProject(p2, admin);
-
-    PushOneCommit.Result r1 = createChange();
-    PushOneCommit.Result r2 = pushFactory.create(db, admin.getIdent(), tr2).to("refs/for/master");
-    Change.Id id1 = r1.getChange().getId();
-    Change.Id id2 = r2.getChange().getId();
+    Change.Id id1 = createChangeOnBothReviewDBAndNoteDb(project);
+    Change.Id id2 = createChangeOnBothReviewDBAndNoteDb(p2);
 
     invalidateNoteDbState(id1, id2);
     migrate(b -> b.setProjects(ImmutableList.of(p2)), NoteDbMigrator::rebuild);
@@ -334,8 +325,6 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
         server.getTestInjector().createChildInjector(new OnlineNoteDbMigrator.Module(true));
     OnlineNoteDbMigrator onlineNoteDbMigrator = injector.getInstance(OnlineNoteDbMigrator.class);
 
-    setNotesMigrationState(WRITE);
-
     ProjectInput in = new ProjectInput();
     in.name = "project2";
     in.parent = allProjects.get();
@@ -343,12 +332,8 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
     gApi.projects().create(in);
     Project.NameKey p2 = new Project.NameKey(in.name);
 
-    TestRepository<?> tr2 = cloneProject(p2, admin);
-
-    PushOneCommit.Result r1 = createChange();
-    PushOneCommit.Result r2 = pushFactory.create(db, admin.getIdent(), tr2).to("refs/for/master");
-    Change.Id id1 = r1.getChange().getId();
-    Change.Id id2 = r2.getChange().getId();
+    Change.Id id1 = createChangeOnBothReviewDBAndNoteDb(project);
+    Change.Id id2 = createChangeOnBothReviewDBAndNoteDb(p2);
 
     invalidateNoteDbState(id1, id2);
 
@@ -360,19 +345,12 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
 
   @Test
   public void rebuildNonSkippedProjects() throws Exception {
-    setNotesMigrationState(WRITE);
-
     Project.NameKey p2 = createProject("project2");
-    TestRepository<?> tr2 = cloneProject(p2, admin);
     Project.NameKey p3 = createProject("project3");
-    TestRepository<?> tr3 = cloneProject(p3, admin);
 
-    PushOneCommit.Result r1 = createChange();
-    PushOneCommit.Result r2 = pushFactory.create(db, admin.getIdent(), tr2).to("refs/for/master");
-    PushOneCommit.Result r3 = pushFactory.create(db, admin.getIdent(), tr3).to("refs/for/master");
-    Change.Id id1 = r1.getChange().getId();
-    Change.Id id2 = r2.getChange().getId();
-    Change.Id id3 = r3.getChange().getId();
+    Change.Id id1 = createChangeOnBothReviewDBAndNoteDb(project);
+    Change.Id id2 = createChangeOnBothReviewDBAndNoteDb(p2);
+    Change.Id id3 = createChangeOnBothReviewDBAndNoteDb(p3);
 
     invalidateNoteDbState(id1, id2, id3);
     migrate(b -> b.setSkipProjects(ImmutableList.of(p3)), NoteDbMigrator::rebuild);
@@ -380,6 +358,84 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
     assertNotRebuilt(id3);
   }
 
+  @Test
+  public void skipAlreadyBuiltChanges_shouldMigrateChangesOnlyOnReviewDB() throws Exception {
+    Change.Id id1 = createChangeOnReviewDBOnly(project);
+    Change.Id id2 = createChangeOnBothReviewDBAndNoteDb(project);
+    invalidateNoteDbState(id2);
+
+    migrate(b -> b.setSkipAlreadyBuilt(true), NoteDbMigrator::rebuild);
+
+    assertRebuilt(id1);
+    assertNotRebuilt(id2);
+  }
+
+  @Test
+  public void skipAlreadyBuiltChanges_shouldHonourProjectsFilter() throws Exception {
+    Change.Id prj1_id1 = createChangeOnReviewDBOnly(project);
+
+    Project.NameKey project2 = createProject("project2");
+    Change.Id prj2_id2 = createChangeOnReviewDBOnly(project2);
+    Change.Id prj2_id3 = createChangeOnBothReviewDBAndNoteDb(project2);
+    invalidateNoteDbState(prj2_id3);
+
+    migrate(
+        b -> b.setSkipAlreadyBuilt(true).setProjects(ImmutableList.of(project2)),
+        NoteDbMigrator::rebuild);
+
+    assertNeverRebuilt(prj1_id1);
+    assertRebuilt(prj2_id2);
+    assertNotRebuilt(prj2_id3);
+  }
+
+  @Test
+  public void skipAlreadyBuiltChanges_shouldHonourSkipProjectsFilter() throws Exception {
+    Change.Id prj1_id1 = createChangeOnReviewDBOnly(project);
+
+    Project.NameKey project2 = createProject("project2");
+    Change.Id prj2_id2 = createChangeOnReviewDBOnly(project2);
+
+    setNotesMigrationState(WRITE);
+
+    migrate(
+        b -> b.setSkipAlreadyBuilt(true).setSkipProjects(ImmutableList.of(project2)),
+        NoteDbMigrator::rebuild);
+
+    assertRebuilt(prj1_id1);
+    assertNeverRebuilt(prj2_id2);
+  }
+
+  @Test
+  public void skipAlreadyBuiltChanges_shouldHonourChangesFilter() throws Exception {
+    Change.Id id1 = createChangeOnReviewDBOnly(project);
+
+    Change.Id id2 = createChangeOnBothReviewDBAndNoteDb(project);
+    invalidateNoteDbState(id2);
+
+    migrate(
+        b -> b.setSkipAlreadyBuilt(true).setChanges(ImmutableList.of(id1, id2)),
+        NoteDbMigrator::rebuild);
+
+    assertRebuilt(id1);
+    assertNotRebuilt(id2);
+  }
+
+  private Change.Id createChangeWithState(NotesMigrationState state, Project.NameKey p)
+      throws Exception {
+    setNotesMigrationState(state);
+    TestRepository<?> tr = cloneProject(p, admin);
+    PushOneCommit.Result r = pushFactory.create(db, admin.getIdent(), tr).to("refs/for/master");
+    return r.getChange().getId();
+  }
+
+  private Change.Id createChangeOnReviewDBOnly(Project.NameKey p) throws Exception {
+    return createChangeWithState(REVIEW_DB, p);
+  }
+
+  private Change.Id createChangeOnBothReviewDBAndNoteDb(Project.NameKey p) throws Exception {
+    return createChangeWithState(WRITE, p);
+  }
+
   private void invalidateNoteDbState(Change.Id... ids) throws OrmException {
     List<Change> list = new ArrayList<>(ids.length);
     try (ReviewDb db = schemaFactory.open()) {
@@ -392,22 +448,27 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
     }
   }
 
+  private void assertNeverRebuilt(Change.Id... ids) throws OrmException {
+    forAllChanges(c -> assertThat(c.getNoteDbState()).isNull(), ids);
+  }
+
   private void assertRebuilt(Change.Id... ids) throws OrmException {
-    try (ReviewDb db = schemaFactory.open()) {
-      for (Change.Id id : ids) {
-        NoteDbChangeState s = NoteDbChangeState.parse(db.changes().get(id));
-        assertThat(s.getChangeMetaId().name()).isNotEqualTo(INVALID_STATE);
-      }
-    }
+    forAllChanges(
+        c -> {
+          NoteDbChangeState s = NoteDbChangeState.parse(c);
+          assertThat(s.getChangeMetaId().name()).isNotNull();
+          assertThat(s.getChangeMetaId().name()).isNotEqualTo(INVALID_STATE);
+        },
+        ids);
   }
 
   private void assertNotRebuilt(Change.Id... ids) throws OrmException {
-    try (ReviewDb db = schemaFactory.open()) {
-      for (Change.Id id : ids) {
-        NoteDbChangeState s = NoteDbChangeState.parse(db.changes().get(id));
-        assertThat(s.getChangeMetaId().name()).isEqualTo(INVALID_STATE);
-      }
-    }
+    forAllChanges(
+        c -> {
+          NoteDbChangeState s = NoteDbChangeState.parse(c);
+          assertThat(s.getChangeMetaId().name()).isEqualTo(INVALID_STATE);
+        },
+        ids);
   }
 
   @Test
@@ -748,4 +809,12 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
           .collect(toImmutableSortedSet(naturalOrder()));
     }
   }
+
+  private void forAllChanges(Consumer<Change> assertChange, Change.Id... ids) throws OrmException {
+    try (ReviewDb db = schemaFactory.open()) {
+      for (Change.Id id : ids) {
+        assertChange.accept(db.changes().get(id));
+      }
+    }
+  }
 }