Rework NotesMigration hierarchy and get rid of test impl

It used to be that ConfigNotesMigration was the only kind of
NotesMigration in a real server, but it was always immutable, while
TestNotesMigration was the main kind of migration in acceptance tests,
which was mutable. However, now that we support modifying
ConfigNotesMigration at runtime as part of the online NoteDb migration
process, TestNotesMigration is no longer strictly necessary, and
continuing to support it is becoming more trouble than it's worth.

One major problem was that only TestNotesMigration was being populated
via NoteDbMode, and the NoteDbMode was not reflected in the
ConfigNotesMigration at all, so callers that were depending on
ConfigNotesMigration directly would not know about the NoteDb migration
state from the GERRIT_NOTEDB env var in tests.

We could have fixed this (and other) problems directly, but there is a
better solution: get rid of the test implementation entirely, and use
the same implementation of NotesMigration in tests as in a running
server.

The class hierarchy now contains only two classes: NotesMigration and
MutableNotesMigration. Most callers just care about inspecting the
state, so they can inject a NotesMigration. The few callers (migration,
tests) that care about mutating the state at runtime can inject/create
MutableNotesMigrations instead. As an implementation detail, the actual
NotesMigration instance continues to be mutable, containing a reference
to the Snapshot, but the base class does not contain any public methods
to mutate the state. We then ensure with Guice that there is only one
actual NotesMigration instance (the MutableNotesMigration), and callers
just may or may not have access to the mutation methods depending on
what they chose to inject.

Ensuring this gets set up correctly in tests requires a bit of tweaking.
  * Since the NotesMigration is populated in the @UseLocalDisk case from
    reading gerrit.config on disk, we need to prepopulate gerrit.config
    with the right config values at startup time.
  * Since MutableNotesMigration is not in the testutil package, it can't
    have its own setFromEnv() method that depends on NoteDbMode.
    Instead, construct MutableNotesMigrations from the test env by using
    a static factory method in NoteDbMode.

Change-Id: If06db3d025cf3e3c9fe464989d5f38a22ce70b56

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -95,6 +95,7 @@ import com.google.gerrit.server.mail.Address;
 import com.google.gerrit.server.mail.send.EmailHeader;
 import com.google.gerrit.server.notedb.ChangeNoteUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.MutableNotesMigration;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.project.Util;
@@ -104,9 +105,9 @@ import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.testutil.ConfigSuite;
 import com.google.gerrit.testutil.FakeEmailSender;
 import com.google.gerrit.testutil.FakeEmailSender.Message;
+import com.google.gerrit.testutil.NoteDbMode;
 import com.google.gerrit.testutil.SshMode;
 import com.google.gerrit.testutil.TempFileUtil;
-import com.google.gerrit.testutil.TestNotesMigration;
 import com.google.gson.Gson;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
@@ -215,7 +216,7 @@ public abstract class AbstractDaemonTest {
   @Inject protected PluginConfigFactory pluginConfig;
   @Inject protected Revisions revisions;
   @Inject protected SystemGroupBackend systemGroupBackend;
-  @Inject protected TestNotesMigration notesMigration;
+  @Inject protected MutableNotesMigration notesMigration;
   @Inject protected ChangeNotes.Factory notesFactory;
   @Inject protected Abandon changeAbandoner;
 
@@ -498,7 +499,7 @@ public abstract class AbstractDaemonTest {
       server.close();
       server = null;
     }
-    notesMigration.resetFromEnv();
+    NoteDbMode.resetFromEnv(notesMigration);
   }
 
   protected TestRepository<?>.CommitBuilder commitBuilder() throws Exception {
@@ -722,12 +723,12 @@ public abstract class AbstractDaemonTest {
   }
 
   protected Context disableDb() {
-    notesMigration.setFailOnLoad(true);
+    notesMigration.setFailOnLoadForTest(true);
     return atrScope.disableDb();
   }
 
   protected void enableDb(Context preDisableContext) {
-    notesMigration.setFailOnLoad(false);
+    notesMigration.setFailOnLoadForTest(false);
     atrScope.set(preDisableContext);
   }
 

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java

@@ -374,6 +374,8 @@ public class GerritServer implements AutoCloseable {
     cfg.setInt("receive", null, "threadPoolSize", 1);
     cfg.setInt("index", null, "threads", 1);
     cfg.setBoolean("index", null, "reindexAfterRefUpdate", false);
+
+    NoteDbMode.newNotesMigrationFromEnv().setConfigValues(cfg);
   }
 
   private static Injector createTestInjector(Daemon daemon) throws Exception {

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java

@@ -38,7 +38,6 @@ import com.google.gerrit.server.schema.SchemaVersion;
 import com.google.gerrit.testutil.InMemoryDatabase;
 import com.google.gerrit.testutil.InMemoryH2Type;
 import com.google.gerrit.testutil.InMemoryRepositoryManager;
-import com.google.gerrit.testutil.TestNotesMigration;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.OrmRuntimeException;
 import com.google.gwtorm.server.SchemaFactory;
@@ -78,7 +77,7 @@ class InMemoryTestingDatabaseModule extends LifecycleModule {
     bind(MetricMaker.class).to(DisabledMetricMaker.class);
     bind(DataSourceType.class).to(InMemoryH2Type.class);
 
-    bind(NotesMigration.class).to(TestNotesMigration.class);
+    install(new NotesMigration.Module());
     TypeLiteral<SchemaFactory<ReviewDb>> schemaFactory =
         new TypeLiteral<SchemaFactory<ReviewDb>>() {};
     bind(schemaFactory).to(NotesMigrationSchemaFactory.class);

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java

@@ -31,10 +31,10 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.notedb.ReviewerStateInternal;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gerrit.server.query.change.InternalChangeQuery;
-import com.google.gerrit.testutil.TestNotesMigration;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Provider;
 import com.google.inject.assistedinject.Assisted;
@@ -144,7 +144,7 @@ public class PushOneCommit {
   private final ChangeNotes.Factory notesFactory;
   private final ApprovalsUtil approvalsUtil;
   private final Provider<InternalChangeQuery> queryProvider;
-  private final TestNotesMigration notesMigration;
+  private final NotesMigration notesMigration;
   private final ReviewDb db;
   private final TestRepository<?> testRepo;
 
@@ -162,7 +162,7 @@ public class PushOneCommit {
       ChangeNotes.Factory notesFactory,
       ApprovalsUtil approvalsUtil,
       Provider<InternalChangeQuery> queryProvider,
-      TestNotesMigration notesMigration,
+      NotesMigration notesMigration,
       @Assisted ReviewDb db,
       @Assisted PersonIdent i,
       @Assisted TestRepository<?> testRepo)
@@ -185,7 +185,7 @@ public class PushOneCommit {
       ChangeNotes.Factory notesFactory,
       ApprovalsUtil approvalsUtil,
       Provider<InternalChangeQuery> queryProvider,
-      TestNotesMigration notesMigration,
+      NotesMigration notesMigration,
       @Assisted ReviewDb db,
       @Assisted PersonIdent i,
       @Assisted TestRepository<?> testRepo,
@@ -210,7 +210,7 @@ public class PushOneCommit {
       ChangeNotes.Factory notesFactory,
       ApprovalsUtil approvalsUtil,
       Provider<InternalChangeQuery> queryProvider,
-      TestNotesMigration notesMigration,
+      NotesMigration notesMigration,
       @Assisted ReviewDb db,
       @Assisted PersonIdent i,
       @Assisted TestRepository<?> testRepo,
@@ -237,7 +237,7 @@ public class PushOneCommit {
       ChangeNotes.Factory notesFactory,
       ApprovalsUtil approvalsUtil,
       Provider<InternalChangeQuery> queryProvider,
-      TestNotesMigration notesMigration,
+      NotesMigration notesMigration,
       @Assisted ReviewDb db,
       @Assisted PersonIdent i,
       @Assisted TestRepository<?> testRepo,
@@ -262,7 +262,7 @@ public class PushOneCommit {
       ChangeNotes.Factory notesFactory,
       ApprovalsUtil approvalsUtil,
       Provider<InternalChangeQuery> queryProvider,
-      TestNotesMigration notesMigration,
+      NotesMigration notesMigration,
       @Assisted ReviewDb db,
       @Assisted PersonIdent i,
       @Assisted TestRepository<?> testRepo,
@@ -288,7 +288,7 @@ public class PushOneCommit {
       ChangeNotes.Factory notesFactory,
       ApprovalsUtil approvalsUtil,
       Provider<InternalChangeQuery> queryProvider,
-      TestNotesMigration notesMigration,
+      NotesMigration notesMigration,
       ReviewDb db,
       PersonIdent i,
       TestRepository<?> testRepo,

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java

@@ -32,7 +32,6 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.index.GerritIndexStatus;
 import com.google.gerrit.server.index.change.ChangeIndexCollection;
 import com.google.gerrit.server.index.change.ChangeSchemaDefinitions;
-import com.google.gerrit.server.notedb.ConfigNotesMigration;
 import com.google.gerrit.server.notedb.NoteDbChangeState;
 import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NoteDbChangeState.RefState;
@@ -201,8 +200,7 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest {
 
   private void assertNotesMigrationState(NotesMigrationState expected) throws Exception {
     gerritConfig.load();
-    assertThat(NotesMigrationState.forNotesMigration(new ConfigNotesMigration(gerritConfig)))
-        .hasValue(expected);
+    assertThat(NotesMigrationState.forConfig(gerritConfig)).hasValue(expected);
   }
 
   private ReviewDb openUnderlyingReviewDb(ServerContext ctx) throws Exception {

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java

@@ -325,7 +325,7 @@ public class ChangeReviewersByEmailIT extends AbstractDaemonTest {
       input.state = state;
       gApi.changes().id(r.getChangeId()).addReviewer(input);
 
-      notesMigration.setFailOnLoad(true);
+      notesMigration.setFailOnLoadForTest(true);
       try {
         ChangeInfo info =
             Iterables.getOnlyElement(
@@ -335,7 +335,7 @@ public class ChangeReviewersByEmailIT extends AbstractDaemonTest {
                     .get());
         assertThat(info.reviewers).isEqualTo(ImmutableMap.of(state, ImmutableList.of(acc)));
       } finally {
-        notesMigration.setFailOnLoad(false);
+        notesMigration.setFailOnLoadForTest(false);
       }
     }
   }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java

@@ -38,7 +38,6 @@ import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.Sequences;
 import com.google.gerrit.server.config.SitePaths;
-import com.google.gerrit.server.notedb.ConfigNotesMigration;
 import com.google.gerrit.server.notedb.NoteDbChangeState;
 import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
 import com.google.gerrit.server.notedb.NoteDbChangeState.RefState;
@@ -380,15 +379,14 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest {
   private void assertNotesMigrationState(NotesMigrationState expected) throws Exception {
     assertThat(NotesMigrationState.forNotesMigration(notesMigration)).hasValue(expected);
     gerritConfig.load();
-    assertThat(NotesMigrationState.forNotesMigration(new ConfigNotesMigration(gerritConfig)))
-        .hasValue(expected);
+    assertThat(NotesMigrationState.forConfig(gerritConfig)).hasValue(expected);
   }
 
   private void setNotesMigrationState(NotesMigrationState state) throws Exception {
     gerritConfig.load();
-    ConfigNotesMigration.setConfigValues(gerritConfig, state.migration());
+    state.setConfigValues(gerritConfig);
     gerritConfig.save();
-    notesMigration.setFrom(state.migration());
+    notesMigration.setFrom(state);
   }
 
   @FunctionalInterface

gerrit-gpg/src/test/java/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java

@@ -46,7 +46,7 @@ import com.google.gerrit.server.util.RequestContext;
 import com.google.gerrit.server.util.ThreadLocalRequestContext;
 import com.google.gerrit.testutil.InMemoryDatabase;
 import com.google.gerrit.testutil.InMemoryModule;
-import com.google.gerrit.testutil.TestNotesMigration;
+import com.google.gerrit.testutil.NoteDbMode;
 import com.google.inject.Guice;
 import com.google.inject.Inject;
 import com.google.inject.Injector;
@@ -104,7 +104,8 @@ public class GerritPublicKeyCheckerTest {
         ImmutableList.of(
             Fingerprint.toString(keyB().getPublicKey().getFingerprint()),
             Fingerprint.toString(keyD().getPublicKey().getFingerprint())));
-    Injector injector = Guice.createInjector(new InMemoryModule(cfg, new TestNotesMigration()));
+    Injector injector =
+        Guice.createInjector(new InMemoryModule(cfg, NoteDbMode.newNotesMigrationFromEnv()));
 
     lifecycle = new LifecycleManager();
     lifecycle.add(injector);

gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitExperimental.java

@@ -14,15 +14,15 @@
 
 package com.google.gerrit.pgm.init;
 
-import static com.google.gerrit.server.notedb.ConfigNotesMigration.SECTION_NOTE_DB;
 import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
+import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB;
 
 import com.google.gerrit.extensions.client.UiType;
 import com.google.gerrit.pgm.init.api.ConsoleUI;
 import com.google.gerrit.pgm.init.api.InitFlags;
 import com.google.gerrit.pgm.init.api.InitStep;
 import com.google.gerrit.pgm.init.api.Section;
-import com.google.gerrit.server.notedb.ConfigNotesMigration;
+import com.google.gerrit.server.notedb.NotesMigrationState;
 import com.google.inject.Inject;
 import java.util.Locale;
 import javax.inject.Singleton;
@@ -66,7 +66,8 @@ class InitExperimental implements InitStep {
       return;
     }
 
-    Config defaultConfig = ConfigNotesMigration.allEnabledConfig();
+    Config defaultConfig = new Config();
+    NotesMigrationState.FINAL.setConfigValues(defaultConfig);
     for (String name : defaultConfig.getNames(SECTION_NOTE_DB, CHANGES.key())) {
       noteDbChanges.set(name, defaultConfig.getString(SECTION_NOTE_DB, CHANGES.key(), name));
     }

gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/SiteProgram.java

@@ -28,7 +28,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.GerritServerConfigModule;
 import com.google.gerrit.server.config.SitePath;
 import com.google.gerrit.server.git.GitRepositoryManagerModule;
-import com.google.gerrit.server.notedb.ConfigNotesMigration;
+import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.schema.DataSourceModule;
 import com.google.gerrit.server.schema.DataSourceProvider;
 import com.google.gerrit.server.schema.DataSourceType;
@@ -182,7 +182,7 @@ public abstract class SiteProgram extends AbstractProgram {
     modules.add(new DatabaseModule());
     modules.add(new SchemaModule());
     modules.add(cfgInjector.getInstance(GitRepositoryManagerModule.class));
-    modules.add(new ConfigNotesMigration.Module());
+    modules.add(new NotesMigration.Module());
 
     try {
       return Guice.createInjector(PRODUCTION, modules);

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

@@ -151,7 +151,7 @@ public abstract class AbstractChangeNotes<T> {
       loadDefaults();
       return self();
     }
-    if (args.migration.failOnLoad()) {
+    if (args.migration.failOnLoadForTest()) {
       throw new OrmException("Reading from NoteDb is disabled");
     }
     try (Timer1.Context timer = args.metrics.readLatency.start(CHANGES);

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

@@ -1,178 +0,0 @@
-// Copyright (C) 2013 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-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;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
-import org.eclipse.jgit.lib.Config;
-
-/**
- * Implement NoteDb migration stages using {@code gerrit.config}.
- *
- * <p>This class controls the state of the migration according to options in {@code gerrit.config}.
- * In general, any changes to these options should only be made by adventurous administrators, who
- * know what they're doing, on non-production data, for the purposes of testing the NoteDb
- * implementation. Changing options quite likely requires re-running {@code MigrateToNoteDb}. For
- * these reasons, the options remain undocumented.
- */
-@Singleton
-public class ConfigNotesMigration extends NotesMigration {
-  public static class Module extends AbstractModule {
-    @Override
-    public void configure() {
-      bind(NotesMigration.class).to(ConfigNotesMigration.class);
-    }
-  }
-
-  public static final String SECTION_NOTE_DB = "noteDb";
-
-  private static final String DISABLE_REVIEW_DB = "disableReviewDb";
-  private static final String FUSE_UPDATES = "fuseUpdates";
-  private static final String PRIMARY_STORAGE = "primaryStorage";
-  private static final String READ = "read";
-  private static final String SEQUENCE = "sequence";
-  private static final String WRITE = "write";
-
-  public static Config allEnabledConfig() {
-    Config cfg = new Config();
-    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), WRITE, true);
-    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), READ, true);
-    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, true);
-    cfg.setString(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.NOTE_DB.name());
-    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, true);
-    // TODO(dborowitz): Set to true when FileRepository supports it.
-    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false);
-    return cfg;
-  }
-
-  public static void setConfigValues(Config cfg, NotesMigration migration) {
-    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), WRITE, migration.rawWriteChangesSetting());
-    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), READ, migration.readChanges());
-    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, migration.readChangeSequence());
-    cfg.setEnum(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, migration.changePrimaryStorage());
-    cfg.setBoolean(
-        SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, migration.disableChangeReviewDb());
-    cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, migration.fuseUpdates());
-  }
-
-  public static String toText(NotesMigration migration) {
-    Config cfg = new Config();
-    setConfigValues(cfg, migration);
-    return cfg.toText();
-  }
-
-  @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);
-      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) {
-    this.snapshot = Snapshot.create(cfg);
-  }
-
-  @Override
-  public boolean rawWriteChangesSetting() {
-    return snapshot.writeChanges();
-  }
-
-  @Override
-  public boolean readChanges() {
-    return snapshot.readChanges();
-  }
-
-  @Override
-  public boolean readChangeSequence() {
-    return snapshot.readChangeSequence();
-  }
-
-  @Override
-  public PrimaryStorage changePrimaryStorage() {
-    return snapshot.changePrimaryStorage();
-  }
-
-  @Override
-  public boolean disableChangeReviewDb() {
-    return snapshot.disableChangeReviewDb();
-  }
-
-  @Override
-  public boolean 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/MutableNotesMigration.java

@@ -0,0 +1,108 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb;
+
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.function.Function;
+import org.eclipse.jgit.lib.Config;
+
+/**
+ * {@link NotesMigration} with additional methods for altering the migration state at runtime.
+ *
+ * <p>Almost all callers care only about inspecting the migration state, and for safety should not
+ * have access to mutation methods, which must be used with extreme care. Those callers should
+ * inject {@link NotesMigration}.
+ *
+ * <p>Some callers, namely the NoteDb migration pipeline and tests, do need to alter the migration
+ * state at runtime, and those callers are expected to take the necessary precautions such as
+ * keeping the in-memory and on-disk config state in sync. Those callers use this class.
+ *
+ * <p>Mutations to the {@link MutableNotesMigration} are guaranteed to be instantly visible to all
+ * callers that use the non-mutable {@link NotesMigration}. The current implementation accomplishes
+ * this by always binding {@link NotesMigration} to {@link MutableNotesMigration} in Guice, so there
+ * is just one {@link NotesMigration} instance process-wide.
+ */
+@Singleton
+public class MutableNotesMigration extends NotesMigration {
+  public static MutableNotesMigration newDisabled() {
+    return new MutableNotesMigration(new Config());
+  }
+
+  public static MutableNotesMigration fromConfig(Config cfg) {
+    return new MutableNotesMigration(cfg);
+  }
+
+  @Inject
+  MutableNotesMigration(@GerritServerConfig Config cfg) {
+    super(Snapshot.create(cfg));
+  }
+
+  public MutableNotesMigration setReadChanges(boolean readChanges) {
+    return set(b -> b.setReadChanges(readChanges));
+  }
+
+  public MutableNotesMigration setWriteChanges(boolean writeChanges) {
+    return set(b -> b.setWriteChanges(writeChanges));
+  }
+
+  public MutableNotesMigration setReadChangeSequence(boolean readChangeSequence) {
+    return set(b -> b.setReadChangeSequence(readChangeSequence));
+  }
+
+  public MutableNotesMigration setChangePrimaryStorage(PrimaryStorage changePrimaryStorage) {
+    return set(b -> b.setChangePrimaryStorage(changePrimaryStorage));
+  }
+
+  public MutableNotesMigration setDisableChangeReviewDb(boolean disableChangeReviewDb) {
+    return set(b -> b.setDisableChangeReviewDb(disableChangeReviewDb));
+  }
+
+  public MutableNotesMigration setFuseUpdates(boolean fuseUpdates) {
+    return set(b -> b.setFuseUpdates(fuseUpdates));
+  }
+
+  public MutableNotesMigration setFailOnLoadForTest(boolean failOnLoadForTest) {
+    return set(b -> b.setFailOnLoadForTest(failOnLoadForTest));
+  }
+
+  /**
+   * Set the in-memory values returned by this instance to match the given state.
+   *
+   * <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.
+   */
+  public MutableNotesMigration setFrom(NotesMigrationState state) {
+    snapshot.set(state.snapshot());
+    return this;
+  }
+
+  /** @see #setFrom(NotesMigrationState) */
+  public MutableNotesMigration setFrom(NotesMigration other) {
+    snapshot.set(other.snapshot.get());
+    return this;
+  }
+
+  private MutableNotesMigration set(Function<Snapshot.Builder, Snapshot.Builder> f) {
+    snapshot.updateAndGet(s -> f.apply(s.toBuilder()).build());
+    return this;
+  }
+}

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

@@ -1,4 +1,4 @@
-// Copyright (C) 2016 The Android Open Source Project
+// Copyright (C) 2017 The Android Open Source Project
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -14,8 +14,14 @@
 
 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.notedb.NoteDbChangeState.PrimaryStorage;
-import java.util.Objects;
+import com.google.inject.AbstractModule;
+import java.util.concurrent.atomic.AtomicReference;
+import org.eclipse.jgit.lib.Config;
 
 /**
  * Current low-level settings of the NoteDb migration for changes.
@@ -47,6 +53,100 @@ import java.util.Objects;
  * NotesMigration}'s methods will not change in a running server.
  */
 public abstract class NotesMigration {
+  public static final String SECTION_NOTE_DB = "noteDb";
+
+  private static final String DISABLE_REVIEW_DB = "disableReviewDb";
+  private static final String FUSE_UPDATES = "fuseUpdates";
+  private static final String PRIMARY_STORAGE = "primaryStorage";
+  private static final String READ = "read";
+  private static final String SEQUENCE = "sequence";
+  private static final String WRITE = "write";
+
+  public static class Module extends AbstractModule {
+    @Override
+    public void configure() {
+      bind(MutableNotesMigration.class);
+      bind(NotesMigration.class).to(MutableNotesMigration.class);
+    }
+  }
+
+  @AutoValue
+  abstract static class Snapshot {
+    static Builder builder() {
+      // Default values are defined as what we would read from an empty config.
+      return create(new Config()).toBuilder();
+    }
+
+    static Snapshot create(Config cfg) {
+      return new AutoValue_NotesMigration_Snapshot.Builder()
+          .setWriteChanges(cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), WRITE, false))
+          .setReadChanges(cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), READ, false))
+          .setReadChangeSequence(cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, false))
+          .setChangePrimaryStorage(
+              cfg.getEnum(
+                  SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB))
+          .setDisableChangeReviewDb(
+              cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, false))
+          .setFuseUpdates(cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false))
+          .setFailOnLoadForTest(false) // Only set in tests, can't be set via config.
+          .build();
+    }
+
+    abstract boolean writeChanges();
+
+    abstract boolean readChanges();
+
+    abstract boolean readChangeSequence();
+
+    abstract PrimaryStorage changePrimaryStorage();
+
+    abstract boolean disableChangeReviewDb();
+
+    abstract boolean fuseUpdates();
+
+    abstract boolean failOnLoadForTest();
+
+    abstract Builder toBuilder();
+
+    void setConfigValues(Config cfg) {
+      cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), WRITE, writeChanges());
+      cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), READ, readChanges());
+      cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, readChangeSequence());
+      cfg.setEnum(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, changePrimaryStorage());
+      cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, disableChangeReviewDb());
+      cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, fuseUpdates());
+    }
+
+    @AutoValue.Builder
+    abstract static class Builder {
+      abstract Builder setWriteChanges(boolean writeChanges);
+
+      abstract Builder setReadChanges(boolean readChanges);
+
+      abstract Builder setReadChangeSequence(boolean readChangeSequence);
+
+      abstract Builder setChangePrimaryStorage(PrimaryStorage changePrimaryStorage);
+
+      abstract Builder setDisableChangeReviewDb(boolean disableChangeReviewDb);
+
+      abstract Builder setFuseUpdates(boolean fuseUpdates);
+
+      abstract Builder setFailOnLoadForTest(boolean failOnLoadForTest);
+
+      abstract Snapshot autoBuild();
+
+      Snapshot build() {
+        Snapshot s = autoBuild();
+        checkArgument(
+            !(s.disableChangeReviewDb() && s.changePrimaryStorage() != PrimaryStorage.NOTE_DB),
+            "cannot disable ReviewDb for changes if default change primary storage is ReviewDb");
+        return s;
+      }
+    }
+  }
+
+  protected final AtomicReference<Snapshot> snapshot;
+
   /**
    * Read changes from NoteDb.
    *
@@ -57,7 +157,9 @@ public abstract class NotesMigration {
    * <p>If true and {@code writeChanges() = false}, changes can still be read from NoteDb, but any
    * attempts to write will generate an error.
    */
-  public abstract boolean readChanges();
+  public final boolean readChanges() {
+    return snapshot.get().readChanges();
+  }
 
   /**
    * Write changes to NoteDb.
@@ -73,7 +175,9 @@ public abstract class NotesMigration {
    * readChanges() = false}, writes to NoteDb are simply ignored; if {@code true}, any attempts to
    * write will generate an error.
    */
-  public abstract boolean rawWriteChangesSetting();
+  public final boolean rawWriteChangesSetting() {
+    return snapshot.get().writeChanges();
+  }
 
   /**
    * Read sequential change ID numbers from NoteDb.
@@ -81,10 +185,14 @@ public abstract class NotesMigration {
    * <p>If true, change IDs are read from {@code refs/sequences/changes} in All-Projects. If false,
    * change IDs are read from ReviewDb's native sequences.
    */
-  public abstract boolean readChangeSequence();
+  public final boolean readChangeSequence() {
+    return snapshot.get().readChangeSequence();
+  }
 
   /** @return default primary storage for new changes. */
-  public abstract PrimaryStorage changePrimaryStorage();
+  public final PrimaryStorage changePrimaryStorage() {
+    return snapshot.get().changePrimaryStorage();
+  }
 
   /**
    * Disable ReviewDb access for changes.
@@ -93,7 +201,9 @@ public abstract class NotesMigration {
    * results; updates do nothing, as does opening, committing, or rolling back a transaction on the
    * Changes table.
    */
-  public abstract boolean disableChangeReviewDb();
+  public final boolean disableChangeReviewDb() {
+    return snapshot.get().disableChangeReviewDb();
+  }
 
   /**
    * Fuse meta ref updates in the same batch as code updates.
@@ -106,18 +216,8 @@ public abstract class NotesMigration {
    *
    * <p>Has no effect if {@link #disableChangeReviewDb()} is false.
    */
-  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");
+  public final boolean fuseUpdates() {
+    return snapshot.get().fuseUpdates();
   }
 
   /**
@@ -125,8 +225,8 @@ public abstract class NotesMigration {
    *
    * <p>Used in conjunction with {@link #readChanges()} for tests.
    */
-  public boolean failOnLoad() {
-    return false;
+  public boolean failOnLoadForTest() {
+    return snapshot.get().failOnLoadForTest();
   }
 
   public final boolean commitChangeWrites() {
@@ -151,30 +251,26 @@ public abstract class NotesMigration {
     return rawWriteChangesSetting() || readChanges();
   }
 
+  public final void setConfigValues(Config cfg) {
+    snapshot.get().setConfigValues(cfg);
+  }
+
   @Override
   public final boolean equals(Object o) {
-    if (!(o instanceof NotesMigration)) {
-      return false;
-    }
-    NotesMigration m = (NotesMigration) o;
-    return readChanges() == m.readChanges()
-        && rawWriteChangesSetting() == m.rawWriteChangesSetting()
-        && readChangeSequence() == m.readChangeSequence()
-        && changePrimaryStorage() == m.changePrimaryStorage()
-        && disableChangeReviewDb() == m.disableChangeReviewDb()
-        && fuseUpdates() == m.fuseUpdates()
-        && failOnLoad() == m.failOnLoad();
+    return o instanceof NotesMigration
+        && snapshot.get().equals(((NotesMigration) o).snapshot.get());
   }
 
   @Override
   public final int hashCode() {
-    return Objects.hash(
-        readChanges(),
-        rawWriteChangesSetting(),
-        readChangeSequence(),
-        changePrimaryStorage(),
-        disableChangeReviewDb(),
-        fuseUpdates(),
-        failOnLoad());
+    return snapshot.get().hashCode();
+  }
+
+  protected NotesMigration(Snapshot snapshot) {
+    this.snapshot = new AtomicReference<>(snapshot);
+  }
+
+  final Snapshot snapshot() {
+    return snapshot.get();
   }
 }

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

@@ -15,8 +15,10 @@
 package com.google.gerrit.server.notedb;
 
 import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
+import com.google.gerrit.server.notedb.NotesMigration.Snapshot;
 import java.util.Optional;
 import java.util.stream.Stream;
+import org.eclipse.jgit.lib.Config;
 
 /**
  * Possible high-level states of the NoteDb migration for changes.
@@ -47,11 +49,22 @@ public enum NotesMigrationState {
 
   NOTE_DB(true, true, true, PrimaryStorage.NOTE_DB, true, true);
 
-  public static Optional<NotesMigrationState> forNotesMigration(NotesMigration migration) {
-    return Stream.of(values()).filter(s -> s.migration().equals(migration)).findFirst();
+  // TODO(dborowitz): Replace with NOTE_DB when FileRepository fuses BatchRefUpdates.
+  public static final NotesMigrationState FINAL = NOTE_DB_UNFUSED;
+
+  public static Optional<NotesMigrationState> forConfig(Config cfg) {
+    return forSnapshot(Snapshot.create(cfg));
   }
 
-  private final NotesMigration migration;
+  public static Optional<NotesMigrationState> forNotesMigration(NotesMigration migration) {
+    return forSnapshot(migration.snapshot());
+  }
+
+  private static Optional<NotesMigrationState> forSnapshot(Snapshot s) {
+    return Stream.of(values()).filter(v -> v.snapshot.equals(s)).findFirst();
+  }
+
+  private final Snapshot snapshot;
 
   NotesMigrationState(
       // Arguments match abstract methods in NotesMigration.
@@ -61,45 +74,28 @@ public enum NotesMigrationState {
       PrimaryStorage changePrimaryStorage,
       boolean disableChangeReviewDb,
       boolean fuseUpdates) {
-    this.migration =
-        new NotesMigration() {
-          @Override
-          public boolean readChanges() {
-            return readChanges;
-          }
-
-          @Override
-          public boolean rawWriteChangesSetting() {
-            return rawWriteChangesSetting;
-          }
-
-          @Override
-          public boolean readChangeSequence() {
-            return readChangeSequence;
-          }
-
-          @Override
-          public PrimaryStorage changePrimaryStorage() {
-            return changePrimaryStorage;
-          }
-
-          @Override
-          public boolean disableChangeReviewDb() {
-            return disableChangeReviewDb;
-          }
-
-          @Override
-          public boolean fuseUpdates() {
-            return fuseUpdates;
-          }
-        };
+    this.snapshot =
+        Snapshot.builder()
+            .setReadChanges(readChanges)
+            .setWriteChanges(rawWriteChangesSetting)
+            .setReadChangeSequence(readChangeSequence)
+            .setChangePrimaryStorage(changePrimaryStorage)
+            .setDisableChangeReviewDb(disableChangeReviewDb)
+            .setFuseUpdates(fuseUpdates)
+            .build();
   }
 
-  public NotesMigration migration() {
-    return migration;
+  public void setConfigValues(Config cfg) {
+    snapshot.setConfigValues(cfg);
   }
 
   public String toText() {
-    return ConfigNotesMigration.toText(migration);
+    Config cfg = new Config();
+    setConfigValues(cfg);
+    return cfg.toText();
+  }
+
+  Snapshot snapshot() {
+    return snapshot;
   }
 }

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

@@ -18,7 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.reviewdb.server.ReviewDbUtil.unwrapDb;
-import static com.google.gerrit.server.notedb.ConfigNotesMigration.SECTION_NOTE_DB;
+import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB;
 import static com.google.gerrit.server.notedb.NotesMigrationState.NOTE_DB_UNFUSED;
 import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_NO_SEQUENCE;
 import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY;
@@ -54,9 +54,8 @@ import com.google.gerrit.server.config.SitePaths;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.LockFailureException;
 import com.google.gerrit.server.git.WorkQueue;
-import com.google.gerrit.server.notedb.ConfigNotesMigration;
+import com.google.gerrit.server.notedb.MutableNotesMigration;
 import com.google.gerrit.server.notedb.NoteDbTable;
-import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.notedb.NotesMigrationState;
 import com.google.gerrit.server.notedb.PrimaryStorageMigrator;
 import com.google.gerrit.server.notedb.RepoSequence;
@@ -114,7 +113,7 @@ public class NoteDbMigrator implements AutoCloseable {
     private final ThreadLocalRequestContext requestContext;
     private final ChangeRebuilder rebuilder;
     private final WorkQueue workQueue;
-    private final NotesMigration globalNotesMigration;
+    private final MutableNotesMigration globalNotesMigration;
     private final PrimaryStorageMigrator primaryStorageMigrator;
 
     private int threads;
@@ -138,7 +137,7 @@ public class NoteDbMigrator implements AutoCloseable {
         InternalUser.Factory userFactory,
         ChangeRebuilder rebuilder,
         WorkQueue workQueue,
-        NotesMigration globalNotesMigration,
+        MutableNotesMigration globalNotesMigration,
         PrimaryStorageMigrator primaryStorageMigrator) {
       this.cfg = cfg;
       this.sitePaths = sitePaths;
@@ -327,7 +326,7 @@ public class NoteDbMigrator implements AutoCloseable {
   private final ThreadLocalRequestContext requestContext;
   private final InternalUser.Factory userFactory;
   private final ChangeRebuilder rebuilder;
-  private final NotesMigration globalNotesMigration;
+  private final MutableNotesMigration globalNotesMigration;
   private final PrimaryStorageMigrator primaryStorageMigrator;
 
   private final ListeningExecutorService executor;
@@ -348,7 +347,7 @@ public class NoteDbMigrator implements AutoCloseable {
       ThreadLocalRequestContext requestContext,
       InternalUser.Factory userFactory,
       ChangeRebuilder rebuilder,
-      NotesMigration globalNotesMigration,
+      MutableNotesMigration globalNotesMigration,
       PrimaryStorageMigrator primaryStorageMigrator,
       ListeningExecutorService executor,
       ImmutableList<Project.NameKey> projects,
@@ -568,7 +567,7 @@ public class NoteDbMigrator implements AutoCloseable {
   private Optional<NotesMigrationState> loadState() throws IOException {
     try {
       gerritConfig.load();
-      return NotesMigrationState.forNotesMigration(new ConfigNotesMigration(gerritConfig));
+      return NotesMigrationState.forConfig(gerritConfig);
     } catch (ConfigInvalidException | IllegalArgumentException e) {
       log.warn("error reading NoteDb migration options from " + gerritConfig.getFile(), e);
       return Optional.empty();
@@ -601,12 +600,12 @@ public class NoteDbMigrator implements AutoCloseable {
                     ? "But found this state:\n" + actualOldState.get().toText()
                     : "But could not parse the current state"));
       }
-      ConfigNotesMigration.setConfigValues(gerritConfig, newState.migration());
+      newState.setConfigValues(gerritConfig);
       additionalUpdates.accept(gerritConfig);
       gerritConfig.save();
 
       // Only set in-memory state once it's been persisted to storage.
-      globalNotesMigration.setFrom(newState.migration());
+      globalNotesMigration.setFrom(newState);
 
       return newState;
     }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java

@@ -56,7 +56,6 @@ import com.google.gerrit.testutil.FakeAccountCache;
 import com.google.gerrit.testutil.GerritBaseTests;
 import com.google.gerrit.testutil.InMemoryRepositoryManager;
 import com.google.gerrit.testutil.TestChanges;
-import com.google.gerrit.testutil.TestNotesMigration;
 import com.google.gerrit.testutil.TestTimeUtil;
 import com.google.gwtorm.server.OrmException;
 import com.google.gwtorm.server.SchemaFactory;
@@ -98,8 +97,6 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
 
   private static final TimeZone TZ = TimeZone.getTimeZone("America/Los_Angeles");
 
-  private static final NotesMigration MIGRATION = new TestNotesMigration().setAllEnabled(true);
-
   protected Account.Id otherUserId;
   protected FakeAccountCache accountCache;
   protected IdentifiedUser changeOwner;
@@ -154,7 +151,6 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
                 install(NoteDbModule.forTest(testConfig));
                 bind(AllUsersName.class).toProvider(AllUsersNameProvider.class);
                 bind(String.class).annotatedWith(GerritServerId.class).toInstance("gerrit");
-                bind(NotesMigration.class).toInstance(MIGRATION);
                 bind(GitRepositoryManager.class).toInstance(repoManager);
                 bind(ProjectCache.class).toProvider(Providers.<ProjectCache>of(null));
                 bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(testConfig);
@@ -177,6 +173,11 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
                 bind(MetricMaker.class).to(DisabledMetricMaker.class);
                 bind(ReviewDb.class).toProvider(Providers.<ReviewDb>of(null));
 
+                MutableNotesMigration migration = MutableNotesMigration.newDisabled();
+                migration.setFrom(NotesMigrationState.FINAL);
+                bind(MutableNotesMigration.class).toInstance(migration);
+                bind(NotesMigration.class).to(MutableNotesMigration.class);
+
                 // Tests don't support ReviewDb at all, but bindings are required via NoteDbModule.
                 bind(new TypeLiteral<SchemaFactory<ReviewDb>>() {})
                     .toInstance(

gerrit-server/src/test/java/com/google/gerrit/server/schema/SchemaUpdaterTest.java

@@ -30,7 +30,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.SitePaths;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.group.SystemGroupBackend;
-import com.google.gerrit.server.notedb.ConfigNotesMigration;
+import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.testutil.InMemoryDatabase;
 import com.google.gerrit.testutil.InMemoryH2Type;
 import com.google.gerrit.testutil.InMemoryRepositoryManager;
@@ -113,7 +113,7 @@ public class SchemaUpdaterTest {
                     bind(DataSourceType.class).to(InMemoryH2Type.class);
 
                     bind(SystemGroupBackend.class);
-                    install(new ConfigNotesMigration.Module());
+                    install(new NotesMigration.Module());
                   }
                 })
             .getInstance(SchemaUpdater.class);

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

@@ -14,6 +14,7 @@
 
 package com.google.gerrit.testutil;
 
+import com.google.gerrit.server.notedb.MutableNotesMigration;
 import org.eclipse.jgit.lib.Config;
 import org.junit.Rule;
 import org.junit.rules.TestRule;
@@ -27,7 +28,7 @@ public class GerritServerTests extends GerritBaseTests {
 
   @ConfigSuite.Name private String configName;
 
-  protected TestNotesMigration notesMigration;
+  protected MutableNotesMigration notesMigration;
 
   @Rule
   public TestRule testRunner =
@@ -49,10 +50,10 @@ public class GerritServerTests extends GerritBaseTests {
       };
 
   public void beforeTest() throws Exception {
-    notesMigration = new TestNotesMigration();
+    notesMigration = NoteDbMode.newNotesMigrationFromEnv();
   }
 
   public void afterTest() {
-    notesMigration.resetFromEnv();
+    NoteDbMode.resetFromEnv(notesMigration);
   }
 }

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

@@ -59,6 +59,7 @@ import com.google.gerrit.server.index.group.GroupSchemaDefinitions;
 import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
 import com.google.gerrit.server.notedb.ChangeBundleReader;
 import com.google.gerrit.server.notedb.GwtormChangeBundleReader;
+import com.google.gerrit.server.notedb.MutableNotesMigration;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.patch.DiffExecutor;
 import com.google.gerrit.server.project.DefaultPermissionBackendModule;
@@ -118,13 +119,13 @@ public class InMemoryModule extends FactoryModule {
   }
 
   private final Config cfg;
-  private final TestNotesMigration notesMigration;
+  private final MutableNotesMigration notesMigration;
 
   public InMemoryModule() {
-    this(newDefaultConfig(), new TestNotesMigration());
+    this(newDefaultConfig(), NoteDbMode.newNotesMigrationFromEnv());
   }
 
-  public InMemoryModule(Config cfg, TestNotesMigration notesMigration) {
+  public InMemoryModule(Config cfg, MutableNotesMigration notesMigration) {
     this.cfg = cfg;
     this.notesMigration = notesMigration;
   }
@@ -176,7 +177,8 @@ public class InMemoryModule extends FactoryModule {
     bind(GitRepositoryManager.class).to(InMemoryRepositoryManager.class);
     bind(InMemoryRepositoryManager.class).in(SINGLETON);
     bind(TrackingFooters.class).toProvider(TrackingFootersProvider.class).in(SINGLETON);
-    bind(NotesMigration.class).toInstance(notesMigration);
+    bind(MutableNotesMigration.class).toInstance(notesMigration);
+    bind(NotesMigration.class).to(MutableNotesMigration.class);
     bind(ListeningExecutorService.class)
         .annotatedWith(ChangeUpdateExecutor.class)
         .toInstance(MoreExecutors.newDirectExecutorService());

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

@@ -32,7 +32,7 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 /** Repository manager that uses in-memory repositories. */
 public class InMemoryRepositoryManager implements GitRepositoryManager {
   public static InMemoryRepository newRepository(Project.NameKey name) {
-    return new Repo(new TestNotesMigration(), name);
+    return new Repo(NoteDbMode.newNotesMigrationFromEnv(), name);
   }
 
   public static class Description extends DfsRepositoryDescription {
@@ -82,7 +82,7 @@ public class InMemoryRepositoryManager implements GitRepositoryManager {
   private final Map<String, Repo> repos;
 
   public InMemoryRepositoryManager() {
-    this(new TestNotesMigration());
+    this(NoteDbMode.newNotesMigrationFromEnv());
   }
 
   @Inject

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

@@ -29,6 +29,7 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 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.MutableNotesMigration;
 import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder;
 import com.google.gwtorm.client.IntKey;
 import com.google.gwtorm.server.OrmException;
@@ -52,7 +53,7 @@ public class NoteDbChecker {
 
   private final Provider<ReviewDb> dbProvider;
   private final GitRepositoryManager repoManager;
-  private final TestNotesMigration notesMigration;
+  private final MutableNotesMigration notesMigration;
   private final ChangeBundleReader bundleReader;
   private final ChangeNotes.Factory notesFactory;
   private final ChangeRebuilder changeRebuilder;
@@ -62,7 +63,7 @@ public class NoteDbChecker {
   NoteDbChecker(
       Provider<ReviewDb> dbProvider,
       GitRepositoryManager repoManager,
-      TestNotesMigration notesMigration,
+      MutableNotesMigration notesMigration,
       ChangeBundleReader bundleReader,
       ChangeNotes.Factory notesFactory,
       ChangeRebuilder changeRebuilder,

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

@@ -18,7 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.common.base.Enums;
 import com.google.common.base.Strings;
-import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.notedb.MutableNotesMigration;
 import com.google.gerrit.server.notedb.NotesMigrationState;
 
 public enum NoteDbMode {
@@ -72,9 +72,19 @@ public enum NoteDbMode {
     return mode;
   }
 
-  final NotesMigration migration;
+  public static MutableNotesMigration newNotesMigrationFromEnv() {
+    MutableNotesMigration m = MutableNotesMigration.newDisabled();
+    resetFromEnv(m);
+    return m;
+  }
+
+  public static void resetFromEnv(MutableNotesMigration migration) {
+    migration.setFrom(get().state);
+  }
+
+  private final NotesMigrationState state;
 
   private NoteDbMode(NotesMigrationState state) {
-    migration = state.migration();
+    this.state = state;
   }
 }

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

@@ -1,128 +0,0 @@
-// Copyright (C) 2016 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.testutil;
-
-import static com.google.common.base.Preconditions.checkNotNull;
-
-import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
-import com.google.gerrit.server.notedb.NotesMigration;
-import com.google.inject.Singleton;
-
-/** {@link NotesMigration} with bits that can be flipped live for testing. */
-@Singleton
-public class TestNotesMigration extends NotesMigration {
-  private volatile boolean readChanges;
-  private volatile boolean writeChanges;
-  private volatile boolean readChangeSequence;
-  private volatile PrimaryStorage changePrimaryStorage = PrimaryStorage.REVIEW_DB;
-  private volatile boolean disableChangeReviewDb;
-  private volatile boolean fuseUpdates;
-  private volatile boolean failOnLoad;
-
-  public TestNotesMigration() {
-    resetFromEnv();
-  }
-
-  @Override
-  public boolean readChanges() {
-    return readChanges;
-  }
-
-  @Override
-  public boolean readChangeSequence() {
-    return readChangeSequence;
-  }
-
-  @Override
-  public PrimaryStorage changePrimaryStorage() {
-    return changePrimaryStorage;
-  }
-
-  @Override
-  public boolean disableChangeReviewDb() {
-    return disableChangeReviewDb;
-  }
-
-  @Override
-  public boolean fuseUpdates() {
-    return fuseUpdates;
-  }
-
-  // Increase visbility from superclass, as tests may want to check whether
-  // NoteDb data is written in specific migration scenarios.
-  @Override
-  public boolean rawWriteChangesSetting() {
-    return writeChanges;
-  }
-
-  @Override
-  public boolean failOnLoad() {
-    return failOnLoad;
-  }
-
-  public TestNotesMigration setReadChanges(boolean readChanges) {
-    this.readChanges = readChanges;
-    return this;
-  }
-
-  public TestNotesMigration setWriteChanges(boolean writeChanges) {
-    this.writeChanges = writeChanges;
-    return this;
-  }
-
-  public TestNotesMigration setReadChangeSequence(boolean readChangeSequence) {
-    this.readChangeSequence = readChangeSequence;
-    return this;
-  }
-
-  public TestNotesMigration setChangePrimaryStorage(PrimaryStorage changePrimaryStorage) {
-    this.changePrimaryStorage = checkNotNull(changePrimaryStorage);
-    return this;
-  }
-
-  public TestNotesMigration setDisableChangeReviewDb(boolean disableChangeReviewDb) {
-    this.disableChangeReviewDb = disableChangeReviewDb;
-    return this;
-  }
-
-  public TestNotesMigration setFuseUpdates(boolean fuseUpdates) {
-    this.fuseUpdates = fuseUpdates;
-    return this;
-  }
-
-  public TestNotesMigration setFailOnLoad(boolean failOnLoad) {
-    this.failOnLoad = failOnLoad;
-    return this;
-  }
-
-  public TestNotesMigration setAllEnabled(boolean enabled) {
-    return setReadChanges(enabled).setWriteChanges(enabled);
-  }
-
-  public TestNotesMigration resetFromEnv() {
-    return setFrom(NoteDbMode.get().migration);
-  }
-
-  @Override
-  public TestNotesMigration setFrom(NotesMigration other) {
-    setWriteChanges(other.rawWriteChangesSetting());
-    setReadChanges(other.readChanges());
-    setReadChangeSequence(other.readChangeSequence());
-    setChangePrimaryStorage(other.changePrimaryStorage());
-    setDisableChangeReviewDb(other.disableChangeReviewDb());
-    setFuseUpdates(other.fuseUpdates());
-    return this;
-  }
-}

gerrit-war/src/main/java/com/google/gerrit/httpd/WebAppInitializer.java

@@ -58,7 +58,7 @@ import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
 import com.google.gerrit.server.mail.receive.MailReceiver;
 import com.google.gerrit.server.mail.send.SmtpEmailSender;
 import com.google.gerrit.server.mime.MimeUtil2Module;
-import com.google.gerrit.server.notedb.ConfigNotesMigration;
+import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.server.patch.DiffExecutorModule;
 import com.google.gerrit.server.plugins.PluginGuiceEnvironment;
 import com.google.gerrit.server.plugins.PluginModule;
@@ -292,7 +292,7 @@ public class WebAppInitializer extends GuiceServletContextListener implements Fi
       modules.add(new GerritServerConfigModule());
     }
     modules.add(new DatabaseModule());
-    modules.add(new ConfigNotesMigration.Module());
+    modules.add(new NotesMigration.Module());
     modules.add(new DropWizardMetricMaker.ApiModule());
     return Guice.createInjector(PRODUCTION, modules);
   }