diff --git a/Documentation/dev-note-db.txt b/Documentation/dev-note-db.txt index 0f6873cd61..e84fecfea1 100644 --- a/Documentation/dev-note-db.txt +++ b/Documentation/dev-note-db.txt @@ -32,8 +32,8 @@ same repository as code changes. - Storing the rest of account data is a work in progress. - Storing group data is a work in progress. -To match the current configuration of `googlesource.com`, paste the following -config snippet in your `gerrit.config`: +To use a configuration similar to the current state of `googlesource.com`, paste +the following config snippet in your `gerrit.config`: ---- [noteDb "changes"] @@ -46,6 +46,9 @@ config snippet in your `gerrit.config`: This is the configuration that will be produced if you enable experimental NoteDb support on a new site with `init`. +`googlesource.com` additionally uses `fuseUpdates = true`, because its repo +backend supports atomic multi-ref transactions. Native JGit doesn't, so setting +this option on a dev server would fail. For an example NoteDb change, poke around at this one: ---- @@ -99,6 +102,11 @@ previous options, unless otherwise noted. implementation of the `rebuild-note-db` program does not do this. + In this phase, it would be possible to delete the Changes tables out from under a running server with no effect. +- `noteDb.changes.fuseUpdates=true`: Code and meta updates within a single + repository are fused into a single atomic `BatchRefUpdate`, so they either + all succeed or all fail. This mode is only possible on a backend that + supports atomic ref updates, which notably excludes the default file repository + backend. [[migration]] == Migration diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 42b10148bf..d3e084a492 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -348,7 +348,6 @@ public abstract class AbstractDaemonTest { } server.getTestInjector().injectMembers(this); - notesMigration.setFromEnv(); Transport.register(inProcessProtocol); toClose = Collections.synchronizedList(new ArrayList()); admin = accounts.admin(); @@ -528,6 +527,7 @@ public abstract class AbstractDaemonTest { server.stop(); server = null; } + notesMigration.resetFromEnv(); } protected TestRepository.CommitBuilder commitBuilder() throws Exception { diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 0ed5d8d25a..f99c35a04a 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -896,7 +896,14 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { PushResult pr = GitUtil.pushHead(testRepo, "refs/for/foo%base=" + rBase.getCommit().name(), false, false); - assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done"); + + if (notesMigration.fuseUpdates()) { + // InMemoryRepository's atomic BatchRefUpdate implementation doesn't update the progress + // monitor. That's fine, we just care that there was at least one new change and no errors. + assertThat(pr.getMessages()).contains("changes: new: 1, done"); + } else { + assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done"); + } assertTwoChangesWithSameRevision(r); } 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 ada17b610f..4a5d496017 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 @@ -31,7 +31,6 @@ import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; import com.google.gerrit.server.update.RepoContext; -import com.google.gerrit.testutil.NoteDbMode; import com.google.inject.Inject; import java.io.IOException; import java.util.EnumSet; @@ -48,11 +47,12 @@ public class NoteDbOnlyIT extends AbstractDaemonTest { @Before public void setUp() throws Exception { - assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.DISABLE_CHANGE_REVIEW_DB); + assume().that(notesMigration.disableChangeReviewDb()).isTrue(); } @Test public void updateChangeFailureRollsBackRefUpdate() throws Exception { + assume().that(notesMigration.fuseUpdates()).isTrue(); PushOneCommit.Result r = createChange(); Change.Id id = r.getChange().getId(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java index 6ee3dfed3b..2e0439968f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java @@ -48,6 +48,7 @@ public class ConfigNotesMigration extends NotesMigration { // All of these names must be reflected in the allowed set in checkConfig. 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"; @@ -77,6 +78,8 @@ public class ConfigNotesMigration extends NotesMigration { cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), SEQUENCE, true); cfg.setString(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.NOTE_DB.name()); cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, true); + // TODO(dborowitz): Set to true when FileRepository supports it. + cfg.setBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false); return cfg; } @@ -85,6 +88,7 @@ public class ConfigNotesMigration extends NotesMigration { private final boolean readChangeSequence; private final PrimaryStorage changePrimaryStorage; private final boolean disableChangeReviewDb; + private final boolean fuseUpdates; @Inject public ConfigNotesMigration(@GerritServerConfig Config cfg) { @@ -103,6 +107,7 @@ public class ConfigNotesMigration extends NotesMigration { cfg.getEnum(SECTION_NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB); disableChangeReviewDb = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), DISABLE_REVIEW_DB, false); + fuseUpdates = cfg.getBoolean(SECTION_NOTE_DB, CHANGES.key(), FUSE_UPDATES, false); checkArgument( !(disableChangeReviewDb && changePrimaryStorage != PrimaryStorage.NOTE_DB), @@ -133,4 +138,9 @@ public class ConfigNotesMigration extends NotesMigration { public boolean disableChangeReviewDb() { return disableChangeReviewDb; } + + @Override + public boolean fuseUpdates() { + return fuseUpdates; + } } 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 995847ff25..7b0d76f02a 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 @@ -82,6 +82,19 @@ public abstract class NotesMigration { */ public abstract boolean 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 abstract boolean fuseUpdates(); + /** * Whether to fail when reading any data from NoteDb. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java index 373f56193f..78a78c2875 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java @@ -170,6 +170,7 @@ public class DeleteRef { private void deleteMultipleRefs(Repository r) throws OrmException, IOException, ResourceConflictException { BatchRefUpdate batchUpdate = r.getRefDatabase().newBatchUpdate(); + batchUpdate.setAtomic(false); List refs = prefix == null ? refsToDelete 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 03a7c375b8..22329fd9bf 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 @@ -94,7 +94,8 @@ public abstract class BatchUpdate implements AutoCloseable { @Override public void configure() { factory(ReviewDbBatchUpdate.AssistedFactory.class); - factory(NoteDbBatchUpdate.AssistedFactory.class); + factory(FusedNoteDbBatchUpdate.AssistedFactory.class); + factory(UnfusedNoteDbBatchUpdate.AssistedFactory.class); } }; } @@ -103,22 +104,28 @@ public abstract class BatchUpdate implements AutoCloseable { public static class Factory { private final NotesMigration migration; private final ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory; - private final NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory; + private final FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory; + private final UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory; @Inject Factory( NotesMigration migration, ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory, - NoteDbBatchUpdate.AssistedFactory noteDbBatchUpdateFactory) { + FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory, + UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory) { this.migration = migration; this.reviewDbBatchUpdateFactory = reviewDbBatchUpdateFactory; - this.noteDbBatchUpdateFactory = noteDbBatchUpdateFactory; + this.fusedNoteDbBatchUpdateFactory = fusedNoteDbBatchUpdateFactory; + this.unfusedNoteDbBatchUpdateFactory = unfusedNoteDbBatchUpdateFactory; } public BatchUpdate create( ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when) { if (migration.disableChangeReviewDb()) { - return noteDbBatchUpdateFactory.create(db, project, user, when); + if (migration.fuseUpdates()) { + return fusedNoteDbBatchUpdateFactory.create(db, project, user, when); + } + return unfusedNoteDbBatchUpdateFactory.create(db, project, user, when); } return reviewDbBatchUpdateFactory.create(db, project, user, when); } @@ -138,9 +145,15 @@ 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()) { - ImmutableList noteDbUpdates = - (ImmutableList) ImmutableList.copyOf(updates); - NoteDbBatchUpdate.execute(noteDbUpdates, listener, requestId, dryRun); + 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); + } } else { ImmutableList reviewDbUpdates = (ImmutableList) ImmutableList.copyOf(updates); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java similarity index 92% rename from gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java rename to gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java index f7988cf371..ad758484cb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.update; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import static java.util.Comparator.comparing; import com.google.common.base.Throwables; @@ -51,23 +52,25 @@ 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 that only supports NoteDb. + * {@link BatchUpdate} implementation using only NoteDb that updates code refs and meta refs in a + * single {@link org.eclipse.jgit.lib.BatchRefUpdate}. * *

Used when {@code noteDb.changes.disableReviewDb=true}, at which point ReviewDb is not * consulted during updates. */ -class NoteDbBatchUpdate extends BatchUpdate { +class FusedNoteDbBatchUpdate extends BatchUpdate { interface AssistedFactory { - NoteDbBatchUpdate create( + FusedNoteDbBatchUpdate create( ReviewDb db, Project.NameKey project, CurrentUser user, Timestamp when); } static void execute( - ImmutableList updates, + ImmutableList updates, BatchUpdateListener listener, @Nullable RequestId requestId, boolean dryrun) @@ -84,11 +87,11 @@ class NoteDbBatchUpdate extends BatchUpdate { try { switch (order) { case REPO_BEFORE_DB: - for (NoteDbBatchUpdate u : updates) { + for (FusedNoteDbBatchUpdate u : updates) { u.executeUpdateRepo(); } listener.afterUpdateRepos(); - for (NoteDbBatchUpdate u : updates) { + for (FusedNoteDbBatchUpdate u : updates) { handles.add(u.executeChangeOps(dryrun)); } for (ChangesHandle h : handles) { @@ -109,10 +112,10 @@ class NoteDbBatchUpdate 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 (NoteDbBatchUpdate u : updates) { + for (FusedNoteDbBatchUpdate u : updates) { handles.add(u.executeChangeOps(dryrun)); } - for (NoteDbBatchUpdate u : updates) { + for (FusedNoteDbBatchUpdate u : updates) { u.executeUpdateRepo(); } for (ChangesHandle h : handles) { @@ -147,7 +150,7 @@ class NoteDbBatchUpdate extends BatchUpdate { u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null))); if (!dryrun) { - for (NoteDbBatchUpdate u : updates) { + for (FusedNoteDbBatchUpdate u : updates) { u.executePostOps(); } } @@ -159,7 +162,7 @@ class NoteDbBatchUpdate extends BatchUpdate { class ContextImpl implements Context { @Override public RepoView getRepoView() throws IOException { - return NoteDbBatchUpdate.this.getRepoView(); + return FusedNoteDbBatchUpdate.this.getRepoView(); } @Override @@ -268,7 +271,7 @@ class NoteDbBatchUpdate extends BatchUpdate { private final ReviewDb db; @Inject - NoteDbBatchUpdate( + FusedNoteDbBatchUpdate( GitRepositoryManager repoManager, @GerritPersonIdent PersonIdent serverIdent, ChangeNotes.Factory changeNotesFactory, @@ -351,7 +354,7 @@ class NoteDbBatchUpdate extends BatchUpdate { } void execute() throws OrmException, IOException { - NoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun); + FusedNoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun); } List> startIndexFutures() { @@ -382,16 +385,19 @@ class NoteDbBatchUpdate extends BatchUpdate { private ChangesHandle executeChangeOps(boolean dryrun) throws Exception { logDebug("Executing change ops"); initRepository(); + Repository repo = repoView.getRepository(); + checkState( + repo.getRefDatabase().performsAtomicTransactions(), + "cannot use noteDb.changes.fuseUpdates=true with a repository that does not support atomic" + + " batch ref updates: %s", + repo); ChangesHandle handle = new ChangesHandle( updateManagerFactory .create(project) .setChangeRepo( - repoView.getRepository(), - repoView.getRevWalk(), - repoView.getInserter(), - repoView.getCommands()), + repo, repoView.getRevWalk(), repoView.getInserter(), repoView.getCommands()), dryrun); if (user.isIdentifiedUser()) { handle.manager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz)); 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 new file mode 100644 index 0000000000..2f7de46999 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java @@ -0,0 +1,458 @@ +// 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.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.common.util.concurrent.CheckedFuture; +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; + + 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); + checkArgument(!db.changesTablesEnabled(), "expected Change tables to be disabled on %s", db); + 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(); + 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/server/update/BatchUpdateTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/update/BatchUpdateTest.java index 892a9ffd36..107326e221 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/update/BatchUpdateTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/update/BatchUpdateTest.java @@ -31,11 +31,13 @@ import com.google.gerrit.server.util.ThreadLocalRequestContext; import com.google.gerrit.testutil.InMemoryDatabase; import com.google.gerrit.testutil.InMemoryModule; import com.google.gerrit.testutil.InMemoryRepositoryManager; +import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Guice; import com.google.inject.Inject; import com.google.inject.Injector; import com.google.inject.Provider; import com.google.inject.util.Providers; + import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.revwalk.RevCommit; @@ -45,19 +47,16 @@ import org.junit.Test; public class BatchUpdateTest { @Inject private AccountManager accountManager; - @Inject private IdentifiedUser.GenericFactory userFactory; - - @Inject private InMemoryDatabase schemaFactory; - + @Inject private SchemaFactory schemaFactory; @Inject private InMemoryRepositoryManager repoManager; - @Inject private SchemaCreator schemaCreator; - @Inject private ThreadLocalRequestContext requestContext; - @Inject private BatchUpdate.Factory batchUpdateFactory; + // Only for use in setting up/tearing down injector; other users should use schemaFactory. + @Inject private InMemoryDatabase inMemoryDatabase; + private LifecycleManager lifecycle; private ReviewDb db; private TestRepository repo; @@ -72,8 +71,10 @@ public class BatchUpdateTest { lifecycle.add(injector); lifecycle.start(); + try (ReviewDb underlyingDb = inMemoryDatabase.getDatabase().open()) { + schemaCreator.create(underlyingDb); + } db = schemaFactory.open(); - schemaCreator.create(db); Account.Id userId = accountManager.authenticate(AuthRequest.forUser("user")).getAccountId(); user = userFactory.create(userId); @@ -108,7 +109,7 @@ public class BatchUpdateTest { if (db != null) { db.close(); } - InMemoryDatabase.drop(schemaFactory); + InMemoryDatabase.drop(inMemoryDatabase); } @Test diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java index 038baacdb6..dd42b67d6a 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/GerritServerTests.java @@ -49,8 +49,10 @@ public class GerritServerTests extends GerritBaseTests { }; public void beforeTest() throws Exception { - notesMigration = new TestNotesMigration().setFromEnv(); + notesMigration = new TestNotesMigration(); } - public void afterTest() {} + public void afterTest() { + notesMigration.resetFromEnv(); + } } 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 4826d9e3e9..a90cbb97ea 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,6 +19,8 @@ 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; import java.util.SortedSet; @@ -30,7 +32,7 @@ import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; /** Repository manager that uses in-memory repositories. */ public class InMemoryRepositoryManager implements GitRepositoryManager { public static InMemoryRepository newRepository(Project.NameKey name) { - return new Repo(name); + return new Repo(new TestNotesMigration(), name); } public static class Description extends DfsRepositoryDescription { @@ -49,11 +51,15 @@ public class InMemoryRepositoryManager implements GitRepositoryManager { public static class Repo extends InMemoryRepository { private String description; - private Repo(Project.NameKey name) { + private Repo(NotesMigration notesMigration, Project.NameKey name) { super(new Description(name)); - // TODO(dborowitz): Allow atomic transactions when this is supported: - // https://git.eclipse.org/r/#/c/61841/2/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java@313 - setPerformsAtomicTransactions(false); + // 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()); } @Override @@ -72,7 +78,18 @@ public class InMemoryRepositoryManager implements GitRepositoryManager { } } - private Map repos = new HashMap<>(); + private final NotesMigration notesMigration; + private final Map repos; + + public InMemoryRepositoryManager() { + this(new TestNotesMigration()); + } + + @Inject + InMemoryRepositoryManager(NotesMigration notesMigration) { + this.notesMigration = notesMigration; + this.repos = new HashMap<>(); + } @Override public synchronized Repo openRepository(Project.NameKey name) throws RepositoryNotFoundException { @@ -89,7 +106,7 @@ public class InMemoryRepositoryManager implements GitRepositoryManager { throw new RepositoryCaseMismatchException(name); } } catch (RepositoryNotFoundException e) { - repo = new Repo(name); + repo = new Repo(notesMigration, 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 552f6f105f..e8446a20b0 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,6 +35,9 @@ public enum NoteDbMode { /** All change tables are entirely disabled. */ DISABLE_CHANGE_REVIEW_DB(true), + /** All change tables are entirely disabled, and code/meta ref updates are fused. */ + FUSED(true), + /** * Run tests with NoteDb disabled, then convert ReviewDb to NoteDb and check that the results * match. diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java index d4ddaf14df..c05dd01994 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java +++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java @@ -27,9 +27,12 @@ public class TestNotesMigration extends NotesMigration { private volatile boolean writeChanges; private volatile PrimaryStorage changePrimaryStorage = PrimaryStorage.REVIEW_DB; private volatile boolean disableChangeReviewDb; + private volatile boolean fuseUpdates; private volatile boolean failOnLoad; - public TestNotesMigration() {} + public TestNotesMigration() { + resetFromEnv(); + } @Override public boolean readChanges() { @@ -53,6 +56,11 @@ public class TestNotesMigration extends NotesMigration { return disableChangeReviewDb; } + @Override + public boolean fuseUpdates() { + return fuseUpdates; + } + // Increase visbility from superclass, as tests may want to check whether // NoteDb data is written in specific migration scenarios. @Override @@ -85,6 +93,11 @@ public class TestNotesMigration extends NotesMigration { return this; } + public TestNotesMigration setFuseUpdates(boolean fuseUpdates) { + this.fuseUpdates = fuseUpdates; + return this; + } + public TestNotesMigration setFailOnLoad(boolean failOnLoad) { this.failOnLoad = failOnLoad; return this; @@ -94,31 +107,42 @@ public class TestNotesMigration extends NotesMigration { return setReadChanges(enabled).setWriteChanges(enabled); } - public TestNotesMigration setFromEnv() { + public TestNotesMigration resetFromEnv() { switch (NoteDbMode.get()) { case READ_WRITE: setWriteChanges(true); setReadChanges(true); setChangePrimaryStorage(PrimaryStorage.REVIEW_DB); setDisableChangeReviewDb(false); + setFuseUpdates(false); break; case WRITE: setWriteChanges(true); setReadChanges(false); setChangePrimaryStorage(PrimaryStorage.REVIEW_DB); setDisableChangeReviewDb(false); + setFuseUpdates(false); break; case PRIMARY: setWriteChanges(true); setReadChanges(true); setChangePrimaryStorage(PrimaryStorage.NOTE_DB); setDisableChangeReviewDb(false); + setFuseUpdates(false); break; case DISABLE_CHANGE_REVIEW_DB: setWriteChanges(true); setReadChanges(true); setChangePrimaryStorage(PrimaryStorage.NOTE_DB); setDisableChangeReviewDb(true); + setFuseUpdates(false); + break; + case FUSED: + setWriteChanges(true); + setReadChanges(true); + setChangePrimaryStorage(PrimaryStorage.NOTE_DB); + setDisableChangeReviewDb(true); + setFuseUpdates(true); break; case CHECK: case OFF: @@ -127,6 +151,7 @@ public class TestNotesMigration extends NotesMigration { setReadChanges(false); setChangePrimaryStorage(PrimaryStorage.REVIEW_DB); setDisableChangeReviewDb(false); + setFuseUpdates(false); break; } return this; @@ -137,6 +162,7 @@ public class TestNotesMigration extends NotesMigration { setReadChanges(other.readChanges()); setChangePrimaryStorage(other.changePrimaryStorage()); setDisableChangeReviewDb(other.disableChangeReviewDb()); + setFuseUpdates(other.fuseUpdates()); return this; } }