Merge changes from topics "remove-notesmigration", "batch-update-factory"
* changes:
NotesMigration: Make config key names private
NoteDbSchemaUpdater: Inline NoteDb config option names
Remove InitNoteDb
NotesMigrationSchemaFactory: Remove NotesMigration check
NotesMigration: Remove readChangeSequence() method
NotesMigration: Remove unused methods
Replace configurable NotesMigration with a stub implementation
Remove unused MutableNotesMigration#failOnLoadForTest
Remove NotesMigration from AbstractChangeNotes.Args
ChangeReviewersByEmailIT: Use enableDb/disableDb helper methods
GetServerInfo: Always report NoteDb as enabled
NoteDbMetrics: Remove unused auto-rebuilding metrics
BatchUpdate: Remove non-assisted Factory
BatchUpdate: Inline newContext method
BatchUpdate: Make remaining protected methods private
Merge NoteDbBatchUpdate into BatchUpdate
* submodules:
* Update plugins/delete-project from branch 'master'
to 5f3fe725b6f943f9acf63270cf8a432f9e7fd97a
- Remove DatabaseDeleteHandler since ReviewDb is gone
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I3e362f58b6deb9878e76dcfb225fef086753e4e9
diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt
index db6f883..ced4609 100644
--- a/Documentation/metrics.txt
+++ b/Documentation/metrics.txt
@@ -140,9 +140,6 @@
* `notedb/stage_update_latency`: Latency for staging updates to NoteDb by table.
* `notedb/read_latency`: NoteDb read latency by table.
* `notedb/parse_latency`: NoteDb parse latency by table.
-* `notedb/auto_rebuild_latency`: NoteDb auto-rebuilding latency by table.
-* `notedb/auto_rebuild_failure_count`: NoteDb auto-rebuilding attempts that
-failed by table.
* `notedb/external_id_update_count`: Total number of external ID updates.
* `notedb/read_all_external_ids_latency`: Latency for reading all
external ID's from NoteDb.
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 17588a1..6e3b100 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -112,9 +112,10 @@
import com.google.gerrit.server.index.change.ChangeIndexCollection;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.index.group.GroupIndexer;
+import com.google.gerrit.server.notedb.AbstractChangeNotes;
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.notedb.NotesMigration;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.testing.Util;
@@ -126,7 +127,6 @@
import com.google.gerrit.testing.FakeEmailSender;
import com.google.gerrit.testing.FakeEmailSender.Message;
import com.google.gerrit.testing.FakeGroupAuditService;
-import com.google.gerrit.testing.NoteDbMode;
import com.google.gerrit.testing.SshMode;
import com.google.gson.Gson;
import com.google.gwtorm.server.OrmException;
@@ -261,7 +261,7 @@
@Inject protected PluginConfigFactory pluginConfig;
@Inject protected Revisions revisions;
@Inject protected SystemGroupBackend systemGroupBackend;
- @Inject protected MutableNotesMigration notesMigration;
+ @Inject protected NotesMigration notesMigration;
@Inject protected ChangeNotes.Factory notesFactory;
@Inject protected BatchAbandon batchAbandon;
@Inject protected TestSshKeys sshKeys;
@@ -293,6 +293,7 @@
@Inject private AccountIndexer accountIndexer;
@Inject private Groups groups;
@Inject private GroupIndexer groupIndexer;
+ @Inject private AbstractChangeNotes.Args changeNotesArgs;
private ProjectResetter resetter;
private List<Repository> toClose;
@@ -596,7 +597,6 @@
server.close();
server = null;
}
- NoteDbMode.resetFromEnv(notesMigration);
}
protected void closeSsh() {
@@ -837,12 +837,12 @@
}
protected Context disableDb() {
- notesMigration.setFailOnLoadForTest(true);
+ changeNotesArgs.failOnLoadForTest.set(true);
return atrScope.disableDb();
}
protected void enableDb(Context preDisableContext) {
- notesMigration.setFailOnLoadForTest(false);
+ changeNotesArgs.failOnLoadForTest.set(false);
atrScope.set(preDisableContext);
}
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java
index 4788ab7..077dd65 100644
--- a/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -45,7 +45,6 @@
import com.google.gerrit.testing.FakeEmailSender;
import com.google.gerrit.testing.FakeGroupAuditService;
import com.google.gerrit.testing.InMemoryRepositoryManager;
-import com.google.gerrit.testing.NoteDbMode;
import com.google.gerrit.testing.SshMode;
import com.google.inject.AbstractModule;
import com.google.inject.Injector;
@@ -482,8 +481,6 @@
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 {
diff --git a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
index e15d162..9c4b6b7 100644
--- a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
+++ b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
@@ -28,7 +28,6 @@
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.config.TrackingFootersProvider;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.schema.NotesMigrationSchemaFactory;
import com.google.gerrit.server.schema.ReviewDbFactory;
import com.google.gerrit.server.schema.ReviewDbSchemaModule;
@@ -75,7 +74,6 @@
bind(MetricMaker.class).to(DisabledMetricMaker.class);
- install(new NotesMigration.Module());
TypeLiteral<SchemaFactory<ReviewDb>> schemaFactory =
new TypeLiteral<SchemaFactory<ReviewDb>>() {};
bind(schemaFactory).to(NotesMigrationSchemaFactory.class);
diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
index 8b113b2..9f6049e 100644
--- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java
+++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
@@ -78,7 +78,6 @@
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.NotesMigration;
import com.google.gerrit.server.patch.DiffExecutorModule;
import com.google.gerrit.server.permissions.DefaultPermissionBackendModule;
import com.google.gerrit.server.plugins.PluginGuiceEnvironment;
@@ -277,7 +276,6 @@
modules.add(new GerritServerConfigModule());
}
modules.add(new DatabaseModule());
- modules.add(new NotesMigration.Module());
modules.add(new DropWizardMetricMaker.ApiModule());
return Guice.createInjector(PRODUCTION, modules);
}
diff --git a/java/com/google/gerrit/pgm/init/InitModule.java b/java/com/google/gerrit/pgm/init/InitModule.java
index 75145de..f2fc001 100644
--- a/java/com/google/gerrit/pgm/init/InitModule.java
+++ b/java/com/google/gerrit/pgm/init/InitModule.java
@@ -42,7 +42,6 @@
// Steps are executed in the order listed here.
//
step().to(InitGitManager.class);
- step().to(InitNoteDb.class);
step().to(InitLogging.class);
step().to(InitIndex.class);
step().to(InitAuth.class);
diff --git a/java/com/google/gerrit/pgm/init/InitNoteDb.java b/java/com/google/gerrit/pgm/init/InitNoteDb.java
deleted file mode 100644
index dc52868..0000000
--- a/java/com/google/gerrit/pgm/init/InitNoteDb.java
+++ /dev/null
@@ -1,50 +0,0 @@
-// Copyright (C) 2018 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.pgm.init;
-
-import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
-import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB;
-
-import com.google.gerrit.pgm.init.api.InitStep;
-import com.google.gerrit.pgm.init.api.Section;
-import com.google.gerrit.server.notedb.NotesMigrationState;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
-import org.eclipse.jgit.lib.Config;
-
-/** Initialize the NoteDb in gerrit site. */
-@Singleton
-class InitNoteDb implements InitStep {
-
- private final Section noteDbChanges;
-
- @Inject
- InitNoteDb(Section.Factory sections) {
- this.noteDbChanges = sections.get(SECTION_NOTE_DB, CHANGES.key());
- }
-
- @Override
- public void run() {
- initNoteDb();
- }
-
- private void initNoteDb() {
- 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));
- }
- }
-}
diff --git a/java/com/google/gerrit/pgm/util/SiteProgram.java b/java/com/google/gerrit/pgm/util/SiteProgram.java
index e273c02..a889277f 100644
--- a/java/com/google/gerrit/pgm/util/SiteProgram.java
+++ b/java/com/google/gerrit/pgm/util/SiteProgram.java
@@ -25,7 +25,6 @@
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.NotesMigration;
import com.google.gerrit.server.schema.DatabaseModule;
import com.google.gerrit.server.schema.ReviewDbSchemaModule;
import com.google.gerrit.server.securestore.SecureStoreClassName;
@@ -123,7 +122,6 @@
modules.add(new DatabaseModule());
modules.add(new ReviewDbSchemaModule());
modules.add(cfgInjector.getInstance(GitRepositoryManagerModule.class));
- modules.add(new NotesMigration.Module());
try {
return Guice.createInjector(PRODUCTION, modules);
diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
index 32d086c..d3ab0e0 100644
--- a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.notedb;
-import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
import static java.util.Objects.requireNonNull;
@@ -34,6 +33,7 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.concurrent.atomic.AtomicBoolean;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -44,8 +44,10 @@
@VisibleForTesting
@Singleton
public static class Args {
+ // TODO(dborowitz): Some less smelly way of disabling NoteDb in tests.
+ public final AtomicBoolean failOnLoadForTest;
+
final GitRepositoryManager repoManager;
- final NotesMigration migration;
final AllUsersName allUsers;
final ChangeNoteJson changeNoteJson;
final LegacyChangeNoteRead legacyChangeNoteRead;
@@ -60,15 +62,14 @@
@Inject
Args(
GitRepositoryManager repoManager,
- NotesMigration migration,
AllUsersName allUsers,
ChangeNoteJson changeNoteJson,
LegacyChangeNoteRead legacyChangeNoteRead,
NoteDbMetrics metrics,
Provider<ReviewDb> db,
Provider<ChangeNotesCache> cache) {
+ this.failOnLoadForTest = new AtomicBoolean();
this.repoManager = repoManager;
- this.migration = migration;
this.allUsers = allUsers;
this.legacyChangeNoteRead = legacyChangeNoteRead;
this.changeNoteJson = changeNoteJson;
@@ -132,8 +133,7 @@
return self();
}
- checkState(args.migration.readChanges(), "NoteDb is required to read changes");
- if (args.migration.failOnLoadForTest()) {
+ if (args.failOnLoadForTest.get()) {
throw new OrmException("Reading from NoteDb is disabled");
}
try (Timer1.Context timer = args.metrics.readLatency.start(CHANGES);
@@ -181,8 +181,6 @@
public ObjectId loadRevision() throws OrmException {
if (loaded) {
return getRevision();
- } else if (!args.migration.readChanges()) {
- return null;
}
try (Repository repo = args.repoManager.openRepository(getProjectName())) {
Ref ref = repo.getRefDatabase().exactRef(getRefName());
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index e5e0d51..a3fa45f 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -120,9 +120,6 @@
throws OrmException {
Change change = readOneReviewDbChange(db, changeId);
if (change == null) {
- if (!args.migration.readChanges()) {
- throw new NoSuchChangeException(changeId);
- }
// Change isn't in ReviewDb, but its primary storage might be in NoteDb.
// Prepopulate the change exists with proper noteDbState field.
change = newNoteDbOnlyChange(project, changeId);
@@ -159,10 +156,7 @@
Change change = readOneReviewDbChange(db, changeId);
if (change == null) {
- if (args.migration.readChanges()) {
- return newNoteDbOnlyChange(project, changeId);
- }
- throw new NoSuchChangeException(changeId);
+ return newNoteDbOnlyChange(project, changeId);
}
checkArgument(
change.getProject().equals(project),
@@ -198,33 +192,15 @@
return new ChangeNotes(args, change, true, refs).load();
}
- // TODO(ekempin): Remove when database backend is deleted
- /**
- * Instantiate ChangeNotes for a change that has been loaded by a batch read from the database.
- */
- private ChangeNotes createFromChangeOnlyWhenNoteDbDisabled(Change change) throws OrmException {
- checkState(
- !args.migration.readChanges(),
- "do not call createFromChangeWhenNoteDbDisabled when NoteDb is enabled");
- return new ChangeNotes(args, change).load();
- }
-
public List<ChangeNotes> create(ReviewDb db, Collection<Change.Id> changeIds)
throws OrmException {
List<ChangeNotes> notes = new ArrayList<>();
- if (args.migration.readChanges()) {
- for (Change.Id changeId : changeIds) {
- try {
- notes.add(createChecked(changeId));
- } catch (NoSuchChangeException e) {
- // Ignore missing changes to match Access#get(Iterable) behavior.
- }
+ for (Change.Id changeId : changeIds) {
+ try {
+ notes.add(createChecked(changeId));
+ } catch (NoSuchChangeException e) {
+ // Ignore missing changes to match Access#get(Iterable) behavior.
}
- return notes;
- }
-
- for (Change c : ReviewDbUtil.unwrapDb(db).changes().get(changeIds)) {
- notes.add(createFromChangeOnlyWhenNoteDbDisabled(c));
}
return notes;
}
@@ -236,28 +212,16 @@
Predicate<ChangeNotes> predicate)
throws OrmException {
List<ChangeNotes> notes = new ArrayList<>();
- if (args.migration.readChanges()) {
- for (Change.Id cid : changeIds) {
- try {
- ChangeNotes cn = create(db, project, cid);
- if (cn.getChange() != null && predicate.test(cn)) {
- notes.add(cn);
- }
- } catch (NoSuchChangeException e) {
- // Match ReviewDb behavior, returning not found; maybe the caller learned about it from
- // a dangling patch set ref or something.
- continue;
- }
- }
- return notes;
- }
-
- for (Change c : ReviewDbUtil.unwrapDb(db).changes().get(changeIds)) {
- if (c != null && project.equals(c.getDest().getParentKey())) {
- ChangeNotes cn = createFromChangeOnlyWhenNoteDbDisabled(c);
- if (predicate.test(cn)) {
+ for (Change.Id cid : changeIds) {
+ try {
+ ChangeNotes cn = create(db, project, cid);
+ if (cn.getChange() != null && predicate.test(cn)) {
notes.add(cn);
}
+ } catch (NoSuchChangeException e) {
+ // Match ReviewDb behavior, returning not found; maybe the caller learned about it from
+ // a dangling patch set ref or something.
+ continue;
}
}
return notes;
@@ -267,22 +231,13 @@
ReviewDb db, Predicate<ChangeNotes> predicate) throws IOException, OrmException {
ListMultimap<Project.NameKey, ChangeNotes> m =
MultimapBuilder.hashKeys().arrayListValues().build();
- if (args.migration.readChanges()) {
- for (Project.NameKey project : projectCache.all()) {
- try (Repository repo = args.repoManager.openRepository(project)) {
- scan(repo, project)
- .filter(r -> !r.error().isPresent())
- .map(ChangeNotesResult::notes)
- .filter(predicate)
- .forEach(n -> m.put(n.getProjectName(), n));
- }
- }
- } else {
- for (Change change : ReviewDbUtil.unwrapDb(db).changes().all()) {
- ChangeNotes notes = createFromChangeOnlyWhenNoteDbDisabled(change);
- if (predicate.test(notes)) {
- m.put(change.getProject(), notes);
- }
+ for (Project.NameKey project : projectCache.all()) {
+ try (Repository repo = args.repoManager.openRepository(project)) {
+ scan(repo, project)
+ .filter(r -> !r.error().isPresent())
+ .map(ChangeNotesResult::notes)
+ .filter(predicate)
+ .forEach(n -> m.put(n.getProjectName(), n));
}
}
return ImmutableListMultimap.copyOf(m);
@@ -302,7 +257,7 @@
return null;
}
- // TODO(dborowitz): See discussion in NoteDbBatchUpdate#newChangeContext.
+ // TODO(dborowitz): See discussion in BatchUpdate#newChangeContext.
Change change = ChangeNotes.Factory.newNoteDbOnlyChange(project, id);
logger.atFine().log("adding change %s found in project %s", id, project);
@@ -594,9 +549,7 @@
protected void onLoad(LoadHandle handle) throws NoSuchChangeException, IOException {
ObjectId rev = handle.id();
if (rev == null) {
- if (args.migration.readChanges()
- && PrimaryStorage.of(change) == PrimaryStorage.NOTE_DB
- && shouldExist) {
+ if (PrimaryStorage.of(change) == PrimaryStorage.NOTE_DB && shouldExist) {
throw new NoSuchChangeException(getChangeId());
}
loadDefaults();
diff --git a/java/com/google/gerrit/server/notedb/MutableNotesMigration.java b/java/com/google/gerrit/server/notedb/MutableNotesMigration.java
deleted file mode 100644
index eb41cbc..0000000
--- a/java/com/google/gerrit/server/notedb/MutableNotesMigration.java
+++ /dev/null
@@ -1,99 +0,0 @@
-// 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.common.annotations.VisibleForTesting;
-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 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 tests.
- *
- * <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.
- */
- @VisibleForTesting
- public MutableNotesMigration setFrom(NotesMigrationState state) {
- snapshot.set(state.snapshot());
- return this;
- }
-
- private MutableNotesMigration set(Function<Snapshot.Builder, Snapshot.Builder> f) {
- snapshot.updateAndGet(s -> f.apply(s.toBuilder()).build());
- return this;
- }
-}
diff --git a/java/com/google/gerrit/server/notedb/NoteDbMetrics.java b/java/com/google/gerrit/server/notedb/NoteDbMetrics.java
index be06d11..61f475f 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbMetrics.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbMetrics.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.notedb;
-import com.google.gerrit.metrics.Counter1;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
import com.google.gerrit.metrics.Field;
@@ -44,16 +43,6 @@
*/
final Timer1<NoteDbTable> parseLatency;
- /**
- * Latency due to auto-rebuilding entities when out of date.
- *
- * <p>Excludes latency from reading ref to check whether the entity is up to date.
- */
- final Timer1<NoteDbTable> autoRebuildLatency;
-
- /** Count of auto-rebuild attempts that failed. */
- final Counter1<NoteDbTable> autoRebuildFailureCount;
-
@Inject
NoteDbMetrics(MetricMaker metrics) {
Field<NoteDbTable> view = Field.ofEnum(NoteDbTable.class, "table");
@@ -89,19 +78,5 @@
.setCumulative()
.setUnit(Units.MICROSECONDS),
view);
-
- autoRebuildLatency =
- metrics.newTimer(
- "notedb/auto_rebuild_latency",
- new Description("NoteDb auto-rebuilding latency by table")
- .setCumulative()
- .setUnit(Units.MILLISECONDS),
- view);
-
- autoRebuildFailureCount =
- metrics.newCounter(
- "notedb/auto_rebuild_failure_count",
- new Description("NoteDb auto-rebuilding attempts that failed by table").setCumulative(),
- view);
}
}
diff --git a/java/com/google/gerrit/server/notedb/NotesMigration.java b/java/com/google/gerrit/server/notedb/NotesMigration.java
index 28754a6..1f9b7dc 100644
--- a/java/com/google/gerrit/server/notedb/NotesMigration.java
+++ b/java/com/google/gerrit/server/notedb/NotesMigration.java
@@ -14,129 +14,22 @@
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 com.google.inject.AbstractModule;
-import java.util.concurrent.atomic.AtomicReference;
-import org.eclipse.jgit.lib.Config;
+import com.google.inject.Singleton;
+import java.util.Objects;
/**
* Current low-level settings of the NoteDb migration for changes.
*
- * <p>This class only describes the migration state of the {@link
- * com.google.gerrit.reviewdb.client.Change Change} entity group, since it is possible for a given
- * site to be in different states of the Change NoteDb migration process while staying at the same
- * ReviewDb schema version. It does <em>not</em> describe the migration state of non-Change tables;
- * those are automatically migrated using the ReviewDb schema migration process, so the NoteDb
- * migration state at a given ReviewDb schema cannot vary.
- *
- * <p>In many places, core Gerrit code should not directly care about the NoteDb migration state,
- * and should prefer high-level APIs like {@link com.google.gerrit.server.ApprovalsUtil
- * ApprovalsUtil} that don't require callers to inspect the migration state. The
- * <em>implementation</em> of those utilities does care about the state, and should query the {@code
- * NotesMigration} for the properties of the migration, for example, {@link #changePrimaryStorage()
- * where new changes should be stored}.
- *
- * <p>Core Gerrit code is mostly interested in one facet of the migration at a time (reading or
- * writing, say), but not all combinations of return values are supported or even make sense.
- *
- * <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.
- *
- * <p><strong>Note:</strong> Callers should not assume the values returned by {@code
- * NotesMigration}'s methods will not change in a running server.
+ * <p>This class is a stub and will be removed soon; NoteDb is the only mode.
*/
-public abstract class NotesMigration {
+@Singleton
+public class NotesMigration {
public static final String SECTION_NOTE_DB = "noteDb";
- public static final String READ = "read";
- public static final String WRITE = "write";
- public static final String DISABLE_REVIEW_DB = "disableReviewDb";
- public static final String PRIMARY_STORAGE = "primaryStorage";
- public static final String SEQUENCE = "sequence";
-
- 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))
- .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 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());
- }
-
- @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 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;
+ private static final String READ = "read";
+ private static final String WRITE = "write";
+ private static final String DISABLE_REVIEW_DB = "disableReviewDb";
+ private static final String PRIMARY_STORAGE = "primaryStorage";
+ private static final String SEQUENCE = "sequence";
/**
* Read changes from NoteDb.
@@ -149,40 +42,7 @@
* attempts to write will generate an error.
*/
public final boolean readChanges() {
- return snapshot.get().readChanges();
- }
-
- /**
- * Write changes to NoteDb.
- *
- * <p>This method is awkwardly named because you should be using either {@link
- * #commitChangeWrites()} or {@link #failChangeWrites()} instead.
- *
- * <p>Updates to change data are written to NoteDb refs, but ReviewDb is still the source of
- * truth. Change data will not be written unless the NoteDb refs are already up to date, and the
- * write path will attempt to rebuild the change if not.
- *
- * <p>If false, the behavior when attempting to write depends on {@code readChanges()}. If {@code
- * readChanges() = false}, writes to NoteDb are simply ignored; if {@code true}, any attempts to
- * write will generate an error.
- */
- public final boolean rawWriteChangesSetting() {
- return snapshot.get().writeChanges();
- }
-
- /**
- * Read sequential change ID numbers from NoteDb.
- *
- * <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 final boolean readChangeSequence() {
- return snapshot.get().readChangeSequence();
- }
-
- /** @return default primary storage for new changes. */
- public final PrimaryStorage changePrimaryStorage() {
- return snapshot.get().changePrimaryStorage();
+ return true;
}
/**
@@ -193,56 +53,26 @@
* Changes table.
*/
public final boolean disableChangeReviewDb() {
- return snapshot.get().disableChangeReviewDb();
- }
-
- /**
- * Whether to fail when reading any data from NoteDb.
- *
- * <p>Used in conjunction with {@link #readChanges()} for tests.
- */
- public boolean failOnLoadForTest() {
- return snapshot.get().failOnLoadForTest();
+ return true;
}
public final boolean commitChangeWrites() {
- // It may seem odd that readChanges() without writeChanges() means we should
- // attempt to commit writes. However, this method is used by callers to know
- // whether or not they should short-circuit and skip attempting to read or
- // write NoteDb refs.
- //
- // It is possible for commitChangeWrites() to return true and
- // failChangeWrites() to also return true, causing an error later in the
- // same codepath. This specific condition is used by the auto-rebuilding
- // path to rebuild a change and stage the results, but not commit them due
- // to failChangeWrites().
- return rawWriteChangesSetting() || readChanges();
+ return true;
}
public final boolean failChangeWrites() {
- return !rawWriteChangesSetting() && readChanges();
- }
-
- public final void setConfigValues(Config cfg) {
- snapshot.get().setConfigValues(cfg);
+ return false;
}
@Override
public final boolean equals(Object o) {
- return o instanceof NotesMigration
- && snapshot.get().equals(((NotesMigration) o).snapshot.get());
+ return o instanceof NotesMigration;
}
@Override
public final int hashCode() {
- return snapshot.get().hashCode();
+ return Objects.hash();
}
- protected NotesMigration(Snapshot snapshot) {
- this.snapshot = new AtomicReference<>(snapshot);
- }
-
- final Snapshot snapshot() {
- return snapshot.get();
- }
+ public NotesMigration() {}
}
diff --git a/java/com/google/gerrit/server/notedb/NotesMigrationState.java b/java/com/google/gerrit/server/notedb/NotesMigrationState.java
deleted file mode 100644
index c682aed..0000000
--- a/java/com/google/gerrit/server/notedb/NotesMigrationState.java
+++ /dev/null
@@ -1,92 +0,0 @@
-// 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.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.
- *
- * <p>This class describes the series of states required to migrate a site from ReviewDb-only to
- * NoteDb-only. This process has several steps, and covers only a small subset of the theoretically
- * possible combinations of {@link NotesMigration} return values.
- *
- * <p>These states are ordered: a one-way migration from ReviewDb to NoteDb will pass through states
- * in the order in which they are defined.
- */
-public enum NotesMigrationState {
- REVIEW_DB(false, false, false, PrimaryStorage.REVIEW_DB, false),
-
- WRITE(false, true, false, PrimaryStorage.REVIEW_DB, false),
-
- READ_WRITE_NO_SEQUENCE(true, true, false, PrimaryStorage.REVIEW_DB, false),
-
- READ_WRITE_WITH_SEQUENCE_REVIEW_DB_PRIMARY(true, true, true, PrimaryStorage.REVIEW_DB, false),
-
- READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY(true, true, true, PrimaryStorage.NOTE_DB, false),
-
- NOTE_DB(true, true, true, PrimaryStorage.NOTE_DB, true);
-
- public static final NotesMigrationState FINAL = NOTE_DB;
-
- public static Optional<NotesMigrationState> forConfig(Config cfg) {
- return forSnapshot(Snapshot.create(cfg));
- }
-
- 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.
- boolean readChanges,
- boolean rawWriteChangesSetting,
- boolean readChangeSequence,
- PrimaryStorage changePrimaryStorage,
- boolean disableChangeReviewDb) {
- this.snapshot =
- Snapshot.builder()
- .setReadChanges(readChanges)
- .setWriteChanges(rawWriteChangesSetting)
- .setReadChangeSequence(readChangeSequence)
- .setChangePrimaryStorage(changePrimaryStorage)
- .setDisableChangeReviewDb(disableChangeReviewDb)
- .build();
- }
-
- public void setConfigValues(Config cfg) {
- snapshot.setConfigValues(cfg);
- }
-
- public String toText() {
- Config cfg = new Config();
- setConfigValues(cfg);
- return cfg.toText();
- }
-
- Snapshot snapshot() {
- return snapshot;
- }
-}
diff --git a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
index 16ab812..96b9519 100644
--- a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
+++ b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
@@ -151,7 +151,7 @@
// Currently there's no way to let some updates succeed even if others fail. Even if there were,
// all updates from this operation only happen in All-Users and thus are fully atomic, so
// allowing partial failure would have little value.
- batchUpdateFactory.execute(updates.values(), BatchUpdateListener.NONE, false);
+ BatchUpdate.execute(updates.values(), BatchUpdateListener.NONE, false);
return ops.stream().map(Op::getResult).filter(Objects::nonNull).collect(toImmutableList());
}
diff --git a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
index 98ef220..c03c4c5 100644
--- a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
+++ b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
@@ -53,7 +53,6 @@
import com.google.gerrit.server.documentation.QueryDocumentationExecutor;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndexCollection;
-import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.plugincontext.PluginItemContext;
import com.google.gerrit.server.plugincontext.PluginMapContext;
@@ -91,7 +90,6 @@
private final PluginItemContext<AvatarProvider> avatar;
private final boolean enableSignedPush;
private final QueryDocumentationExecutor docSearcher;
- private final NotesMigration migration;
private final ProjectCache projectCache;
private final AgreementJson agreementJson;
private final ChangeIndexCollection indexes;
@@ -114,7 +112,6 @@
PluginItemContext<AvatarProvider> avatar,
@EnableSignedPush boolean enableSignedPush,
QueryDocumentationExecutor docSearcher,
- NotesMigration migration,
ProjectCache projectCache,
AgreementJson agreementJson,
ChangeIndexCollection indexes,
@@ -134,7 +131,6 @@
this.avatar = avatar;
this.enableSignedPush = enableSignedPush;
this.docSearcher = docSearcher;
- this.migration = migration;
this.projectCache = projectCache;
this.agreementJson = agreementJson;
this.indexes = indexes;
@@ -149,7 +145,7 @@
info.change = getChangeInfo();
info.download = getDownloadInfo();
info.gerrit = getGerritInfo();
- info.noteDbEnabled = toBoolean(isNoteDbEnabled());
+ info.noteDbEnabled = true;
info.plugin = getPluginInfo();
info.defaultTheme = getDefaultTheme();
info.sshd = getSshdInfo();
@@ -313,10 +309,6 @@
return CharMatcher.is('/').trimTrailingFrom(docUrl) + '/';
}
- private boolean isNoteDbEnabled() {
- return migration.readChanges();
- }
-
private PluginConfigInfo getPluginInfo() {
PluginConfigInfo info = new PluginConfigInfo();
info.hasAvatars = toBoolean(avatar.hasImplementation());
diff --git a/java/com/google/gerrit/server/schema/AllProjectsCreator.java b/java/com/google/gerrit/server/schema/AllProjectsCreator.java
index 14da9eb..78324fa 100644
--- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java
+++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java
@@ -264,8 +264,7 @@
}
private void initSequences(Repository git, BatchRefUpdate bru) throws IOException {
- if (notesMigration.readChangeSequence()
- && git.exactRef(REFS_SEQUENCES + Sequences.NAME_CHANGES) == null) {
+ if (git.exactRef(REFS_SEQUENCES + Sequences.NAME_CHANGES) == null) {
// Can't easily reuse the inserter from MetaDataUpdate, but this shouldn't slow down site
// initialization unduly.
try (ObjectInserter ins = git.newObjectInserter()) {
diff --git a/java/com/google/gerrit/server/schema/NoteDbSchemaUpdater.java b/java/com/google/gerrit/server/schema/NoteDbSchemaUpdater.java
index 602b639..73deb3e 100644
--- a/java/com/google/gerrit/server/schema/NoteDbSchemaUpdater.java
+++ b/java/com/google/gerrit/server/schema/NoteDbSchemaUpdater.java
@@ -15,12 +15,6 @@
package com.google.gerrit.server.schema;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
-import static com.google.gerrit.server.notedb.NotesMigration.DISABLE_REVIEW_DB;
-import static com.google.gerrit.server.notedb.NotesMigration.PRIMARY_STORAGE;
-import static com.google.gerrit.server.notedb.NotesMigration.READ;
-import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB;
-import static com.google.gerrit.server.notedb.NotesMigration.WRITE;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
@@ -144,11 +138,11 @@
// * Completed the change migration to NoteDB.
// * Ran schema upgrades from a 2.16 final release.
- if (!cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), WRITE, false)
- || !cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), READ, false)
- || cfg.getEnum(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB)
+ if (!cfg.getBoolean("noteDb", "changes", "write", false)
+ || !cfg.getBoolean("noteDb", "changes", "read", false)
+ || cfg.getEnum("noteDb", "changes", "primaryStorage", PrimaryStorage.REVIEW_DB)
!= PrimaryStorage.NOTE_DB
- || !cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, false)) {
+ || !cfg.getBoolean("noteDb", "changes", "disableReviewDb", false)) {
throw new OrmException(
"You appear to be upgrading from a 2.x site, but the NoteDb change migration was"
+ " not completed. See documentation:\n"
diff --git a/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java b/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java
index c533619..63b3eaa 100644
--- a/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java
+++ b/java/com/google/gerrit/server/schema/NotesMigrationSchemaFactory.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.schema;
-import static com.google.common.base.Preconditions.checkState;
-
import com.google.gerrit.reviewdb.server.DisallowedReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.notedb.NotesMigration;
@@ -60,8 +58,6 @@
// do nothing. This implementation is not a public class and callers couldn't do anything
// useful with it even if it were.
- // First create the underlying stub.
- checkState(migration.readChanges() && migration.disableChangeReviewDb());
// Disable writes to change tables in ReviewDb (ReviewDb access for changes are No-Ops); all
// other table accesses throw runtime exceptions.
ReviewDb db = new NoChangesReviewDb();
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 81e2661..13affc3 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -607,7 +607,7 @@
SubmoduleOp submoduleOp = subOpFactory.create(branches, orm);
List<SubmitStrategy> strategies = getSubmitStrategies(toSubmit, submoduleOp, dryrun);
this.allProjects = submoduleOp.getProjectsInOrder();
- batchUpdateFactory.execute(
+ BatchUpdate.execute(
orm.batchUpdates(allProjects),
new SubmitStrategyListener(submitInput, strategies, commitStatus),
dryrun);
diff --git a/java/com/google/gerrit/server/submit/SubmoduleOp.java b/java/com/google/gerrit/server/submit/SubmoduleOp.java
index 50be62a..3fefed3 100644
--- a/java/com/google/gerrit/server/submit/SubmoduleOp.java
+++ b/java/com/google/gerrit/server/submit/SubmoduleOp.java
@@ -101,39 +101,29 @@
private final Provider<PersonIdent> serverIdent;
private final Config cfg;
private final ProjectCache projectCache;
- private final BatchUpdate.Factory batchUpdateFactory;
@Inject
Factory(
GitModules.Factory gitmodulesFactory,
@GerritPersonIdent Provider<PersonIdent> serverIdent,
@GerritServerConfig Config cfg,
- ProjectCache projectCache,
- BatchUpdate.Factory batchUpdateFactory) {
+ ProjectCache projectCache) {
this.gitmodulesFactory = gitmodulesFactory;
this.serverIdent = serverIdent;
this.cfg = cfg;
this.projectCache = projectCache;
- this.batchUpdateFactory = batchUpdateFactory;
}
public SubmoduleOp create(Set<Branch.NameKey> updatedBranches, MergeOpRepoManager orm)
throws SubmoduleException {
return new SubmoduleOp(
- gitmodulesFactory,
- serverIdent.get(),
- cfg,
- projectCache,
- batchUpdateFactory,
- updatedBranches,
- orm);
+ gitmodulesFactory, serverIdent.get(), cfg, projectCache, updatedBranches, orm);
}
}
private final GitModules.Factory gitmodulesFactory;
private final PersonIdent myIdent;
private final ProjectCache projectCache;
- private final BatchUpdate.Factory batchUpdateFactory;
private final VerboseSuperprojectUpdate verboseSuperProject;
private final boolean enableSuperProjectSubscriptions;
private final long maxCombinedCommitMessageSize;
@@ -173,14 +163,12 @@
PersonIdent myIdent,
Config cfg,
ProjectCache projectCache,
- BatchUpdate.Factory batchUpdateFactory,
Set<Branch.NameKey> updatedBranches,
MergeOpRepoManager orm)
throws SubmoduleException {
this.gitmodulesFactory = gitmodulesFactory;
this.myIdent = myIdent;
this.projectCache = projectCache;
- this.batchUpdateFactory = batchUpdateFactory;
this.verboseSuperProject =
cfg.getEnum("submodule", null, "verboseSuperprojectUpdate", VerboseSuperprojectUpdate.TRUE);
this.enableSuperProjectSubscriptions =
@@ -420,7 +408,7 @@
}
}
}
- batchUpdateFactory.execute(orm.batchUpdates(superProjects), BatchUpdateListener.NONE, false);
+ BatchUpdate.execute(orm.batchUpdates(superProjects), BatchUpdateListener.NONE, false);
} catch (RestApiException | UpdateException | IOException | NoSuchProjectException e) {
throw new SubmoduleException("Cannot update gitlinks", e);
}
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index a768888..0889f52 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -17,7 +17,10 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableMultiset.toImmutableMultiset;
+import static com.google.common.flogger.LazyArgs.lazy;
+import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toSet;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
@@ -32,20 +35,28 @@
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
+import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.logging.RequestId;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.notedb.ChangeUpdate;
+import com.google.gerrit.server.notedb.NoteDbUpdateManager;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.NoSuchRefException;
+import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Module;
-import com.google.inject.Singleton;
+import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
@@ -55,6 +66,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.TimeZone;
+import java.util.TreeMap;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
@@ -64,81 +76,145 @@
import org.eclipse.jgit.transport.ReceiveCommand;
/**
- * Helper for a set of updates that should be applied for a site.
+ * Helper for a set of change updates that should be applied to the NoteDb database.
*
* <p>An update operation can be divided into three phases:
*
* <ol>
* <li>Git reference updates
- * <li>Database updates
+ * <li>Review metadata updates
* <li>Post-update steps
* <li>
* </ol>
*
* A single conceptual operation, such as a REST API call or a merge operation, may make multiple
* changes at each step, which all need to be serialized relative to each other. Moreover, for
- * consistency, <em>all</em> git ref updates must be performed before <em>any</em> database updates,
- * since database updates might refer to newly-created patch set refs. And all post-update steps,
- * such as hooks, should run only after all storage mutations have completed.
+ * consistency, the git ref updates must be visible to the review metadata updates, since for
+ * example the metadata might refer to newly-created patch set refs. In NoteDb, this is accomplished
+ * by combining these two phases into a single {@link BatchRefUpdate}.
*
- * <p>Depending on the backend used, each step might support batching, for example in a {@code
- * BatchRefUpdate} or one or more database transactions. All operations in one phase must complete
- * successfully before proceeding to the next phase.
+ * <p>Similarly, all post-update steps, such as sending email, must run only after all storage
+ * mutations have completed.
*/
-public abstract class BatchUpdate implements AutoCloseable {
+public class BatchUpdate implements AutoCloseable {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public static Module module() {
return new FactoryModule() {
@Override
public void configure() {
- factory(NoteDbBatchUpdate.AssistedFactory.class);
+ factory(BatchUpdate.Factory.class);
}
};
}
- @Singleton
- public static class Factory {
- private final NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory;
+ // TODO(dborowitz): Make this package-private to force all callers to use RetryHelper.
+ public interface Factory {
+ BatchUpdate create(ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when);
+ }
- // TODO(dborowitz): Make this non-injectable to force all callers to use RetryHelper.
- @Inject
- Factory(NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory) {
- this.noteDbBatchUpdateFactory = noteDbBatchUpdateFactory;
+ public static void execute(
+ Collection<BatchUpdate> updates, BatchUpdateListener listener, boolean dryrun)
+ throws UpdateException, RestApiException {
+ requireNonNull(listener);
+ if (updates.isEmpty()) {
+ return;
}
- public BatchUpdate create(
- ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when) {
- return noteDbBatchUpdateFactory.create(db, project, user, when);
- }
+ checkDifferentProject(updates);
- @SuppressWarnings({"rawtypes", "unchecked"})
- public void execute(
- Collection<BatchUpdate> updates, BatchUpdateListener listener, boolean dryRun)
- throws UpdateException, RestApiException {
- requireNonNull(listener);
- checkDifferentProject(updates);
- // It's safe to downcast all members of the input collection in this case, because the only
- // way a caller could have gotten any BatchUpdates in the first place is to call the create
- // method above, which always returns instances of the type we expect. Just to be safe,
- // copy them into an ImmutableList so there is no chance the callee can pollute the input
- // collection.
- ImmutableList<NoteDbBatchUpdate> noteDbUpdates =
- (ImmutableList) ImmutableList.copyOf(updates);
- NoteDbBatchUpdate.execute(noteDbUpdates, listener, dryRun);
- }
+ try {
+ @SuppressWarnings("deprecation")
+ List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures =
+ new ArrayList<>();
+ List<ChangesHandle> handles = new ArrayList<>(updates.size());
+ Order order = getOrder(updates, listener);
+ try {
+ switch (order) {
+ case REPO_BEFORE_DB:
+ for (BatchUpdate u : updates) {
+ u.executeUpdateRepo();
+ }
+ listener.afterUpdateRepos();
+ for (BatchUpdate u : updates) {
+ handles.add(u.executeChangeOps(dryrun));
+ }
+ for (ChangesHandle h : handles) {
+ h.execute();
+ indexFutures.addAll(h.startIndexFutures());
+ }
+ listener.afterUpdateRefs();
+ listener.afterUpdateChanges();
+ break;
- private static void checkDifferentProject(Collection<BatchUpdate> updates) {
- Multiset<Project.NameKey> projectCounts =
- updates.stream().map(u -> u.project).collect(toImmutableMultiset());
- checkArgument(
- projectCounts.entrySet().size() == updates.size(),
- "updates must all be for different projects, got: %s",
- projectCounts);
+ case DB_BEFORE_REPO:
+ // Call updateChange for each op before updateRepo, but defer executing the
+ // NoteDbUpdateManager until after calling updateRepo. They share an inserter and
+ // BatchRefUpdate, so it will all execute as a single batch. But we have to let
+ // NoteDbUpdateManager actually execute the update, since it has to interleave it
+ // properly with All-Users updates.
+ //
+ // TODO(dborowitz): This may still result in multiple updates to All-Users, but that's
+ // currently not a big deal because multi-change batches generally aren't affecting
+ // drafts anyway.
+ for (BatchUpdate u : updates) {
+ handles.add(u.executeChangeOps(dryrun));
+ }
+ for (BatchUpdate u : updates) {
+ u.executeUpdateRepo();
+ }
+ for (ChangesHandle h : handles) {
+ // TODO(dborowitz): This isn't quite good enough: in theory updateRepo may want to
+ // see the results of change meta commands, but they aren't actually added to the
+ // BatchUpdate until the body of execute. To fix this, execute needs to be split up
+ // into a method that returns a BatchRefUpdate before execution. Not a big deal at the
+ // moment, because this order is only used for deleting changes, and those updateRepo
+ // implementations definitely don't need to observe the updated change meta refs.
+ h.execute();
+ indexFutures.addAll(h.startIndexFutures());
+ }
+ break;
+ default:
+ throw new IllegalStateException("invalid execution order: " + order);
+ }
+ } finally {
+ for (ChangesHandle h : handles) {
+ h.close();
+ }
+ }
+
+ ChangeIndexer.allAsList(indexFutures).get();
+
+ // Fire ref update events only after all mutations are finished, since callers may assume a
+ // patch set ref being created means the change was created, or a branch advancing meaning
+ // some changes were closed.
+ updates
+ .stream()
+ .filter(u -> u.batchRefUpdate != null)
+ .forEach(
+ u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null)));
+
+ if (!dryrun) {
+ for (BatchUpdate u : updates) {
+ u.executePostOps();
+ }
+ }
+ } catch (Exception e) {
+ wrapAndThrowException(e);
}
}
- static Order getOrder(Collection<? extends BatchUpdate> updates, BatchUpdateListener listener) {
+ private static void checkDifferentProject(Collection<BatchUpdate> updates) {
+ Multiset<Project.NameKey> projectCounts =
+ updates.stream().map(u -> u.project).collect(toImmutableMultiset());
+ checkArgument(
+ projectCounts.entrySet().size() == updates.size(),
+ "updates must all be for different projects, got: %s",
+ projectCounts);
+ }
+
+ private static Order getOrder(
+ Collection<? extends BatchUpdate> updates, BatchUpdateListener listener) {
Order o = null;
for (BatchUpdate u : updates) {
if (o == null) {
@@ -156,7 +232,7 @@
return o;
}
- static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException {
+ private static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException {
Throwables.throwIfUnchecked(e);
// Propagate REST API exceptions thrown by operations; they commonly throw exceptions like
@@ -178,32 +254,155 @@
throw new UpdateException(e);
}
- protected GitRepositoryManager repoManager;
+ class ContextImpl implements Context {
+ @Override
+ public RepoView getRepoView() throws IOException {
+ return BatchUpdate.this.getRepoView();
+ }
- protected final Project.NameKey project;
- protected final CurrentUser user;
- protected final Timestamp when;
- protected final TimeZone tz;
+ @Override
+ public RevWalk getRevWalk() throws IOException {
+ return getRepoView().getRevWalk();
+ }
- protected final ListMultimap<Change.Id, BatchUpdateOp> ops =
+ @Override
+ public Project.NameKey getProject() {
+ return project;
+ }
+
+ @Override
+ public Timestamp getWhen() {
+ return when;
+ }
+
+ @Override
+ public TimeZone getTimeZone() {
+ return tz;
+ }
+
+ @Override
+ public ReviewDb getDb() {
+ return db;
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ return user;
+ }
+
+ @Override
+ public Order getOrder() {
+ return order;
+ }
+ }
+
+ private class RepoContextImpl extends ContextImpl implements RepoContext {
+ @Override
+ public ObjectInserter getInserter() throws IOException {
+ return getRepoView().getInserterWrapper();
+ }
+
+ @Override
+ public void addRefUpdate(ReceiveCommand cmd) throws IOException {
+ getRepoView().getCommands().add(cmd);
+ }
+ }
+
+ private class ChangeContextImpl extends ContextImpl implements ChangeContext {
+ private final ChangeNotes notes;
+ private final Map<PatchSet.Id, ChangeUpdate> updates;
+
+ private boolean deleted;
+
+ ChangeContextImpl(ChangeNotes notes) {
+ this.notes = requireNonNull(notes);
+ updates = new TreeMap<>(comparing(PatchSet.Id::get));
+ }
+
+ @Override
+ public ChangeUpdate getUpdate(PatchSet.Id psId) {
+ ChangeUpdate u = updates.get(psId);
+ if (u == null) {
+ u = changeUpdateFactory.create(notes, user, when);
+ if (newChanges.containsKey(notes.getChangeId())) {
+ u.setAllowWriteToNewRef(true);
+ }
+ u.setPatchSetId(psId);
+ updates.put(psId, u);
+ }
+ return u;
+ }
+
+ @Override
+ public ChangeNotes getNotes() {
+ return notes;
+ }
+
+ @Override
+ public void dontBumpLastUpdatedOn() {
+ // Do nothing; NoteDb effectively updates timestamp if and only if a commit was written to the
+ // change meta ref.
+ }
+
+ @Override
+ public void deleteChange() {
+ deleted = true;
+ }
+ }
+
+ /** Per-change result status from {@link #executeChangeOps}. */
+ private enum ChangeResult {
+ SKIPPED,
+ UPSERTED,
+ DELETED;
+ }
+
+ private GitRepositoryManager repoManager;
+
+ private final ChangeNotes.Factory changeNotesFactory;
+ private final ChangeUpdate.Factory changeUpdateFactory;
+ private final NoteDbUpdateManager.Factory updateManagerFactory;
+ private final ChangeIndexer indexer;
+ private final GitReferenceUpdated gitRefUpdated;
+
+ private final ReviewDb db;
+ private final Project.NameKey project;
+ private final CurrentUser user;
+ private final Timestamp when;
+ private final TimeZone tz;
+
+ private final ListMultimap<Change.Id, BatchUpdateOp> ops =
MultimapBuilder.linkedHashKeys().arrayListValues().build();
- protected final Map<Change.Id, Change> newChanges = new HashMap<>();
- protected final List<RepoOnlyOp> repoOnlyOps = new ArrayList<>();
+ private final Map<Change.Id, Change> newChanges = new HashMap<>();
+ private final List<RepoOnlyOp> repoOnlyOps = new ArrayList<>();
- protected RepoView repoView;
- protected BatchRefUpdate batchRefUpdate;
- protected Order order;
- protected OnSubmitValidators onSubmitValidators;
- protected PushCertificate pushCert;
- protected String refLogMessage;
+ private RepoView repoView;
+ private BatchRefUpdate batchRefUpdate;
+ private Order order;
+ private OnSubmitValidators onSubmitValidators;
+ private PushCertificate pushCert;
+ private String refLogMessage;
- protected BatchUpdate(
+ @Inject
+ BatchUpdate(
GitRepositoryManager repoManager,
- PersonIdent serverIdent,
- Project.NameKey project,
- CurrentUser user,
- Timestamp when) {
+ @GerritPersonIdent PersonIdent serverIdent,
+ ChangeNotes.Factory changeNotesFactory,
+ ChangeUpdate.Factory changeUpdateFactory,
+ NoteDbUpdateManager.Factory updateManagerFactory,
+ ChangeIndexer indexer,
+ GitReferenceUpdated gitRefUpdated,
+ @Assisted ReviewDb db,
+ @Assisted Project.NameKey project,
+ @Assisted CurrentUser user,
+ @Assisted Timestamp when) {
this.repoManager = repoManager;
+ this.changeNotesFactory = changeNotesFactory;
+ this.changeUpdateFactory = changeUpdateFactory;
+ this.updateManagerFactory = updateManagerFactory;
+ this.indexer = indexer;
+ this.gitRefUpdated = gitRefUpdated;
+ this.db = db;
this.project = project;
this.user = user;
this.when = when;
@@ -218,15 +417,14 @@
}
}
- public abstract void execute(BatchUpdateListener listener)
- throws UpdateException, RestApiException;
+ public void execute(BatchUpdateListener listener) throws UpdateException, RestApiException {
+ execute(ImmutableList.of(this), listener, false);
+ }
public void execute() throws UpdateException, RestApiException {
execute(BatchUpdateListener.NONE);
}
- protected abstract Context newContext();
-
public BatchUpdate setRepository(Repository repo, RevWalk revWalk, ObjectInserter inserter) {
checkState(this.repoView == null, "repo already set");
repoView = new RepoView(repo, revWalk, inserter);
@@ -257,32 +455,23 @@
return this;
}
- protected void initRepository() throws IOException {
+ private void initRepository() throws IOException {
if (repoView == null) {
repoView = new RepoView(repoManager, project);
}
}
- protected RepoView getRepoView() throws IOException {
+ private RepoView getRepoView() throws IOException {
initRepository();
return repoView;
}
- protected CurrentUser getUser() {
- return user;
- }
-
- protected Optional<AccountState> getAccount() {
+ private Optional<AccountState> getAccount() {
return user.isIdentifiedUser()
? Optional.of(user.asIdentifiedUser().state())
: Optional.empty();
}
- protected RevWalk getRevWalk() throws IOException {
- initRepository();
- return repoView.getRevWalk();
- }
-
public Map<String, ReceiveCommand> getRefUpdates() {
return repoView != null ? repoView.getCommands().getCommands() : ImmutableMap.of();
}
@@ -301,7 +490,7 @@
}
public BatchUpdate insertChange(InsertChangeOp op) throws IOException {
- Context ctx = newContext();
+ Context ctx = new ContextImpl();
Change c = op.createChange(ctx);
checkArgument(
!newChanges.containsKey(c.getId()), "only one op allowed to create change %s", c.getId());
@@ -310,16 +499,165 @@
return this;
}
- protected static void logDebug(String msg, Throwable t) {
- // Only log if there is a requestId assigned, since those are the
- // expensive/complicated requests like MergeOp. Doing it every time would be
- // noisy.
- if (RequestId.isSet()) {
- logger.atFine().withCause(t).log("%s", msg);
+ private void executeUpdateRepo() throws UpdateException, RestApiException {
+ try {
+ logDebug("Executing updateRepo on %d ops", ops.size());
+ RepoContextImpl ctx = new RepoContextImpl();
+ for (BatchUpdateOp op : ops.values()) {
+ op.updateRepo(ctx);
+ }
+
+ logDebug("Executing updateRepo on %d RepoOnlyOps", repoOnlyOps.size());
+ for (RepoOnlyOp op : repoOnlyOps) {
+ op.updateRepo(ctx);
+ }
+
+ if (onSubmitValidators != null && !getRefUpdates().isEmpty()) {
+ // Validation of refs has to take place here and not at the beginning of executeRefUpdates.
+ // Otherwise, failing validation in a second BatchUpdate object will happen *after* the
+ // first update's executeRefUpdates has finished, hence after first repo's refs have been
+ // updated, which is too late.
+ onSubmitValidators.validate(
+ project, ctx.getRevWalk().getObjectReader(), repoView.getCommands());
+ }
+ } catch (Exception e) {
+ Throwables.throwIfInstanceOf(e, RestApiException.class);
+ throw new UpdateException(e);
}
}
- protected static void logDebug(String msg) {
+ private class ChangesHandle implements AutoCloseable {
+ private final NoteDbUpdateManager manager;
+ private final boolean dryrun;
+ private final Map<Change.Id, ChangeResult> results;
+
+ ChangesHandle(NoteDbUpdateManager manager, boolean dryrun) {
+ this.manager = manager;
+ this.dryrun = dryrun;
+ results = new HashMap<>();
+ }
+
+ @Override
+ public void close() {
+ manager.close();
+ }
+
+ void setResult(Change.Id id, ChangeResult result) {
+ ChangeResult old = results.putIfAbsent(id, result);
+ checkArgument(old == null, "result for change %s already set: %s", id, old);
+ }
+
+ void execute() throws OrmException, IOException {
+ BatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
+ }
+
+ @SuppressWarnings("deprecation")
+ List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> startIndexFutures() {
+ if (dryrun) {
+ return ImmutableList.of();
+ }
+ logDebug("Reindexing %d changes", results.size());
+ List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures =
+ new ArrayList<>(results.size());
+ for (Map.Entry<Change.Id, ChangeResult> e : results.entrySet()) {
+ Change.Id id = e.getKey();
+ switch (e.getValue()) {
+ case UPSERTED:
+ indexFutures.add(indexer.indexAsync(project, id));
+ break;
+ case DELETED:
+ indexFutures.add(indexer.deleteAsync(id));
+ break;
+ case SKIPPED:
+ break;
+ default:
+ throw new IllegalStateException("unexpected result: " + e.getValue());
+ }
+ }
+ return indexFutures;
+ }
+ }
+
+ private ChangesHandle executeChangeOps(boolean dryrun) throws Exception {
+ logDebug("Executing change ops");
+ initRepository();
+ Repository repo = repoView.getRepository();
+ checkState(
+ repo.getRefDatabase().performsAtomicTransactions(),
+ "cannot use NoteDb with a repository that does not support atomic batch ref updates: %s",
+ repo);
+
+ ChangesHandle handle =
+ new ChangesHandle(
+ updateManagerFactory
+ .create(project)
+ .setChangeRepo(
+ repo, repoView.getRevWalk(), repoView.getInserter(), repoView.getCommands()),
+ dryrun);
+ if (user.isIdentifiedUser()) {
+ handle.manager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz));
+ }
+ handle.manager.setRefLogMessage(refLogMessage);
+ handle.manager.setPushCertificate(pushCert);
+ for (Map.Entry<Change.Id, Collection<BatchUpdateOp>> e : ops.asMap().entrySet()) {
+ Change.Id id = e.getKey();
+ ChangeContextImpl ctx = newChangeContext(id);
+ boolean dirty = false;
+ logDebug(
+ "Applying %d ops for change %s: %s",
+ e.getValue().size(),
+ id,
+ lazy(() -> e.getValue().stream().map(op -> op.getClass().getName()).collect(toSet())));
+ for (BatchUpdateOp op : e.getValue()) {
+ dirty |= op.updateChange(ctx);
+ }
+ if (!dirty) {
+ logDebug("No ops reported dirty, short-circuiting");
+ handle.setResult(id, ChangeResult.SKIPPED);
+ continue;
+ }
+ for (ChangeUpdate u : ctx.updates.values()) {
+ handle.manager.add(u);
+ }
+ if (ctx.deleted) {
+ logDebug("Change %s was deleted", id);
+ handle.manager.deleteChange(id);
+ handle.setResult(id, ChangeResult.DELETED);
+ } else {
+ handle.setResult(id, ChangeResult.UPSERTED);
+ }
+ }
+ return handle;
+ }
+
+ private ChangeContextImpl newChangeContext(Change.Id id) throws OrmException {
+ logDebug("Opening change %s for update", id);
+ Change c = newChanges.get(id);
+ boolean isNew = c != null;
+ if (!isNew) {
+ // Pass a synthetic change into ChangeNotes.Factory, which will take care of checking for
+ // existence and populating columns from the parsed notes state.
+ // TODO(dborowitz): This dance made more sense when using Reviewdb; consider a nicer way.
+ c = ChangeNotes.Factory.newNoteDbOnlyChange(project, id);
+ } else {
+ logDebug("Change %s is new", id);
+ }
+ ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew);
+ return new ChangeContextImpl(notes);
+ }
+
+ private void executePostOps() throws Exception {
+ ContextImpl ctx = new ContextImpl();
+ for (BatchUpdateOp op : ops.values()) {
+ op.postUpdate(ctx);
+ }
+
+ for (RepoOnlyOp op : repoOnlyOps) {
+ op.postUpdate(ctx);
+ }
+ }
+
+ private static void logDebug(String msg) {
// Only log if there is a requestId assigned, since those are the
// expensive/complicated requests like MergeOp. Doing it every time would be
// noisy.
@@ -328,7 +666,7 @@
}
}
- protected static void logDebug(String msg, @Nullable Object arg) {
+ private static void logDebug(String msg, @Nullable Object arg) {
// Only log if there is a requestId assigned, since those are the
// expensive/complicated requests like MergeOp. Doing it every time would be
// noisy.
@@ -337,16 +675,7 @@
}
}
- protected static void logDebug(String msg, @Nullable Object arg1, @Nullable Object arg2) {
- // Only log if there is a requestId assigned, since those are the
- // expensive/complicated requests like MergeOp. Doing it every time would be
- // noisy.
- if (RequestId.isSet()) {
- logger.atFine().log(msg, arg1, arg2);
- }
- }
-
- protected static void logDebug(
+ private static void logDebug(
String msg, @Nullable Object arg1, @Nullable Object arg2, @Nullable Object arg3) {
// Only log if there is a requestId assigned, since those are the
// expensive/complicated requests like MergeOp. Doing it every time would be
diff --git a/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java b/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java
deleted file mode 100644
index d881b0f..0000000
--- a/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java
+++ /dev/null
@@ -1,457 +0,0 @@
-// 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.update;
-
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.common.base.Preconditions.checkState;
-import static com.google.common.flogger.LazyArgs.lazy;
-import static java.util.Comparator.comparing;
-import static java.util.Objects.requireNonNull;
-import static java.util.stream.Collectors.toSet;
-
-import com.google.common.base.Throwables;
-import com.google.common.collect.ImmutableList;
-import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.reviewdb.client.Change;
-import com.google.gerrit.reviewdb.client.PatchSet;
-import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.reviewdb.server.ReviewDb;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.GerritPersonIdent;
-import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
-import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.index.change.ChangeIndexer;
-import com.google.gerrit.server.notedb.ChangeNotes;
-import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.notedb.NoteDbUpdateManager;
-import com.google.gwtorm.server.OrmException;
-import com.google.inject.Inject;
-import com.google.inject.assistedinject.Assisted;
-import java.io.IOException;
-import java.sql.Timestamp;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.TimeZone;
-import java.util.TreeMap;
-import org.eclipse.jgit.lib.ObjectInserter;
-import org.eclipse.jgit.lib.PersonIdent;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.transport.ReceiveCommand;
-
-/**
- * {@link BatchUpdate} implementation using only NoteDb that updates code refs and meta refs in a
- * single {@link org.eclipse.jgit.lib.BatchRefUpdate}.
- *
- * <p>Used when {@code noteDb.changes.disableReviewDb=true}, at which point ReviewDb is not
- * consulted during updates.
- */
-public class NoteDbBatchUpdate extends BatchUpdate {
- public interface AssistedFactory {
- NoteDbBatchUpdate create(
- ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when);
- }
-
- static void execute(
- ImmutableList<NoteDbBatchUpdate> updates, BatchUpdateListener listener, boolean dryrun)
- throws UpdateException, RestApiException {
- if (updates.isEmpty()) {
- return;
- }
-
- try {
- @SuppressWarnings("deprecation")
- List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures =
- new ArrayList<>();
- List<ChangesHandle> handles = new ArrayList<>(updates.size());
- Order order = getOrder(updates, listener);
- try {
- switch (order) {
- case REPO_BEFORE_DB:
- for (NoteDbBatchUpdate u : updates) {
- u.executeUpdateRepo();
- }
- listener.afterUpdateRepos();
- for (NoteDbBatchUpdate u : updates) {
- handles.add(u.executeChangeOps(dryrun));
- }
- for (ChangesHandle h : handles) {
- h.execute();
- indexFutures.addAll(h.startIndexFutures());
- }
- listener.afterUpdateRefs();
- listener.afterUpdateChanges();
- break;
-
- case DB_BEFORE_REPO:
- // Call updateChange for each op before updateRepo, but defer executing the
- // NoteDbUpdateManager until after calling updateRepo. They share an inserter and
- // BatchRefUpdate, so it will all execute as a single batch. But we have to let
- // NoteDbUpdateManager actually execute the update, since it has to interleave it
- // properly with All-Users updates.
- //
- // TODO(dborowitz): This may still result in multiple updates to All-Users, but that's
- // currently not a big deal because multi-change batches generally aren't affecting
- // drafts anyway.
- for (NoteDbBatchUpdate u : updates) {
- handles.add(u.executeChangeOps(dryrun));
- }
- for (NoteDbBatchUpdate u : updates) {
- u.executeUpdateRepo();
- }
- for (ChangesHandle h : handles) {
- // TODO(dborowitz): This isn't quite good enough: in theory updateRepo may want to
- // see the results of change meta commands, but they aren't actually added to the
- // BatchUpdate until the body of execute. To fix this, execute needs to be split up
- // into a method that returns a BatchRefUpdate before execution. Not a big deal at the
- // moment, because this order is only used for deleting changes, and those updateRepo
- // implementations definitely don't need to observe the updated change meta refs.
- h.execute();
- indexFutures.addAll(h.startIndexFutures());
- }
- break;
- default:
- throw new IllegalStateException("invalid execution order: " + order);
- }
- } finally {
- for (ChangesHandle h : handles) {
- h.close();
- }
- }
-
- ChangeIndexer.allAsList(indexFutures).get();
-
- // Fire ref update events only after all mutations are finished, since callers may assume a
- // patch set ref being created means the change was created, or a branch advancing meaning
- // some changes were closed.
- updates
- .stream()
- .filter(u -> u.batchRefUpdate != null)
- .forEach(
- u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null)));
-
- if (!dryrun) {
- for (NoteDbBatchUpdate u : updates) {
- u.executePostOps();
- }
- }
- } catch (Exception e) {
- wrapAndThrowException(e);
- }
- }
-
- class ContextImpl implements Context {
- @Override
- public RepoView getRepoView() throws IOException {
- return NoteDbBatchUpdate.this.getRepoView();
- }
-
- @Override
- public RevWalk getRevWalk() throws IOException {
- return getRepoView().getRevWalk();
- }
-
- @Override
- public Project.NameKey getProject() {
- return project;
- }
-
- @Override
- public Timestamp getWhen() {
- return when;
- }
-
- @Override
- public TimeZone getTimeZone() {
- return tz;
- }
-
- @Override
- public ReviewDb getDb() {
- return db;
- }
-
- @Override
- public CurrentUser getUser() {
- return user;
- }
-
- @Override
- public Order getOrder() {
- return order;
- }
- }
-
- private class RepoContextImpl extends ContextImpl implements RepoContext {
- @Override
- public ObjectInserter getInserter() throws IOException {
- return getRepoView().getInserterWrapper();
- }
-
- @Override
- public void addRefUpdate(ReceiveCommand cmd) throws IOException {
- getRepoView().getCommands().add(cmd);
- }
- }
-
- private class ChangeContextImpl extends ContextImpl implements ChangeContext {
- private final ChangeNotes notes;
- private final Map<PatchSet.Id, ChangeUpdate> updates;
-
- private boolean deleted;
-
- protected ChangeContextImpl(ChangeNotes notes) {
- this.notes = requireNonNull(notes);
- updates = new TreeMap<>(comparing(PatchSet.Id::get));
- }
-
- @Override
- public ChangeUpdate getUpdate(PatchSet.Id psId) {
- ChangeUpdate u = updates.get(psId);
- if (u == null) {
- u = changeUpdateFactory.create(notes, user, when);
- if (newChanges.containsKey(notes.getChangeId())) {
- u.setAllowWriteToNewRef(true);
- }
- u.setPatchSetId(psId);
- updates.put(psId, u);
- }
- return u;
- }
-
- @Override
- public ChangeNotes getNotes() {
- return notes;
- }
-
- @Override
- public void dontBumpLastUpdatedOn() {
- // Do nothing; NoteDb effectively updates timestamp if and only if a commit was written to the
- // change meta ref.
- }
-
- @Override
- public void deleteChange() {
- deleted = true;
- }
- }
-
- /** Per-change result status from {@link #executeChangeOps}. */
- private enum ChangeResult {
- SKIPPED,
- UPSERTED,
- DELETED;
- }
-
- private final ChangeNotes.Factory changeNotesFactory;
- private final ChangeUpdate.Factory changeUpdateFactory;
- private final NoteDbUpdateManager.Factory updateManagerFactory;
- private final ChangeIndexer indexer;
- private final GitReferenceUpdated gitRefUpdated;
- private final ReviewDb db;
-
- @Inject
- NoteDbBatchUpdate(
- GitRepositoryManager repoManager,
- @GerritPersonIdent PersonIdent serverIdent,
- ChangeNotes.Factory changeNotesFactory,
- ChangeUpdate.Factory changeUpdateFactory,
- NoteDbUpdateManager.Factory updateManagerFactory,
- ChangeIndexer indexer,
- GitReferenceUpdated gitRefUpdated,
- @Assisted ReviewDb db,
- @Assisted Project.NameKey project,
- @Assisted CurrentUser user,
- @Assisted Timestamp when) {
- super(repoManager, serverIdent, project, user, when);
- this.changeNotesFactory = changeNotesFactory;
- this.changeUpdateFactory = changeUpdateFactory;
- this.updateManagerFactory = updateManagerFactory;
- this.indexer = indexer;
- this.gitRefUpdated = gitRefUpdated;
- this.db = db;
- }
-
- @Override
- public void execute(BatchUpdateListener listener) throws UpdateException, RestApiException {
- execute(ImmutableList.of(this), listener, false);
- }
-
- @Override
- protected Context newContext() {
- return new ContextImpl();
- }
-
- private void executeUpdateRepo() throws UpdateException, RestApiException {
- try {
- logDebug("Executing updateRepo on %d ops", ops.size());
- RepoContextImpl ctx = new RepoContextImpl();
- for (BatchUpdateOp op : ops.values()) {
- op.updateRepo(ctx);
- }
-
- logDebug("Executing updateRepo on %d RepoOnlyOps", repoOnlyOps.size());
- for (RepoOnlyOp op : repoOnlyOps) {
- op.updateRepo(ctx);
- }
-
- if (onSubmitValidators != null && !getRefUpdates().isEmpty()) {
- // Validation of refs has to take place here and not at the beginning of executeRefUpdates.
- // Otherwise, failing validation in a second BatchUpdate object will happen *after* the
- // first update's executeRefUpdates has finished, hence after first repo's refs have been
- // updated, which is too late.
- onSubmitValidators.validate(
- project, ctx.getRevWalk().getObjectReader(), repoView.getCommands());
- }
- } catch (Exception e) {
- Throwables.throwIfInstanceOf(e, RestApiException.class);
- throw new UpdateException(e);
- }
- }
-
- private class ChangesHandle implements AutoCloseable {
- private final NoteDbUpdateManager manager;
- private final boolean dryrun;
- private final Map<Change.Id, ChangeResult> results;
-
- ChangesHandle(NoteDbUpdateManager manager, boolean dryrun) {
- this.manager = manager;
- this.dryrun = dryrun;
- results = new HashMap<>();
- }
-
- @Override
- public void close() {
- manager.close();
- }
-
- void setResult(Change.Id id, ChangeResult result) {
- ChangeResult old = results.putIfAbsent(id, result);
- checkArgument(old == null, "result for change %s already set: %s", id, old);
- }
-
- void execute() throws OrmException, IOException {
- NoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
- }
-
- @SuppressWarnings("deprecation")
- List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> startIndexFutures() {
- if (dryrun) {
- return ImmutableList.of();
- }
- logDebug("Reindexing %d changes", results.size());
- List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures =
- new ArrayList<>(results.size());
- for (Map.Entry<Change.Id, ChangeResult> e : results.entrySet()) {
- Change.Id id = e.getKey();
- switch (e.getValue()) {
- case UPSERTED:
- indexFutures.add(indexer.indexAsync(project, id));
- break;
- case DELETED:
- indexFutures.add(indexer.deleteAsync(id));
- break;
- case SKIPPED:
- break;
- default:
- throw new IllegalStateException("unexpected result: " + e.getValue());
- }
- }
- return indexFutures;
- }
- }
-
- private ChangesHandle executeChangeOps(boolean dryrun) throws Exception {
- logDebug("Executing change ops");
- initRepository();
- Repository repo = repoView.getRepository();
- checkState(
- repo.getRefDatabase().performsAtomicTransactions(),
- "cannot use NoteDb with a repository that does not support atomic batch ref updates: %s",
- repo);
-
- ChangesHandle handle =
- new ChangesHandle(
- updateManagerFactory
- .create(project)
- .setChangeRepo(
- repo, repoView.getRevWalk(), repoView.getInserter(), repoView.getCommands()),
- dryrun);
- if (user.isIdentifiedUser()) {
- handle.manager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz));
- }
- handle.manager.setRefLogMessage(refLogMessage);
- handle.manager.setPushCertificate(pushCert);
- for (Map.Entry<Change.Id, Collection<BatchUpdateOp>> e : ops.asMap().entrySet()) {
- Change.Id id = e.getKey();
- ChangeContextImpl ctx = newChangeContext(id);
- boolean dirty = false;
- logDebug(
- "Applying %d ops for change %s: %s",
- e.getValue().size(),
- id,
- lazy(() -> e.getValue().stream().map(op -> op.getClass().getName()).collect(toSet())));
- for (BatchUpdateOp op : e.getValue()) {
- dirty |= op.updateChange(ctx);
- }
- if (!dirty) {
- logDebug("No ops reported dirty, short-circuiting");
- handle.setResult(id, ChangeResult.SKIPPED);
- continue;
- }
- for (ChangeUpdate u : ctx.updates.values()) {
- handle.manager.add(u);
- }
- if (ctx.deleted) {
- logDebug("Change %s was deleted", id);
- handle.manager.deleteChange(id);
- handle.setResult(id, ChangeResult.DELETED);
- } else {
- handle.setResult(id, ChangeResult.UPSERTED);
- }
- }
- return handle;
- }
-
- private ChangeContextImpl newChangeContext(Change.Id id) throws OrmException {
- logDebug("Opening change %s for update", id);
- Change c = newChanges.get(id);
- boolean isNew = c != null;
- if (!isNew) {
- // Pass a synthetic change into ChangeNotes.Factory, which will take care of checking for
- // existence and populating columns from the parsed notes state.
- // TODO(dborowitz): This dance made more sense when using Reviewdb; consider a nicer way.
- c = ChangeNotes.Factory.newNoteDbOnlyChange(project, id);
- } else {
- logDebug("Change %s is new", id);
- }
- ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew);
- return new ChangeContextImpl(notes);
- }
-
- private void executePostOps() throws Exception {
- ContextImpl ctx = new ContextImpl();
- for (BatchUpdateOp op : ops.values()) {
- op.postUpdate(ctx);
- }
-
- for (RepoOnlyOp op : repoOnlyOps) {
- op.postUpdate(ctx);
- }
- }
-}
diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java
index 9bdf293..d2bccf1 100644
--- a/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/java/com/google/gerrit/server/update/RetryHelper.java
@@ -147,21 +147,18 @@
@Nullable private final Consumer<RetryerBuilder<?>> overwriteDefaultRetryerStrategySetup;
@Inject
- RetryHelper(
- @GerritServerConfig Config cfg,
- Metrics metrics,
- NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory) {
- this(cfg, metrics, noteDbBatchUpdateFactory, null);
+ RetryHelper(@GerritServerConfig Config cfg, Metrics metrics, BatchUpdate.Factory updateFactory) {
+ this(cfg, metrics, updateFactory, null);
}
@VisibleForTesting
public RetryHelper(
@GerritServerConfig Config cfg,
Metrics metrics,
- NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory,
+ BatchUpdate.Factory updateFactory,
@Nullable Consumer<RetryerBuilder<?>> overwriteDefaultRetryerStrategySetup) {
this.metrics = metrics;
- this.updateFactory = new BatchUpdate.Factory(noteDbBatchUpdateFactory);
+ this.updateFactory = updateFactory;
Duration defaultTimeout =
Duration.ofMillis(
diff --git a/java/com/google/gerrit/testing/GerritServerTests.java b/java/com/google/gerrit/testing/GerritServerTests.java
index 69806e1..75479d2 100644
--- a/java/com/google/gerrit/testing/GerritServerTests.java
+++ b/java/com/google/gerrit/testing/GerritServerTests.java
@@ -14,7 +14,7 @@
package com.google.gerrit.testing;
-import com.google.gerrit.server.notedb.MutableNotesMigration;
+import com.google.gerrit.server.notedb.NotesMigration;
import org.eclipse.jgit.lib.Config;
import org.junit.Rule;
import org.junit.rules.TestRule;
@@ -28,7 +28,7 @@
@ConfigSuite.Name private String configName;
- protected MutableNotesMigration notesMigration;
+ protected NotesMigration notesMigration;
@Rule
public TestRule testRunner =
@@ -39,21 +39,13 @@
@Override
public void evaluate() throws Throwable {
beforeTest();
- try {
- base.evaluate();
- } finally {
- afterTest();
- }
+ base.evaluate();
}
};
}
};
- public void beforeTest() throws Exception {
- notesMigration = NoteDbMode.newNotesMigrationFromEnv();
- }
-
- public void afterTest() {
- NoteDbMode.resetFromEnv(notesMigration);
+ public void beforeTest() {
+ notesMigration = new NotesMigration();
}
}
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 682e8c2..ac7224f 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -73,8 +73,6 @@
import com.google.gerrit.server.index.group.GroupIndexCollection;
import com.google.gerrit.server.index.group.GroupSchemaDefinitions;
import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier;
-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.permissions.DefaultPermissionBackendModule;
import com.google.gerrit.server.plugins.ServerInformationImpl;
@@ -135,15 +133,13 @@
}
private final Config cfg;
- private final MutableNotesMigration notesMigration;
public InMemoryModule() {
- this(newDefaultConfig(), NoteDbMode.newNotesMigrationFromEnv());
+ this(newDefaultConfig());
}
- public InMemoryModule(Config cfg, MutableNotesMigration notesMigration) {
+ public InMemoryModule(Config cfg) {
this.cfg = cfg;
- this.notesMigration = notesMigration;
}
public void inject(Object instance) {
@@ -191,8 +187,6 @@
bind(GitRepositoryManager.class).to(InMemoryRepositoryManager.class);
bind(InMemoryRepositoryManager.class).in(SINGLETON);
bind(TrackingFooters.class).toProvider(TrackingFootersProvider.class).in(SINGLETON);
- bind(MutableNotesMigration.class).toInstance(notesMigration);
- bind(NotesMigration.class).to(MutableNotesMigration.class);
bind(ListeningExecutorService.class)
.annotatedWith(ChangeUpdateExecutor.class)
.toInstance(MoreExecutors.newDirectExecutorService());
diff --git a/java/com/google/gerrit/testing/InMemoryTestEnvironment.java b/java/com/google/gerrit/testing/InMemoryTestEnvironment.java
index f665015..a8afdb3 100644
--- a/java/com/google/gerrit/testing/InMemoryTestEnvironment.java
+++ b/java/com/google/gerrit/testing/InMemoryTestEnvironment.java
@@ -108,8 +108,7 @@
Config cfg = configProvider.get();
InMemoryModule.setDefaults(cfg);
- Injector injector =
- Guice.createInjector(new InMemoryModule(cfg, NoteDbMode.newNotesMigrationFromEnv()));
+ Injector injector = Guice.createInjector(new InMemoryModule(cfg));
injector.injectMembers(this);
lifecycle = new LifecycleManager();
lifecycle.add(injector);
diff --git a/java/com/google/gerrit/testing/NoteDbMode.java b/java/com/google/gerrit/testing/NoteDbMode.java
deleted file mode 100644
index f901cce..0000000
--- a/java/com/google/gerrit/testing/NoteDbMode.java
+++ /dev/null
@@ -1,81 +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.testing;
-
-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.MutableNotesMigration;
-import com.google.gerrit.server.notedb.NotesMigrationState;
-
-public enum NoteDbMode {
- /** NoteDb is disabled. */
- OFF(NotesMigrationState.REVIEW_DB),
-
- /** Writing data to NoteDb is enabled. */
- WRITE(NotesMigrationState.WRITE),
-
- /** Reading and writing all data to NoteDb is enabled. */
- READ_WRITE(NotesMigrationState.READ_WRITE_WITH_SEQUENCE_REVIEW_DB_PRIMARY),
-
- /** Changes are created with their primary storage as NoteDb. */
- PRIMARY(NotesMigrationState.READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY),
-
- /** All change tables are entirely disabled, and code/meta ref updates are fused. */
- ON(NotesMigrationState.NOTE_DB);
-
- private static final String ENV_VAR = "GERRIT_NOTEDB";
- private static final String SYS_PROP = "gerrit.notedb";
-
- public static NoteDbMode get() {
- String value = System.getenv(ENV_VAR);
- if (Strings.isNullOrEmpty(value)) {
- value = System.getProperty(SYS_PROP);
- }
- if (Strings.isNullOrEmpty(value)) {
- return ON;
- }
- value = value.toUpperCase().replace("-", "_");
- NoteDbMode mode = Enums.getIfPresent(NoteDbMode.class, value).orNull();
- if (!Strings.isNullOrEmpty(System.getenv(ENV_VAR))) {
- checkArgument(
- mode != null, "Invalid value for env variable %s: %s", ENV_VAR, System.getenv(ENV_VAR));
- } else {
- checkArgument(
- mode != null,
- "Invalid value for system property %s: %s",
- SYS_PROP,
- System.getProperty(SYS_PROP));
- }
- return mode;
- }
-
- 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) {
- this.state = state;
- }
-}
diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index d2fd331..7108a98 100644
--- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -53,7 +53,6 @@
import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
import com.google.gerrit.server.project.testing.Util;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.testing.NoteDbMode;
import com.google.inject.Inject;
import java.io.IOException;
import java.util.ArrayList;
@@ -369,7 +368,6 @@
@Test
public void uploadPackSequencesWithAccessDatabase() throws Exception {
- assume().that(notesMigration.readChangeSequence()).isTrue();
try (Repository repo = repoManager.openRepository(allProjects)) {
assertRefs(repo, newFilter(allProjects, user), true);
@@ -665,9 +663,7 @@
List<String> expectedMetaRefs =
new ArrayList<>(ImmutableList.of(mr.getPatchSetId().toRefName()));
- if (NoteDbMode.get() != NoteDbMode.OFF) {
- expectedMetaRefs.add(changeRefPrefix(mr.getChange().getId()) + "meta");
- }
+ expectedMetaRefs.add(changeRefPrefix(mr.getChange().getId()) + "meta");
List<String> expectedAllRefs = new ArrayList<>(expectedNonMetaRefs);
expectedAllRefs.addAll(expectedMetaRefs);
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
index 2a397e4..7b302c4 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
@@ -21,6 +21,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
@@ -319,14 +320,14 @@
input.state = state;
gApi.changes().id(r.getChangeId()).addReviewer(input);
- notesMigration.setFailOnLoadForTest(true);
+ Context oldCtx = disableDb();
try {
ChangeInfo info =
Iterables.getOnlyElement(
gApi.changes().query(r.getChangeId()).withOption(DETAILED_LABELS).get());
assertThat(info.reviewers).isEqualTo(ImmutableMap.of(state, ImmutableList.of(acc)));
} finally {
- notesMigration.setFailOnLoadForTest(false);
+ enableDb(oldCtx);
}
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
index 6c0b707..14521cc 100644
--- a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
@@ -127,10 +127,7 @@
assertThat(i.user.anonymousCowardName).isEqualTo("Unnamed User");
// notedb
- notesMigration.setReadChanges(true);
assertThat(gApi.config().server().getInfo().noteDbEnabled).isTrue();
- notesMigration.setReadChanges(false);
- assertThat(gApi.config().server().getInfo().noteDbEnabled).isNull();
}
@Test
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryAccountsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryAccountsTest.java
index c8ce54a..27868d2 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryAccountsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryAccountsTest.java
@@ -65,6 +65,6 @@
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(
elasticsearchConfig, nodeInfo.port, indicesPrefix, ElasticVersion.V5_6);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryChangesTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryChangesTest.java
index cfdfa98..2e4e22a 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryChangesTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryChangesTest.java
@@ -65,6 +65,6 @@
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(
elasticsearchConfig, nodeInfo.port, indicesPrefix, ElasticVersion.V5_6);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryGroupsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryGroupsTest.java
index 832a7bd..98c4321 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryGroupsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryGroupsTest.java
@@ -65,6 +65,6 @@
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(
elasticsearchConfig, nodeInfo.port, indicesPrefix, ElasticVersion.V5_6);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryProjectsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryProjectsTest.java
index 29d3fa4..6b4b58c 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryProjectsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV5QueryProjectsTest.java
@@ -65,6 +65,6 @@
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(
elasticsearchConfig, nodeInfo.port, indicesPrefix, ElasticVersion.V5_6);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryAccountsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryAccountsTest.java
index 8833907..53593ef 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryAccountsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryAccountsTest.java
@@ -64,6 +64,6 @@
InMemoryModule.setDefaults(elasticsearchConfig);
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryChangesTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryChangesTest.java
index 8ba753c..6429431 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryChangesTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryChangesTest.java
@@ -64,6 +64,6 @@
InMemoryModule.setDefaults(elasticsearchConfig);
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryGroupsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryGroupsTest.java
index cecb085..de0af97 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryGroupsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryGroupsTest.java
@@ -64,6 +64,6 @@
InMemoryModule.setDefaults(elasticsearchConfig);
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryProjectsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryProjectsTest.java
index 47e9b10..0ce66e8 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryProjectsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV6QueryProjectsTest.java
@@ -64,6 +64,6 @@
InMemoryModule.setDefaults(elasticsearchConfig);
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryAccountsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryAccountsTest.java
index bddbbc9..6972a18 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryAccountsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryAccountsTest.java
@@ -64,6 +64,6 @@
InMemoryModule.setDefaults(elasticsearchConfig);
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryChangesTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryChangesTest.java
index 5dcf159..988abca 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryChangesTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryChangesTest.java
@@ -82,6 +82,6 @@
InMemoryModule.setDefaults(elasticsearchConfig);
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryGroupsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryGroupsTest.java
index 54be7b9..534bc36 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryGroupsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryGroupsTest.java
@@ -64,6 +64,6 @@
InMemoryModule.setDefaults(elasticsearchConfig);
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryProjectsTest.java b/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryProjectsTest.java
index e8b4a2c..1f4653c 100644
--- a/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryProjectsTest.java
+++ b/javatests/com/google/gerrit/elasticsearch/ElasticV7QueryProjectsTest.java
@@ -64,6 +64,6 @@
InMemoryModule.setDefaults(elasticsearchConfig);
String indicesPrefix = getSanitizedMethodName();
ElasticTestUtils.configure(elasticsearchConfig, nodeInfo.port, indicesPrefix);
- return Guice.createInjector(new InMemoryModule(elasticsearchConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(elasticsearchConfig));
}
}
diff --git a/javatests/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java b/javatests/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java
index d5add7d..e5578e1 100644
--- a/javatests/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java
+++ b/javatests/com/google/gerrit/gpg/GerritPublicKeyCheckerTest.java
@@ -47,7 +47,6 @@
import com.google.gerrit.testing.GerritBaseTests;
import com.google.gerrit.testing.InMemoryDatabase;
import com.google.gerrit.testing.InMemoryModule;
-import com.google.gerrit.testing.NoteDbMode;
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Injector;
@@ -103,8 +102,7 @@
ImmutableList.of(
Fingerprint.toString(keyB().getPublicKey().getFingerprint()),
Fingerprint.toString(keyD().getPublicKey().getFingerprint())));
- Injector injector =
- Guice.createInjector(new InMemoryModule(cfg, NoteDbMode.newNotesMigrationFromEnv()));
+ Injector injector = Guice.createInjector(new InMemoryModule(cfg));
lifecycle = new LifecycleManager();
lifecycle.add(injector);
diff --git a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
index ed4aacb..138635e 100644
--- a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
@@ -161,10 +161,6 @@
bind(GitReferenceUpdated.class).toInstance(GitReferenceUpdated.DISABLED);
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>>() {})
diff --git a/javatests/com/google/gerrit/server/query/account/LuceneQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/LuceneQueryAccountsTest.java
index 660c1d8..e36b79e 100644
--- a/javatests/com/google/gerrit/server/query/account/LuceneQueryAccountsTest.java
+++ b/javatests/com/google/gerrit/server/query/account/LuceneQueryAccountsTest.java
@@ -44,6 +44,6 @@
protected Injector createInjector() {
Config luceneConfig = new Config(config);
InMemoryModule.setDefaults(luceneConfig);
- return Guice.createInjector(new InMemoryModule(luceneConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(luceneConfig));
}
}
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
index 1dfe7df..2ea198f 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
@@ -49,7 +49,7 @@
protected Injector createInjector() {
Config luceneConfig = new Config(config);
InMemoryModule.setDefaults(luceneConfig);
- return Guice.createInjector(new InMemoryModule(luceneConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(luceneConfig));
}
@Test
diff --git a/javatests/com/google/gerrit/server/query/group/LuceneQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/LuceneQueryGroupsTest.java
index 83835c1..2a453a0 100644
--- a/javatests/com/google/gerrit/server/query/group/LuceneQueryGroupsTest.java
+++ b/javatests/com/google/gerrit/server/query/group/LuceneQueryGroupsTest.java
@@ -43,6 +43,6 @@
protected Injector createInjector() {
Config luceneConfig = new Config(config);
InMemoryModule.setDefaults(luceneConfig);
- return Guice.createInjector(new InMemoryModule(luceneConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(luceneConfig));
}
}
diff --git a/javatests/com/google/gerrit/server/query/project/LuceneQueryProjectsTest.java b/javatests/com/google/gerrit/server/query/project/LuceneQueryProjectsTest.java
index 42964fa..77a56ed 100644
--- a/javatests/com/google/gerrit/server/query/project/LuceneQueryProjectsTest.java
+++ b/javatests/com/google/gerrit/server/query/project/LuceneQueryProjectsTest.java
@@ -45,6 +45,6 @@
protected Injector createInjector() {
Config luceneConfig = new Config(config);
InMemoryModule.setDefaults(luceneConfig);
- return Guice.createInjector(new InMemoryModule(luceneConfig, notesMigration));
+ return Guice.createInjector(new InMemoryModule(luceneConfig));
}
}
diff --git a/javatests/com/google/gerrit/server/schema/NoteDbSchemaUpdaterTest.java b/javatests/com/google/gerrit/server/schema/NoteDbSchemaUpdaterTest.java
index 6018d85..eeefd75 100644
--- a/javatests/com/google/gerrit/server/schema/NoteDbSchemaUpdaterTest.java
+++ b/javatests/com/google/gerrit/server/schema/NoteDbSchemaUpdaterTest.java
@@ -29,9 +29,8 @@
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.IntBlob;
-import com.google.gerrit.server.notedb.MutableNotesMigration;
import com.google.gerrit.server.notedb.NoteDbSchemaVersionManager;
-import com.google.gerrit.server.notedb.NotesMigrationState;
+import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.notedb.RepoSequence;
import com.google.gerrit.testing.GerritBaseTests;
import com.google.gerrit.testing.InMemoryRepositoryManager;
@@ -117,8 +116,6 @@
args = new NoteDbSchemaVersion.Arguments(repoManager, allProjectsName);
NoteDbSchemaVersionManager versionManager =
new NoteDbSchemaVersionManager(allProjectsName, repoManager);
- MutableNotesMigration notesMigration = MutableNotesMigration.newDisabled();
- notesMigration.setFrom(NotesMigrationState.NOTE_DB);
updater =
new NoteDbSchemaUpdater(
cfg,
@@ -126,7 +123,7 @@
allUsersName,
repoManager,
schemaCreator,
- notesMigration,
+ new NotesMigration(),
versionManager,
args,
ImmutableSortedMap.of(10, TestSchema_10.class, 11, TestSchema_11.class));
diff --git a/plugins/delete-project b/plugins/delete-project
index 6857771..5f3fe72 160000
--- a/plugins/delete-project
+++ b/plugins/delete-project
@@ -1 +1 @@
-Subproject commit 685777171abb03dec969354fe97b703945f1c39b
+Subproject commit 5f3fe725b6f943f9acf63270cf8a432f9e7fd97a
