From 24b3e672a98df41f85668634abe1fbe01615e1f1 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 28 Jul 2017 12:55:47 -0400 Subject: [PATCH] Fuse code and meta ref updates in NoteDb JGit now supports atomic multi-ref BatchRefUpdates natively in FileRepository, so we can eliminate the distinction between fused and unfused NoteDb modes. Now, disableChangeReviewDb=true means NoteDb is fully enabled. Change-Id: I085502687c5e9a1f426c55334fe8bf59fa36d9e8 --- .../gerrit/acceptance/GerritServer.java | 5 - ...bmoduleSubscriptionsWholeTopicMergeIT.java | 2 +- .../pgm/StandaloneNoteDbMigrationIT.java | 6 +- .../rest/change/AbstractSubmit.java | 4 +- .../server/notedb/NoteDbOnlyIT.java | 51 +- .../notedb/OnlineNoteDbMigrationIT.java | 6 +- .../com/google/gerrit/server/git/MergeOp.java | 11 +- .../server/notedb/MutableNotesMigration.java | 4 - .../gerrit/server/notedb/NotesMigration.java | 22 - .../server/notedb/NotesMigrationState.java | 25 +- .../server/notedb/rebuild/NoteDbMigrator.java | 13 +- .../gerrit/server/update/BatchUpdate.java | 29 +- ...atchUpdate.java => NoteDbBatchUpdate.java} | 22 +- .../gerrit/server/update/RetryHelper.java | 11 +- .../update/UnfusedNoteDbBatchUpdate.java | 459 ------------------ .../testutil/InMemoryRepositoryManager.java | 23 +- .../google/gerrit/testutil/NoteDbMode.java | 6 +- 17 files changed, 57 insertions(+), 642 deletions(-) rename gerrit-server/src/main/java/com/google/gerrit/server/update/{FusedNoteDbBatchUpdate.java => NoteDbBatchUpdate.java} (96%) delete mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java index 551777a14e..256be820b6 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java @@ -16,7 +16,6 @@ package com.google.gerrit.acceptance; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; -import static com.google.common.truth.TruthJUnit.assume; import com.google.auto.value.AutoValue; import com.google.common.base.MoreObjects; @@ -208,10 +207,6 @@ public class GerritServer implements AutoCloseable { */ public static void init(Description desc, Config baseConfig, Path site) throws Exception { checkArgument(!desc.memory(), "can't initialize site path for in-memory test: %s", desc); - assume() - .withMessage("FUSED mode not yet supported for on-disk sites") - .that(NoteDbMode.get()) - .isNotEqualTo(NoteDbMode.FUSED); Config cfg = desc.buildConfig(baseConfig); Map pluginConfigs = desc.buildPluginConfigs(); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java index d64d67f1a4..8e3aeaff1c 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java @@ -748,7 +748,7 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT extends AbstractSubmoduleSu @Test public void retrySubmitAfterTornTopicOnLockFailure() throws Exception { - assume().that(notesMigration.fuseUpdates()).isTrue(); + assume().that(notesMigration.disableChangeReviewDb()).isTrue(); TestRepository superRepo = createProjectWithPush("super-project"); TestRepository sub1 = createProjectWithPush("sub1"); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java index 29a9a1f438..281e6cdf54 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/StandaloneNoteDbMigrationIT.java @@ -105,7 +105,7 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest { setUpOneChange(); migrate("--trial", "false"); - assertNotesMigrationState(NotesMigrationState.NOTE_DB_UNFUSED); + assertNotesMigrationState(NotesMigrationState.NOTE_DB); try (ServerContext ctx = startServer()) { GitRepositoryManager repoManager = ctx.getInjector().getInstance(GitRepositoryManager.class); @@ -143,7 +143,7 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest { assertServerStartupFails(); migrate("--trial", "false"); - assertNotesMigrationState(NotesMigrationState.NOTE_DB_UNFUSED); + assertNotesMigrationState(NotesMigrationState.NOTE_DB); status = new GerritIndexStatus(sitePaths); assertThat(status.getReady(ChangeSchemaDefinitions.NAME, version)).isTrue(); @@ -175,7 +175,7 @@ public class StandaloneNoteDbMigrationIT extends StandaloneSiteTest { u.runUpgrades(); assertThat(indexes.getSearchIndex().getSchema().getVersion()).isEqualTo(currVersion); - assertNotesMigrationState(NotesMigrationState.NOTE_DB_UNFUSED); + assertNotesMigrationState(NotesMigrationState.NOTE_DB); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 77b70d3b45..6cbc5329f4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -916,7 +916,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { @Test public void retrySubmitSingleChangeOnLockFailure() throws Exception { - assume().that(notesMigration.fuseUpdates()).isTrue(); + assume().that(notesMigration.disableChangeReviewDb()).isTrue(); PushOneCommit.Result change = createChange(); String id = change.getChangeId(); @@ -943,7 +943,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { @Test public void retrySubmitAfterTornTopicOnLockFailure() throws Exception { - assume().that(notesMigration.fuseUpdates()).isTrue(); + assume().that(notesMigration.disableChangeReviewDb()).isTrue(); assume().that(isSubmitWholeTopicEnabled()).isTrue(); String topic = "test-topic"; diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java index 9efbe3607f..2cd1800218 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java @@ -28,7 +28,6 @@ import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.restapi.ResourceConflictException; -import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateListener; @@ -77,7 +76,7 @@ public class NoteDbOnlyIT extends AbstractDaemonTest { @Test public void updateChangeFailureRollsBackRefUpdate() throws Exception { - assume().that(notesMigration.fuseUpdates()).isTrue(); + assume().that(notesMigration.disableChangeReviewDb()).isTrue(); PushOneCommit.Result r = createChange(); Change.Id id = r.getChange().getId(); @@ -149,7 +148,7 @@ public class NoteDbOnlyIT extends AbstractDaemonTest { @Test public void retryOnLockFailureWithAtomicUpdates() throws Exception { - assume().that(notesMigration.fuseUpdates()).isTrue(); + assume().that(notesMigration.disableChangeReviewDb()).isTrue(); PushOneCommit.Result r = createChange(); Change.Id id = r.getChange().getId(); String master = "refs/heads/master"; @@ -195,52 +194,6 @@ public class NoteDbOnlyIT extends AbstractDaemonTest { } } - @Test - public void noRetryOnLockFailureWithoutAtomicUpdates() throws Exception { - assume().that(notesMigration.fuseUpdates()).isFalse(); - - PushOneCommit.Result r = createChange(); - Change.Id id = r.getChange().getId(); - String master = "refs/heads/master"; - ObjectId initial; - try (Repository repo = repoManager.openRepository(project)) { - initial = repo.exactRef(master).getObjectId(); - } - - AtomicInteger updateRepoCalledCount = new AtomicInteger(); - AtomicInteger updateChangeCalledCount = new AtomicInteger(); - AtomicInteger afterUpdateReposCalledCount = new AtomicInteger(); - - try { - retryHelper.execute( - batchUpdateFactory -> { - try (BatchUpdate bu = newBatchUpdate(batchUpdateFactory)) { - bu.addOp( - id, new UpdateRefAndAddMessageOp(updateRepoCalledCount, updateChangeCalledCount)); - bu.execute(new ConcurrentWritingListener(afterUpdateReposCalledCount)); - } - return null; - }); - assert_().fail("expected RestApiException"); - } catch (RestApiException e) { - // Expected. - } - - assertThat(updateRepoCalledCount.get()).isEqualTo(1); - assertThat(afterUpdateReposCalledCount.get()).isEqualTo(1); - assertThat(updateChangeCalledCount.get()).isEqualTo(0); - - // updateChange was never called, so no message was ever added. - assertThat(getMessages(id)).doesNotContain(UpdateRefAndAddMessageOp.CHANGE_MESSAGE); - - try (Repository repo = repoManager.openRepository(project)) { - // Op lost the race, so the other writer's commit happened first. Op didn't retry, because the - // ref updates weren't atomic, so it didn't throw LockFailureException on failure. - assertThat(commitMessages(repo, initial, repo.exactRef(master).getObjectId())) - .containsExactly(ConcurrentWritingListener.MSG_PREFIX + "1"); - } - } - private class ConcurrentWritingListener implements BatchUpdateListener { static final String MSG_PREFIX = "Other writer "; diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java index 485bb1b5a3..f08a16ef24 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/OnlineNoteDbMigrationIT.java @@ -17,7 +17,7 @@ package com.google.gerrit.acceptance.server.notedb; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static com.google.common.truth.TruthJUnit.assume; -import static com.google.gerrit.server.notedb.NotesMigrationState.NOTE_DB_UNFUSED; +import static com.google.gerrit.server.notedb.NotesMigrationState.NOTE_DB; import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_NO_SEQUENCE; import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY; import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_WITH_SEQUENCE_REVIEW_DB_PRIMARY; @@ -313,7 +313,7 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest { Change.Id id2 = r2.getChange().getId(); migrate(b -> b.setThreads(threads)); - assertNotesMigrationState(NOTE_DB_UNFUSED); + assertNotesMigrationState(NOTE_DB); assertThat(sequences.nextChangeId()).isEqualTo(503); @@ -372,7 +372,7 @@ public class OnlineNoteDbMigrationIT extends AbstractDaemonTest { assertThat(NoteDbMigrator.getAutoMigrate(gerritConfig)).isTrue(); migrate(b -> b); - assertNotesMigrationState(NOTE_DB_UNFUSED); + assertNotesMigrationState(NOTE_DB); assertThat(NoteDbMigrator.getAutoMigrate(gerritConfig)).isFalse(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index ff5c5ed214..ea28fa94af 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -63,7 +63,6 @@ import com.google.gerrit.server.git.strategy.SubmitStrategyFactory; import com.google.gerrit.server.git.strategy.SubmitStrategyListener; import com.google.gerrit.server.git.validators.MergeValidationException; import com.google.gerrit.server.git.validators.MergeValidators; -import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.SubmitRuleOptions; @@ -234,7 +233,6 @@ public class MergeOp implements AutoCloseable { private final Provider ormProvider; private final NotifyUtil notifyUtil; private final RetryHelper retryHelper; - private final NotesMigration notesMigration; private Timestamp ts; private RequestId submissionId; @@ -262,8 +260,7 @@ public class MergeOp implements AutoCloseable { Provider ormProvider, NotifyUtil notifyUtil, TopicMetrics topicMetrics, - RetryHelper retryHelper, - NotesMigration notesMigration) { + RetryHelper retryHelper) { this.cmUtil = cmUtil; this.batchUpdateFactory = batchUpdateFactory; this.internalUserFactory = internalUserFactory; @@ -276,7 +273,6 @@ public class MergeOp implements AutoCloseable { this.notifyUtil = notifyUtil; this.retryHelper = retryHelper; this.topicMetrics = topicMetrics; - this.notesMigration = notesMigration; } @Override @@ -909,11 +905,6 @@ public class MergeOp implements AutoCloseable { return "Error submitting changes"; } if (p == 1) { - if (!notesMigration.fuseUpdates()) { - // No fused updates: any subset of changes might or might not have been submitted, so don't - // make any strong claims. - return "Error submitting changes"; - } // Fused updates: it's correct to say that none of the n changes were submitted. return "Error submitting " + c + " changes"; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/MutableNotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/MutableNotesMigration.java index e16763536e..7f4912b125 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/MutableNotesMigration.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/MutableNotesMigration.java @@ -72,10 +72,6 @@ public class MutableNotesMigration extends NotesMigration { return set(b -> b.setDisableChangeReviewDb(disableChangeReviewDb)); } - public MutableNotesMigration setFuseUpdates(boolean fuseUpdates) { - return set(b -> b.setFuseUpdates(fuseUpdates)); - } - public MutableNotesMigration setFailOnLoadForTest(boolean failOnLoadForTest) { return set(b -> b.setFailOnLoadForTest(failOnLoadForTest)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java index 9c7029cf80..e560ec8a00 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java @@ -56,7 +56,6 @@ public abstract class NotesMigration { public static final String SECTION_NOTE_DB = "noteDb"; private static final String DISABLE_REVIEW_DB = "disableReviewDb"; - private static final String FUSE_UPDATES = "fuseUpdates"; private static final String PRIMARY_STORAGE = "primaryStorage"; private static final String READ = "read"; private static final String SEQUENCE = "sequence"; @@ -87,7 +86,6 @@ public abstract class NotesMigration { SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB)) .setDisableChangeReviewDb( cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, false)) - .setFuseUpdates(cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false)) .setFailOnLoadForTest(false) // Only set in tests, can't be set via config. .build(); } @@ -102,8 +100,6 @@ public abstract class NotesMigration { abstract boolean disableChangeReviewDb(); - abstract boolean fuseUpdates(); - abstract boolean failOnLoadForTest(); abstract Builder toBuilder(); @@ -114,7 +110,6 @@ public abstract class NotesMigration { cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, readChangeSequence()); cfg.setEnum(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, changePrimaryStorage()); cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, disableChangeReviewDb()); - cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, fuseUpdates()); } @AutoValue.Builder @@ -129,8 +124,6 @@ public abstract class NotesMigration { abstract Builder setDisableChangeReviewDb(boolean disableChangeReviewDb); - abstract Builder setFuseUpdates(boolean fuseUpdates); - abstract Builder setFailOnLoadForTest(boolean failOnLoadForTest); abstract Snapshot autoBuild(); @@ -205,21 +198,6 @@ public abstract class NotesMigration { return snapshot.get().disableChangeReviewDb(); } - /** - * Fuse meta ref updates in the same batch as code updates. - * - *

When set, each {@link com.google.gerrit.server.update.BatchUpdate} results in a single - * {@link org.eclipse.jgit.lib.BatchRefUpdate} to update both code and meta refs atomically. - * Setting this option with a repository backend that does not support atomic multi-ref - * transactions ({@link org.eclipse.jgit.lib.RefDatabase#performsAtomicTransactions()}) is a - * configuration error, and all updates will fail at runtime. - * - *

Has no effect if {@link #disableChangeReviewDb()} is false. - */ - public final boolean fuseUpdates() { - return snapshot.get().fuseUpdates(); - } - /** * Whether to fail when reading any data from NoteDb. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigrationState.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigrationState.java index 2469574db8..c682aed8c2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigrationState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigrationState.java @@ -31,26 +31,19 @@ import org.eclipse.jgit.lib.Config; * in the order in which they are defined. */ public enum NotesMigrationState { - REVIEW_DB(false, false, false, PrimaryStorage.REVIEW_DB, false, false), + REVIEW_DB(false, false, false, PrimaryStorage.REVIEW_DB, false), - WRITE(false, true, false, PrimaryStorage.REVIEW_DB, false, false), + WRITE(false, true, false, PrimaryStorage.REVIEW_DB, false), - READ_WRITE_NO_SEQUENCE(true, true, false, PrimaryStorage.REVIEW_DB, false, 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, 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, false), + READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY(true, true, true, PrimaryStorage.NOTE_DB, false), - // TODO(dborowitz): This only exists as a separate state to support testing in different - // NoteDbModes. Once FileRepository fuses BatchRefUpdates, we won't have separate fused/unfused - // states. - NOTE_DB_UNFUSED(true, true, true, PrimaryStorage.NOTE_DB, true, false), + NOTE_DB(true, true, true, PrimaryStorage.NOTE_DB, true); - NOTE_DB(true, true, true, PrimaryStorage.NOTE_DB, true, true); - - // TODO(dborowitz): Replace with NOTE_DB when FileRepository fuses BatchRefUpdates. - public static final NotesMigrationState FINAL = NOTE_DB_UNFUSED; + public static final NotesMigrationState FINAL = NOTE_DB; public static Optional forConfig(Config cfg) { return forSnapshot(Snapshot.create(cfg)); @@ -72,8 +65,7 @@ public enum NotesMigrationState { boolean rawWriteChangesSetting, boolean readChangeSequence, PrimaryStorage changePrimaryStorage, - boolean disableChangeReviewDb, - boolean fuseUpdates) { + boolean disableChangeReviewDb) { this.snapshot = Snapshot.builder() .setReadChanges(readChanges) @@ -81,7 +73,6 @@ public enum NotesMigrationState { .setReadChangeSequence(readChangeSequence) .setChangePrimaryStorage(changePrimaryStorage) .setDisableChangeReviewDb(disableChangeReviewDb) - .setFuseUpdates(fuseUpdates) .build(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java index 0a7629481c..7323c2e0d5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/NoteDbMigrator.java @@ -19,7 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.reviewdb.server.ReviewDbUtil.unwrapDb; import static com.google.gerrit.server.notedb.NotesMigration.SECTION_NOTE_DB; -import static com.google.gerrit.server.notedb.NotesMigrationState.NOTE_DB_UNFUSED; +import static com.google.gerrit.server.notedb.NotesMigrationState.NOTE_DB; import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_NO_SEQUENCE; import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY; import static com.google.gerrit.server.notedb.NotesMigrationState.READ_WRITE_WITH_SEQUENCE_REVIEW_DB_PRIMARY; @@ -419,7 +419,7 @@ public class NoteDbMigrator implements AutoCloseable { } boolean rebuilt = false; - while (state.compareTo(NOTE_DB_UNFUSED) < 0) { + while (state.compareTo(NOTE_DB) < 0) { if (state.equals(stopAtState)) { return; } @@ -458,18 +458,15 @@ public class NoteDbMigrator implements AutoCloseable { break; case READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY: // The only way we can get here is if there was a failure on a previous run of - // setNoteDbPrimary, since that method moves to NOTE_DB_UNFUSED if it completes + // setNoteDbPrimary, since that method moves to NOTE_DB if it completes // successfully. Assume that not all changes were converted and re-run the step. // migrateToNoteDbPrimary is a relatively fast no-op for already-migrated changes, so this // isn't actually repeating work. state = setNoteDbPrimary(state); break; - case NOTE_DB_UNFUSED: + case NOTE_DB: // Done! break; - case NOTE_DB: - // TODO(dborowitz): Allow this state once FileRepository supports fused updates. - // Until then, fallthrough and throw. default: throw new MigrationException( "Migration out of the following state is not supported:\n" + state.toText()); @@ -561,7 +558,7 @@ public class NoteDbMigrator implements AutoCloseable { } private NotesMigrationState disableReviewDb(NotesMigrationState prev) throws IOException { - return saveState(prev, NOTE_DB_UNFUSED, c -> setAutoMigrate(c, false)); + return saveState(prev, NOTE_DB, c -> setAutoMigrate(c, false)); } private Optional loadState() throws IOException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java index fdae8e93a5..cf88be0c8a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java @@ -95,8 +95,7 @@ public abstract class BatchUpdate implements AutoCloseable { @Override public void configure() { factory(ReviewDbBatchUpdate.AssistedFactory.class); - factory(FusedNoteDbBatchUpdate.AssistedFactory.class); - factory(UnfusedNoteDbBatchUpdate.AssistedFactory.class); + factory(NoteDbBatchUpdate.AssistedFactory.class); } }; } @@ -105,29 +104,23 @@ public abstract class BatchUpdate implements AutoCloseable { public static class Factory { private final NotesMigration migration; private final ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory; - private final FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory; - private final UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory; + private final NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory; // TODO(dborowitz): Make this non-injectable to force all callers to use RetryHelper. @Inject Factory( NotesMigration migration, ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory, - FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory, - UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory) { + NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory) { this.migration = migration; this.reviewDbBatchUpdateFactory = reviewDbBatchUpdateFactory; - this.fusedNoteDbBatchUpdateFactory = fusedNoteDbBatchUpdateFactory; - this.unfusedNoteDbBatchUpdateFactory = unfusedNoteDbBatchUpdateFactory; + this.noteDbBatchUpdateFactory = noteDbBatchUpdateFactory; } public BatchUpdate create( ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when) { if (migration.disableChangeReviewDb()) { - if (migration.fuseUpdates()) { - return fusedNoteDbBatchUpdateFactory.create(db, project, user, when); - } - return unfusedNoteDbBatchUpdateFactory.create(db, project, user, when); + return noteDbBatchUpdateFactory.create(db, project, user, when); } return reviewDbBatchUpdateFactory.create(db, project, user, when); } @@ -147,15 +140,9 @@ public abstract class BatchUpdate implements AutoCloseable { // copy them into an ImmutableList so there is no chance the callee can pollute the input // collection. if (migration.disableChangeReviewDb()) { - if (migration.fuseUpdates()) { - ImmutableList noteDbUpdates = - (ImmutableList) ImmutableList.copyOf(updates); - FusedNoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun); - } else { - ImmutableList noteDbUpdates = - (ImmutableList) ImmutableList.copyOf(updates); - UnfusedNoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun); - } + ImmutableList noteDbUpdates = + (ImmutableList) ImmutableList.copyOf(updates); + NoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun); } else { ImmutableList reviewDbUpdates = (ImmutableList) ImmutableList.copyOf(updates); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java similarity index 96% rename from gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java rename to gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java index 7db5e43e18..fe9575e3da 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java @@ -62,14 +62,14 @@ import org.eclipse.jgit.transport.ReceiveCommand; *

Used when {@code noteDb.changes.disableReviewDb=true}, at which point ReviewDb is not * consulted during updates. */ -class FusedNoteDbBatchUpdate extends BatchUpdate { +class NoteDbBatchUpdate extends BatchUpdate { interface AssistedFactory { - FusedNoteDbBatchUpdate create( + NoteDbBatchUpdate create( ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when); } static void execute( - ImmutableList updates, + ImmutableList updates, BatchUpdateListener listener, @Nullable RequestId requestId, boolean dryrun) @@ -88,11 +88,11 @@ class FusedNoteDbBatchUpdate extends BatchUpdate { try { switch (order) { case REPO_BEFORE_DB: - for (FusedNoteDbBatchUpdate u : updates) { + for (NoteDbBatchUpdate u : updates) { u.executeUpdateRepo(); } listener.afterUpdateRepos(); - for (FusedNoteDbBatchUpdate u : updates) { + for (NoteDbBatchUpdate u : updates) { handles.add(u.executeChangeOps(dryrun)); } for (ChangesHandle h : handles) { @@ -113,10 +113,10 @@ class FusedNoteDbBatchUpdate extends BatchUpdate { // 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 (FusedNoteDbBatchUpdate u : updates) { + for (NoteDbBatchUpdate u : updates) { handles.add(u.executeChangeOps(dryrun)); } - for (FusedNoteDbBatchUpdate u : updates) { + for (NoteDbBatchUpdate u : updates) { u.executeUpdateRepo(); } for (ChangesHandle h : handles) { @@ -151,7 +151,7 @@ class FusedNoteDbBatchUpdate extends BatchUpdate { u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null))); if (!dryrun) { - for (FusedNoteDbBatchUpdate u : updates) { + for (NoteDbBatchUpdate u : updates) { u.executePostOps(); } } @@ -163,7 +163,7 @@ class FusedNoteDbBatchUpdate extends BatchUpdate { class ContextImpl implements Context { @Override public RepoView getRepoView() throws IOException { - return FusedNoteDbBatchUpdate.this.getRepoView(); + return NoteDbBatchUpdate.this.getRepoView(); } @Override @@ -272,7 +272,7 @@ class FusedNoteDbBatchUpdate extends BatchUpdate { private final ReviewDb db; @Inject - FusedNoteDbBatchUpdate( + NoteDbBatchUpdate( GitRepositoryManager repoManager, @GerritPersonIdent PersonIdent serverIdent, ChangeNotes.Factory changeNotesFactory, @@ -354,7 +354,7 @@ class FusedNoteDbBatchUpdate extends BatchUpdate { } void execute() throws OrmException, IOException { - FusedNoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun); + NoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun); } @SuppressWarnings("deprecation") diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java index 25a4e3c53d..7a1de31c84 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java @@ -51,15 +51,10 @@ public class RetryHelper { @GerritServerConfig Config cfg, NotesMigration migration, ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory, - FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory, - UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory) { + NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory) { this.migration = migration; this.updateFactory = - new BatchUpdate.Factory( - migration, - reviewDbBatchUpdateFactory, - fusedNoteDbBatchUpdateFactory, - unfusedNoteDbBatchUpdateFactory); + new BatchUpdate.Factory(migration, reviewDbBatchUpdateFactory, noteDbBatchUpdateFactory); this.stopStrategy = StopStrategies.stopAfterDelay( cfg.getTimeUnit("noteDb", null, "retryTimeout", SECONDS.toMillis(5), MILLISECONDS), @@ -80,7 +75,7 @@ public class RetryHelper { throws RestApiException, UpdateException { try { RetryerBuilder builder = RetryerBuilder.newBuilder(); - if (migration.disableChangeReviewDb() && migration.fuseUpdates()) { + if (migration.disableChangeReviewDb()) { builder .withStopStrategy(stopStrategy) .withWaitStrategy(waitStrategy) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java deleted file mode 100644 index ce96c0edb3..0000000000 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java +++ /dev/null @@ -1,459 +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.checkNotNull; -import static java.util.Comparator.comparing; -import static java.util.stream.Collectors.toList; - -import com.google.common.base.Throwables; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.Maps; -import com.google.gerrit.common.Nullable; -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.gerrit.server.project.ChangeControl; -import com.google.gerrit.server.util.RequestId; -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.List; -import java.util.Map; -import java.util.TimeZone; -import java.util.TreeMap; -import org.eclipse.jgit.lib.NullProgressMonitor; -import org.eclipse.jgit.lib.ObjectInserter; -import org.eclipse.jgit.lib.ObjectReader; -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 that only supports NoteDb. - * - *

Used when {@code noteDb.changes.disableReviewDb=true}, at which point ReviewDb is not - * consulted during updates. - */ -class UnfusedNoteDbBatchUpdate extends BatchUpdate { - interface AssistedFactory { - UnfusedNoteDbBatchUpdate create( - ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when); - } - - static void execute( - ImmutableList updates, - BatchUpdateListener listener, - @Nullable RequestId requestId, - boolean dryrun) - throws UpdateException, RestApiException { - if (updates.isEmpty()) { - return; - } - setRequestIds(updates, requestId); - - try { - Order order = getOrder(updates, listener); - // TODO(dborowitz): Fuse implementations to use a single BatchRefUpdate between phases. Note - // that we may still need to respect the order, since op implementations may make assumptions - // about the order in which their methods are called. - switch (order) { - case REPO_BEFORE_DB: - for (UnfusedNoteDbBatchUpdate u : updates) { - u.executeUpdateRepo(); - } - listener.afterUpdateRepos(); - for (UnfusedNoteDbBatchUpdate u : updates) { - u.executeRefUpdates(dryrun); - } - listener.afterUpdateRefs(); - for (UnfusedNoteDbBatchUpdate u : updates) { - u.reindexChanges(u.executeChangeOps(dryrun), dryrun); - } - listener.afterUpdateChanges(); - break; - case DB_BEFORE_REPO: - for (UnfusedNoteDbBatchUpdate u : updates) { - u.reindexChanges(u.executeChangeOps(dryrun), dryrun); - } - for (UnfusedNoteDbBatchUpdate u : updates) { - u.executeUpdateRepo(); - } - for (UnfusedNoteDbBatchUpdate u : updates) { - u.executeRefUpdates(dryrun); - } - break; - default: - throw new IllegalStateException("invalid execution order: " + order); - } - - ChangeIndexer.allAsList( - updates.stream().flatMap(u -> u.indexFutures.stream()).collect(toList())) - .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 (UnfusedNoteDbBatchUpdate u : updates) { - u.executePostOps(); - } - } - } catch (Exception e) { - wrapAndThrowException(e); - } - } - - class ContextImpl implements Context { - @Override - public RepoView getRepoView() throws IOException { - return UnfusedNoteDbBatchUpdate.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 ChangeControl ctl; - private final Map updates; - - private boolean deleted; - - protected ChangeContextImpl(ChangeControl ctl) { - this.ctl = checkNotNull(ctl); - 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(ctl, when); - if (newChanges.containsKey(ctl.getId())) { - u.setAllowWriteToNewRef(true); - } - u.setPatchSetId(psId); - updates.put(psId, u); - } - return u; - } - - @Override - public ChangeControl getControl() { - return ctl; - } - - @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 ChangeControl.GenericFactory changeControlFactory; - private final ChangeUpdate.Factory changeUpdateFactory; - private final NoteDbUpdateManager.Factory updateManagerFactory; - private final ChangeIndexer indexer; - private final GitReferenceUpdated gitRefUpdated; - private final ReviewDb db; - - @SuppressWarnings("deprecation") - private List> indexFutures; - - @Inject - UnfusedNoteDbBatchUpdate( - GitRepositoryManager repoManager, - @GerritPersonIdent PersonIdent serverIdent, - ChangeNotes.Factory changeNotesFactory, - ChangeControl.GenericFactory changeControlFactory, - 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.changeControlFactory = changeControlFactory; - this.changeUpdateFactory = changeUpdateFactory; - this.updateManagerFactory = updateManagerFactory; - this.indexer = indexer; - this.gitRefUpdated = gitRefUpdated; - this.db = db; - this.indexFutures = new ArrayList<>(); - } - - @Override - public void execute(BatchUpdateListener listener) throws UpdateException, RestApiException { - execute(ImmutableList.of(this), listener, requestId, false); - } - - @Override - protected Context newContext() { - return new ContextImpl(); - } - - private void executeUpdateRepo() throws UpdateException, RestApiException { - try { - logDebug("Executing updateRepo on {} ops", ops.size()); - RepoContextImpl ctx = new RepoContextImpl(); - for (BatchUpdateOp op : ops.values()) { - op.updateRepo(ctx); - } - - logDebug("Executing updateRepo on {} 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()); - } - - // TODO(dborowitz): Don't flush when fusing phases. - if (repoView != null) { - logDebug("Flushing inserter"); - repoView.getInserter().flush(); - } else { - logDebug("No objects to flush"); - } - } catch (Exception e) { - Throwables.throwIfInstanceOf(e, RestApiException.class); - throw new UpdateException(e); - } - } - - // TODO(dborowitz): Don't execute non-change ref updates separately when fusing phases. - private void executeRefUpdates(boolean dryrun) throws IOException, RestApiException { - if (getRefUpdates().isEmpty()) { - logDebug("No ref updates to execute"); - return; - } - // May not be opened if the caller added ref updates but no new objects. - initRepository(); - batchRefUpdate = repoView.getRepository().getRefDatabase().newBatchUpdate(); - batchRefUpdate.setPushCertificate(pushCert); - batchRefUpdate.setRefLogMessage(refLogMessage, true); - batchRefUpdate.setAllowNonFastForwards(true); - repoView.getCommands().addTo(batchRefUpdate); - logDebug("Executing batch of {} ref updates", batchRefUpdate.getCommands().size()); - if (dryrun) { - return; - } - - // Force BatchRefUpdate to read newly referenced objects using a new RevWalk, rather than one - // that might have access to unflushed objects. - try (RevWalk updateRw = new RevWalk(repoView.getRepository())) { - batchRefUpdate.execute(updateRw, NullProgressMonitor.INSTANCE); - } - boolean ok = true; - for (ReceiveCommand cmd : batchRefUpdate.getCommands()) { - if (cmd.getResult() != ReceiveCommand.Result.OK) { - ok = false; - break; - } - } - if (!ok) { - throw new RestApiException("BatchRefUpdate failed: " + batchRefUpdate); - } - } - - private Map executeChangeOps(boolean dryrun) throws Exception { - logDebug("Executing change ops"); - Map result = - Maps.newLinkedHashMapWithExpectedSize(ops.keySet().size()); - initRepository(); - Repository repo = repoView.getRepository(); - // TODO(dborowitz): Teach NoteDbUpdateManager to allow reusing the same inserter and batch ref - // update as in executeUpdateRepo. - try (ObjectInserter ins = repo.newObjectInserter(); - ObjectReader reader = ins.newReader(); - RevWalk rw = new RevWalk(reader); - NoteDbUpdateManager updateManager = - updateManagerFactory - .create(project) - .setChangeRepo(repo, rw, ins, new ChainedReceiveCommands(repo))) { - if (user.isIdentifiedUser()) { - updateManager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz)); - } - for (Map.Entry> e : ops.asMap().entrySet()) { - Change.Id id = e.getKey(); - ChangeContextImpl ctx = newChangeContext(id); - boolean dirty = false; - logDebug("Applying {} ops for change {}", e.getValue().size(), id); - for (BatchUpdateOp op : e.getValue()) { - dirty |= op.updateChange(ctx); - } - if (!dirty) { - logDebug("No ops reported dirty, short-circuiting"); - result.put(id, ChangeResult.SKIPPED); - continue; - } - for (ChangeUpdate u : ctx.updates.values()) { - updateManager.add(u); - } - if (ctx.deleted) { - logDebug("Change {} was deleted", id); - updateManager.deleteChange(id); - result.put(id, ChangeResult.DELETED); - } else { - result.put(id, ChangeResult.UPSERTED); - } - } - - if (!dryrun) { - logDebug("Executing NoteDb updates"); - updateManager.execute(); - } - } - return result; - } - - private ChangeContextImpl newChangeContext(Change.Id id) throws OrmException { - logDebug("Opening change {} 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 {} is new", id); - } - ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew); - ChangeControl ctl = changeControlFactory.controlFor(notes, user); - return new ChangeContextImpl(ctl); - } - - private void reindexChanges(Map updateResults, boolean dryrun) { - if (dryrun) { - return; - } - logDebug("Reindexing {} changes", updateResults.size()); - for (Map.Entry e : updateResults.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()); - } - } - } - - 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/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryRepositoryManager.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryRepositoryManager.java index 0e7201a6d7..e0c51b74f8 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryRepositoryManager.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/InMemoryRepositoryManager.java @@ -19,7 +19,6 @@ import com.google.common.collect.Sets; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.RepositoryCaseMismatchException; -import com.google.gerrit.server.notedb.NotesMigration; import com.google.inject.Inject; import java.util.HashMap; import java.util.Map; @@ -32,7 +31,7 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; /** Repository manager that uses in-memory repositories. */ public class InMemoryRepositoryManager implements GitRepositoryManager { public static InMemoryRepository newRepository(Project.NameKey name) { - return new Repo(NoteDbMode.newNotesMigrationFromEnv(), name); + return new Repo(name); } public static class Description extends DfsRepositoryDescription { @@ -51,15 +50,9 @@ public class InMemoryRepositoryManager implements GitRepositoryManager { public static class Repo extends InMemoryRepository { private String description; - private Repo(NotesMigration notesMigration, Project.NameKey name) { + private Repo(Project.NameKey name) { super(new Description(name)); - // Normally, mimic the behavior of JGit FileRepository, the standard Gerrit repository - // backend, and don't support atomic ref updates. The exception is when we're testing with - // fused ref updates, which requires atomic ref updates to function. - // - // TODO(dborowitz): Change to match the behavior of JGit FileRepository after fixing - // https://bugs.eclipse.org/bugs/show_bug.cgi?id=515678 - setPerformsAtomicTransactions(notesMigration.fuseUpdates()); + setPerformsAtomicTransactions(true); } @Override @@ -78,16 +71,10 @@ public class InMemoryRepositoryManager implements GitRepositoryManager { } } - private final NotesMigration notesMigration; private final Map repos; - public InMemoryRepositoryManager() { - this(NoteDbMode.newNotesMigrationFromEnv()); - } - @Inject - InMemoryRepositoryManager(NotesMigration notesMigration) { - this.notesMigration = notesMigration; + public InMemoryRepositoryManager() { this.repos = new HashMap<>(); } @@ -106,7 +93,7 @@ public class InMemoryRepositoryManager implements GitRepositoryManager { throw new RepositoryCaseMismatchException(name); } } catch (RepositoryNotFoundException e) { - repo = new Repo(notesMigration, name); + repo = new Repo(name); repos.put(normalize(name), repo); } return repo; diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java index 7c19191094..b51a011f1f 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java @@ -35,11 +35,15 @@ public enum NoteDbMode { PRIMARY(NotesMigrationState.READ_WRITE_WITH_SEQUENCE_NOTE_DB_PRIMARY), /** All change tables are entirely disabled. */ - DISABLE_CHANGE_REVIEW_DB(NotesMigrationState.NOTE_DB_UNFUSED), + DISABLE_CHANGE_REVIEW_DB(NotesMigrationState.NOTE_DB), /** All change tables are entirely disabled, and code/meta ref updates are fused. */ FUSED(NotesMigrationState.NOTE_DB), + // TODO(dborowitz): Change CI to use this, then remove FUSED and DISABLE_CHANGE_REVIEW_DB. + /** All change tables are entirely disabled, and code/meta ref updates are fused. */ + ON(NotesMigrationState.NOTE_DB), + /** * Run tests with NoteDb disabled, then convert ReviewDb to NoteDb and check that the results * match.