Support modifying the global ConfigNotesMigration at runtime

Most server code already needs to be able to deal with NotesMigration
values changing at runtime, since we make heavy usage of this feature in
tests. ConfigNotesMigration previously never needed to change at
runtime, because of the assumption that gerrit.config is never reloaded
while a server is running. Now that we plan to support online migration
to NoteDb, that assumption no longer holds.

Copying values from another instance is straightforward, we just need to
be careful to keep the on-disk and in-memory values in sync.

Change-Id: Ie29f7153420c38c4d11733c80a1c9a987affe5f4

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.notedb;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
 
+import com.google.auto.value.AutoValue;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.inject.AbstractModule;
@@ -79,62 +80,105 @@ public class ConfigNotesMigration extends NotesMigration {
     return cfg.toText();
   }
 
-  private final boolean writeChanges;
-  private final boolean readChanges;
-  private final boolean readChangeSequence;
-  private final PrimaryStorage changePrimaryStorage;
-  private final boolean disableChangeReviewDb;
-  private final boolean fuseUpdates;
+  @AutoValue
+  abstract static class Snapshot {
+    static Snapshot create(Config cfg) {
+      boolean writeChanges = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), WRITE, false);
+      boolean readChanges = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), READ, false);
+
+      // Reading change sequence numbers from NoteDb is not the default even if
+      // reading changes themselves is. Once this is enabled, it's not easy to
+      // undo: ReviewDb might hand out numbers that have already been assigned by
+      // NoteDb. This decision for the default may be reevaluated later.
+      boolean readChangeSequence = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, false);
+
+      PrimaryStorage changePrimaryStorage =
+          cfg.getEnum(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB);
+      boolean disableChangeReviewDb =
+          cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, false);
+      boolean fuseUpdates = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false);
+
+      checkArgument(
+          !(disableChangeReviewDb && changePrimaryStorage != PrimaryStorage.NOTE_DB),
+          "cannot disable ReviewDb for changes if default change primary storage is ReviewDb");
+
+      return new AutoValue_ConfigNotesMigration_Snapshot(
+          writeChanges,
+          readChanges,
+          readChangeSequence,
+          changePrimaryStorage,
+          disableChangeReviewDb,
+          fuseUpdates);
+    }
+
+    abstract boolean writeChanges();
+
+    abstract boolean readChanges();
+
+    abstract boolean readChangeSequence();
+
+    abstract PrimaryStorage changePrimaryStorage();
+
+    abstract boolean disableChangeReviewDb();
+
+    abstract boolean fuseUpdates();
+  }
+
+  private volatile Snapshot snapshot;
 
   @Inject
   public ConfigNotesMigration(@GerritServerConfig Config cfg) {
-    writeChanges = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), WRITE, false);
-    readChanges = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), READ, false);
-
-    // Reading change sequence numbers from NoteDb is not the default even if
-    // reading changes themselves is. Once this is enabled, it's not easy to
-    // undo: ReviewDb might hand out numbers that have already been assigned by
-    // NoteDb. This decision for the default may be reevaluated later.
-    readChangeSequence = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, false);
-
-    changePrimaryStorage =
-        cfg.getEnum(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB);
-    disableChangeReviewDb =
-        cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, false);
-    fuseUpdates = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false);
-
-    checkArgument(
-        !(disableChangeReviewDb && changePrimaryStorage != PrimaryStorage.NOTE_DB),
-        "cannot disable ReviewDb for changes if default change primary storage is ReviewDb");
+    this.snapshot = Snapshot.create(cfg);
   }
 
   @Override
   public boolean rawWriteChangesSetting() {
-    return writeChanges;
+    return snapshot.writeChanges();
   }
 
   @Override
   public boolean readChanges() {
-    return readChanges;
+    return snapshot.readChanges();
   }
 
   @Override
   public boolean readChangeSequence() {
-    return readChangeSequence;
+    return snapshot.readChangeSequence();
   }
 
   @Override
   public PrimaryStorage changePrimaryStorage() {
-    return changePrimaryStorage;
+    return snapshot.changePrimaryStorage();
   }
 
   @Override
   public boolean disableChangeReviewDb() {
-    return disableChangeReviewDb;
+    return snapshot.disableChangeReviewDb();
   }
 
   @Override
   public boolean fuseUpdates() {
-    return fuseUpdates;
+    return snapshot.fuseUpdates();
+  }
+
+  /**
+   * Set the in-memory values returned by this instance to match another instance.
+   *
+   * <p>This method is only intended for use by {@link
+   * com.google.gerrit.server.notedb.rebuild.NoteDbMigrator}.
+   *
+   * <p>This <em>only</em> modifies the in-memory state; if this instance was initialized from a
+   * file-based config, the underlying storage is not updated. Callers are responsible for managing
+   * the underlying storage on their own. This method is synchronized to aid in such
+   * implementations.
+   *
+   * @see NotesMigration#setFrom(NotesMigration)
+   */
+  @Override
+  public synchronized ConfigNotesMigration setFrom(NotesMigration other) {
+    Config cfg = new Config();
+    setConfigValues(cfg, other);
+    snapshot = Snapshot.create(cfg);
+    return this;
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java

@@ -108,6 +108,18 @@ public abstract class NotesMigration {
    */
   public abstract boolean fuseUpdates();
 
+  /**
+   * Set the values returned by this instance to match another instance.
+   *
+   * <p>Optional operation: not all implementations support setting values after initialization.
+   *
+   * @param other other instance to copy values from.
+   * @return this.
+   */
+  public NotesMigration setFrom(NotesMigration other) {
+    throw new UnsupportedOperationException(getClass().getSimpleName() + " is read-only");
+  }
+
   /**
    * Whether to fail when reading any data from NoteDb.
    *

gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java

@@ -48,6 +48,7 @@ import com.google.gerrit.server.git.WorkQueue;
 import com.google.gerrit.server.notedb.ChangeBundleReader;
 import com.google.gerrit.server.notedb.ConfigNotesMigration;
 import com.google.gerrit.server.notedb.NoteDbUpdateManager;
+import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.notedb.NotesMigrationState;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder.NoPatchSetsException;
 import com.google.gwtorm.server.OrmException;
@@ -85,6 +86,7 @@ public class NoteDbMigrator implements AutoCloseable {
     private final ChangeRebuilder rebuilder;
     private final ChangeBundleReader bundleReader;
     private final WorkQueue workQueue;
+    private final NotesMigration globalNotesMigration;
 
     private int threads;
     private ImmutableList<Project.NameKey> projects = ImmutableList.of();
@@ -100,13 +102,15 @@ public class NoteDbMigrator implements AutoCloseable {
         NoteDbUpdateManager.Factory updateManagerFactory,
         ChangeRebuilder rebuilder,
         ChangeBundleReader bundleReader,
-        WorkQueue workQueue) {
+        WorkQueue workQueue,
+        NotesMigration globalNotesMigration) {
       this.sitePaths = sitePaths;
       this.schemaFactory = schemaFactory;
       this.updateManagerFactory = updateManagerFactory;
       this.rebuilder = rebuilder;
       this.bundleReader = bundleReader;
       this.workQueue = workQueue;
+      this.globalNotesMigration = globalNotesMigration;
     }
 
     /**
@@ -207,6 +211,7 @@ public class NoteDbMigrator implements AutoCloseable {
           updateManagerFactory,
           rebuilder,
           bundleReader,
+          globalNotesMigration,
           threads > 1
               ? MoreExecutors.listeningDecorator(workQueue.createQueue(threads, "RebuildChange"))
               : MoreExecutors.newDirectExecutorService(),
@@ -223,6 +228,7 @@ public class NoteDbMigrator implements AutoCloseable {
   private final NoteDbUpdateManager.Factory updateManagerFactory;
   private final ChangeRebuilder rebuilder;
   private final ChangeBundleReader bundleReader;
+  private final NotesMigration globalNotesMigration;
 
   private final ListeningExecutorService executor;
   private final ImmutableList<Project.NameKey> projects;
@@ -237,6 +243,7 @@ public class NoteDbMigrator implements AutoCloseable {
       NoteDbUpdateManager.Factory updateManagerFactory,
       ChangeRebuilder rebuilder,
       ChangeBundleReader bundleReader,
+      NotesMigration globalNotesMigration,
       ListeningExecutorService executor,
       ImmutableList<Project.NameKey> projects,
       ImmutableList<Change.Id> changes,
@@ -247,6 +254,7 @@ public class NoteDbMigrator implements AutoCloseable {
     this.updateManagerFactory = updateManagerFactory;
     this.rebuilder = rebuilder;
     this.bundleReader = bundleReader;
+    this.globalNotesMigration = globalNotesMigration;
 
     boolean hasChanges = !changes.isEmpty();
     boolean hasProjects = !projects.isEmpty();
@@ -364,24 +372,30 @@ public class NoteDbMigrator implements AutoCloseable {
 
   private NotesMigrationState saveState(
       NotesMigrationState expectedOldState, NotesMigrationState newState) throws IOException {
-    // This read-modify-write is racy. We're counting on the fact that no other Gerrit operation
-    // modifies gerrit.config, and hoping that admins don't either.
-    Optional<NotesMigrationState> actualOldState = loadState();
-    if (!actualOldState.equals(Optional.of(expectedOldState))) {
-      throw new MigrationException(
-          "Cannot move to new state:\n"
-              + newState.toText()
-              + "\n\n"
-              + "Expected this state in gerrit.config:\n"
-              + expectedOldState.toText()
-              + "\n\n"
-              + (actualOldState.isPresent()
-                  ? "But found this state:\n" + actualOldState.get().toText()
-                  : "But could not parse the current state"));
+    synchronized (globalNotesMigration) {
+      // This read-modify-write is racy. We're counting on the fact that no other Gerrit operation
+      // modifies gerrit.config, and hoping that admins don't either.
+      Optional<NotesMigrationState> actualOldState = loadState();
+      if (!actualOldState.equals(Optional.of(expectedOldState))) {
+        throw new MigrationException(
+            "Cannot move to new state:\n"
+                + newState.toText()
+                + "\n\n"
+                + "Expected this state in gerrit.config:\n"
+                + expectedOldState.toText()
+                + "\n\n"
+                + (actualOldState.isPresent()
+                    ? "But found this state:\n" + actualOldState.get().toText()
+                    : "But could not parse the current state"));
+      }
+      ConfigNotesMigration.setConfigValues(gerritConfig, newState.migration());
+      gerritConfig.save();
+
+      // Only set in-memory state once it's been persisted to storage.
+      globalNotesMigration.setFrom(newState.migration());
+
+      return newState;
     }
-    ConfigNotesMigration.setConfigValues(gerritConfig, newState.migration());
-    gerritConfig.save();
-    return newState;
   }
 
   public void rebuild() throws MigrationException, OrmException {

gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java

@@ -111,6 +111,7 @@ public class TestNotesMigration extends NotesMigration {
     return setFrom(NoteDbMode.get().migration);
   }
 
+  @Override
   public TestNotesMigration setFrom(NotesMigration other) {
     setWriteChanges(other.rawWriteChangesSetting());
     setReadChanges(other.readChanges());