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 0c93b526b9..ade1287aeb 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 @@ -64,6 +64,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.ChangeFinder; @@ -1072,6 +1073,8 @@ public abstract class AbstractDaemonTest { /** * Fetches each bundle into a newly cloned repository, then it applies the bundle, and returns the * resulting tree id. + * + *

Omits NoteDb meta refs. */ protected Map fetchFromBundles(BinaryResult bundles) throws Exception { assertThat(bundles.getContentType()).isEqualTo("application/x-zip"); @@ -1105,11 +1108,12 @@ public abstract class AbstractDaemonTest { NullProgressMonitor.INSTANCE, Arrays.asList(new RefSpec("refs/*:refs/preview/*"))); for (Ref r : fr.getAdvertisedRefs()) { - String branchName = r.getName(); - Branch.NameKey n = new Branch.NameKey(proj, branchName); - + String refName = r.getName(); + if (RefNames.isNoteDbMetaRef(refName)) { + continue; + } RevCommit c = localRepo.getRevWalk().parseCommit(r.getObjectId()); - ret.put(n, c.getTree().copy()); + ret.put(new Branch.NameKey(proj, refName), c.getTree().copy()); } } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java index 0250db1888..edf5420831 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java @@ -130,6 +130,9 @@ public abstract class AbstractSubmitByMerge extends AbstractSubmit { @Test public void repairChangeStateAfterFailure() throws Exception { + // In NoteDb-only mode, repo and meta updates are atomic (at least in InMemoryRepository). + assume().that(notesMigration.disableChangeReviewDb()).isFalse(); + RevCommit initialHead = getRemoteHead(); PushOneCommit.Result change = createChange("Change 1", "a.txt", "content"); submit(change.getChangeId()); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java index d8aa35caa5..b94b06249c 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java @@ -15,6 +15,7 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.acceptance.GitUtil.getChangeId; import static com.google.gerrit.acceptance.GitUtil.pushHead; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; @@ -244,6 +245,9 @@ public abstract class AbstractSubmitByRebase extends AbstractSubmit { @Test public void repairChangeStateAfterFailure() throws Exception { + // In NoteDb-only mode, repo and meta updates are atomic (at least in InMemoryRepository). + assume().that(notesMigration.disableChangeReviewDb()).isFalse(); + RevCommit initialHead = getRemoteHead(); PushOneCommit.Result change = createChange("Change 1", "a.txt", "content"); submit(change.getChangeId()); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java index 26a91aab6c..5235b14837 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java @@ -15,6 +15,7 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.TruthJUnit.assume; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.PushOneCommit; @@ -388,6 +389,9 @@ public class SubmitByCherryPickIT extends AbstractSubmit { @Test public void repairChangeStateAfterFailure() throws Exception { + // In NoteDb-only mode, repo and meta updates are atomic (at least in InMemoryRepository). + assume().that(notesMigration.disableChangeReviewDb()).isFalse(); + RevCommit initialHead = getRemoteHead(); PushOneCommit.Result change = createChange("Change 1", "a.txt", "content"); submit(change.getChangeId()); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java index 65ad4991dd..b65cc0e23e 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java @@ -15,6 +15,7 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.acceptance.GitUtil.pushHead; import com.google.common.collect.Iterables; @@ -145,6 +146,9 @@ public class SubmitByFastForwardIT extends AbstractSubmit { @Test public void repairChangeStateAfterFailure() throws Exception { + // In NoteDb-only mode, repo and meta updates are atomic (at least in InMemoryRepository). + assume().that(notesMigration.disableChangeReviewDb()).isFalse(); + PushOneCommit.Result change = createChange("Change 1", "a.txt", "content"); Change.Id id = change.getChange().getId(); SubmitInput failAfterRefUpdates = new TestSubmitInput(new SubmitInput(), true); 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 new file mode 100644 index 0000000000..9850f2e19c --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java @@ -0,0 +1,146 @@ +// 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.acceptance.server.notedb; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assert_; +import static com.google.common.truth.Truth8.assertThat; +import static com.google.common.truth.TruthJUnit.assume; +import static java.util.stream.Collectors.toList; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.PushOneCommit; +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.reviewdb.client.Change; +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; +import java.util.List; +import java.util.Optional; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.transport.ReceiveCommand; +import org.junit.Before; +import org.junit.Test; + +public class NoteDbOnlyIT extends AbstractDaemonTest { + @Inject private BatchUpdate.Factory batchUpdateFactory; + + @Before + public void setUp() throws Exception { + assume().that(NoteDbMode.get()).isEqualTo(NoteDbMode.DISABLE_CHANGE_REVIEW_DB); + } + + @Test + public void updateChangeFailureRollsBackRefUpdate() throws Exception { + PushOneCommit.Result r = createChange(); + Change.Id id = r.getChange().getId(); + + String master = "refs/heads/master"; + String backup = "refs/backup/master"; + ObjectId master1 = getRef(master).get(); + assertThat(getRef(backup)).isEmpty(); + + // Toy op that copies the value of refs/heads/master to refs/backup/master. + BatchUpdateOp backupMasterOp = + new BatchUpdateOp() { + ObjectId newId; + + @Override + public void updateRepo(RepoContext ctx) throws IOException { + ObjectId oldId = ctx.getRepoView().getRef(backup).orElse(ObjectId.zeroId()); + newId = ctx.getRepoView().getRef(master).get(); + ctx.addRefUpdate(new ReceiveCommand(oldId, newId, backup)); + } + + @Override + public boolean updateChange(ChangeContext ctx) { + ctx.getUpdate(ctx.getChange().currentPatchSetId()) + .setChangeMessage("Backed up master branch to " + newId.name()); + return true; + } + }; + + try (BatchUpdate bu = newBatchUpdate()) { + bu.addOp(id, backupMasterOp); + bu.execute(); + } + + // Ensure backupMasterOp worked. + assertThat(getRef(backup)).hasValue(master1); + assertThat(getMessages(id)).contains("Backed up master branch to " + master1.name()); + + // Advance master by submitting the change. + gApi.changes().id(id.get()).current().review(ReviewInput.approve()); + gApi.changes().id(id.get()).current().submit(); + ObjectId master2 = getRef(master).get(); + assertThat(master2).isNotEqualTo(master1); + int msgCount = getMessages(id).size(); + + try (BatchUpdate bu = newBatchUpdate()) { + // This time, we attempt to back up master, but we fail during updateChange. + bu.addOp(id, backupMasterOp); + String msg = "Change is bad"; + bu.addOp( + id, + new BatchUpdateOp() { + @Override + public boolean updateChange(ChangeContext ctx) throws ResourceConflictException { + throw new ResourceConflictException(msg); + } + }); + try { + bu.execute(); + assert_().fail("expected ResourceConflictException"); + } catch (ResourceConflictException e) { + assertThat(e).hasMessageThat().isEqualTo(msg); + } + } + + // If updateChange hadn't failed, backup would have been updated to master2. + assertThat(getRef(backup)).hasValue(master1); + assertThat(getMessages(id)).hasSize(msgCount); + } + + private BatchUpdate newBatchUpdate() { + return batchUpdateFactory.create( + db, project, identifiedUserFactory.create(user.getId()), TimeUtil.nowTs()); + } + + private Optional getRef(String name) throws Exception { + try (Repository repo = repoManager.openRepository(project)) { + return Optional.ofNullable(repo.exactRef(name)).map(Ref::getObjectId); + } + } + + private List getMessages(Change.Id id) throws Exception { + return gApi.changes() + .id(id.get()) + .get(EnumSet.of(ListChangesOption.MESSAGES)) + .messages + .stream() + .map(m -> m.message) + .collect(toList()); + } +} diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java index b892e3dee1..8407bc6dd0 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java @@ -105,6 +105,17 @@ public class RefNames { return r.toString(); } + public static boolean isNoteDbMetaRef(String ref) { + if (ref.startsWith(REFS_CHANGES) + && (ref.endsWith(META_SUFFIX) || ref.endsWith(ROBOT_COMMENTS_SUFFIX))) { + return true; + } + if (ref.startsWith(REFS_DRAFT_COMMENTS) || ref.startsWith(REFS_STARRED_CHANGES)) { + return true; + } + return false; + } + public static String refsUsers(Account.Id accountId) { StringBuilder r = new StringBuilder(); r.append(REFS_USERS); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java index 093ff21d63..7dec0fe4c9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java @@ -25,6 +25,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.LimitedByteArrayOutputStream.LimitExceededException; @@ -151,7 +152,9 @@ public class PreviewSubmit implements RestReadView { for (ReceiveCommand r : refs) { bw.include(r.getRefName(), r.getNewId()); ObjectId oldId = r.getOldId(); - if (!oldId.equals(ObjectId.zeroId())) { + if (!oldId.equals(ObjectId.zeroId()) + // Probably the client doesn't already have NoteDb data. + && !RefNames.isNoteDbMetaRef(r.getRefName())) { bw.assume(or.getCodeReviewRevWalk().parseCommit(oldId)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java index 194372f0cc..d23e856d61 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java @@ -168,12 +168,16 @@ public class NoteDbUpdateManager implements AutoCloseable { } void flush() throws IOException { + flushToFinalInserter(); + finalIns.flush(); + tempIns.clear(); + } + + void flushToFinalInserter() throws IOException { checkState(finalIns != null); for (InsertedObject obj : tempIns.getInsertedObjects()) { finalIns.insert(obj.type(), obj.data().toByteArray()); } - finalIns.flush(); - tempIns.clear(); } @Override @@ -317,7 +321,13 @@ public class NoteDbUpdateManager implements AutoCloseable { return changeUpdates.isEmpty() && draftUpdates.isEmpty() && robotCommentUpdates.isEmpty() - && toDelete.isEmpty(); + && toDelete.isEmpty() + && !hasCommands(changeRepo) + && !hasCommands(allUsersRepo); + } + + private static boolean hasCommands(@Nullable OpenRepo or) { + return or != null && !or.cmds.isEmpty(); } /** @@ -385,6 +395,10 @@ public class NoteDbUpdateManager implements AutoCloseable { Set changeIds = new HashSet<>(); for (ReceiveCommand cmd : changeRepo.getCommandsSnapshot()) { Change.Id changeId = Change.Id.fromRef(cmd.getRefName()); + if (changeId == null || !cmd.getRefName().equals(RefNames.changeMetaRef(changeId))) { + // Not a meta ref update, likely due to a repo update along with the change meta update. + continue; + } changeIds.add(changeId); Optional metaId = Optional.of(cmd.getNewId()); staged.put( @@ -450,13 +464,19 @@ public class NoteDbUpdateManager implements AutoCloseable { } } - public void execute() throws OrmException, IOException { + @Nullable + public BatchRefUpdate execute() throws OrmException, IOException { + return execute(false); + } + + @Nullable + public BatchRefUpdate execute(boolean dryrun) throws OrmException, IOException { // Check before even inspecting the list, as this is a programmer error. if (migration.failChangeWrites()) { throw new OrmException(CHANGES_READ_ONLY); } if (isEmpty()) { - return; + return null; } try (Timer1.Context timer = metrics.updateLatency.start(CHANGES)) { stage(); @@ -468,36 +488,51 @@ public class NoteDbUpdateManager implements AutoCloseable { // we may have stale draft comments. Doing it in this order allows stale // comments to be filtered out by ChangeNotes, reflecting the fact that // comments can only go from DRAFT to PUBLISHED, not vice versa. - execute(changeRepo); - execute(allUsersRepo); + BatchRefUpdate result = execute(changeRepo, dryrun); + execute(allUsersRepo, dryrun); + return result; } finally { close(); } } - private void execute(OpenRepo or) throws IOException { + private BatchRefUpdate execute(OpenRepo or, boolean dryrun) throws IOException { if (or == null || or.cmds.isEmpty()) { - return; + return null; } - or.flush(); + if (!dryrun) { + or.flush(); + } else { + // OpenRepo buffers objects separately; caller may assume that objects are available in the + // inserter it previously passed via setChangeRepo. + // TODO(dborowitz): This should be flushToFinalInserter to avoid flushing objects to the + // underlying repo during a dry run. However, the only user of this is PreviewSubmit, which + // uses BundleWriter, which only takes a Repository so it can't read unflushed objects. Fix + // BundleWriter, then fix this call. + or.flush(); + } + BatchRefUpdate bru = or.repo.getRefDatabase().newBatchUpdate(); bru.setRefLogMessage(firstNonNull(refLogMessage, "Update NoteDb refs"), false); bru.setRefLogIdent(refLogIdent != null ? refLogIdent : serverIdent.get()); or.cmds.addTo(bru); bru.setAllowNonFastForwards(true); - bru.execute(or.rw, NullProgressMonitor.INSTANCE); - boolean lockFailure = false; - for (ReceiveCommand cmd : bru.getCommands()) { - if (cmd.getResult() == ReceiveCommand.Result.LOCK_FAILURE) { - lockFailure = true; - } else if (cmd.getResult() != ReceiveCommand.Result.OK) { - throw new IOException("Update failed: " + bru); + if (!dryrun) { + bru.execute(or.rw, NullProgressMonitor.INSTANCE); + boolean lockFailure = false; + for (ReceiveCommand cmd : bru.getCommands()) { + if (cmd.getResult() == ReceiveCommand.Result.LOCK_FAILURE) { + lockFailure = true; + } else if (cmd.getResult() != ReceiveCommand.Result.OK) { + throw new IOException("Update failed: " + bru); + } + } + if (lockFailure) { + throw new LockFailureException("Update failed with one or more lock failures: " + bru); } } - if (lockFailure) { - throw new LockFailureException("Update failed with one or more lock failures: " + bru); - } + return bru; } private void addCommands() throws OrmException, IOException { 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/NoteDbBatchUpdate.java index 674ecb1d90..f7988cf371 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/NoteDbBatchUpdate.java @@ -17,11 +17,9 @@ 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; @@ -46,15 +44,13 @@ 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.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; @@ -82,43 +78,64 @@ class NoteDbBatchUpdate extends BatchUpdate { setRequestIds(updates, requestId); try { + List> indexFutures = new ArrayList<>(); + List handles = new ArrayList<>(updates.size()); 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 (NoteDbBatchUpdate u : updates) { - u.executeUpdateRepo(); - } - listener.afterUpdateRepos(); - for (NoteDbBatchUpdate u : updates) { - u.executeRefUpdates(dryrun); - } - listener.afterUpdateRefs(); - for (NoteDbBatchUpdate u : updates) { - u.reindexChanges(u.executeChangeOps(dryrun), dryrun); - } - listener.afterUpdateChanges(); - break; - case DB_BEFORE_REPO: - for (NoteDbBatchUpdate u : updates) { - u.reindexChanges(u.executeChangeOps(dryrun), dryrun); - } - for (NoteDbBatchUpdate u : updates) { - u.executeUpdateRepo(); - } - for (NoteDbBatchUpdate u : updates) { - u.executeRefUpdates(dryrun); - } - break; - default: - throw new IllegalStateException("invalid execution order: " + order); + 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( - updates.stream().flatMap(u -> u.indexFutures.stream()).collect(toList())) - .get(); + 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 @@ -250,8 +267,6 @@ class NoteDbBatchUpdate extends BatchUpdate { private final GitReferenceUpdated gitRefUpdated; private final ReviewDb db; - private List> indexFutures; - @Inject NoteDbBatchUpdate( GitRepositoryManager repoManager, @@ -275,7 +290,6 @@ class NoteDbBatchUpdate extends BatchUpdate { this.indexer = indexer; this.gitRefUpdated = gitRefUpdated; this.db = db; - this.indexFutures = new ArrayList<>(); } @Override @@ -309,101 +323,104 @@ class NoteDbBatchUpdate extends BatchUpdate { 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; + private class ChangesHandle implements AutoCloseable { + private final NoteDbUpdateManager manager; + private final boolean dryrun; + private final Map results; + + ChangesHandle(NoteDbUpdateManager manager, boolean dryrun) { + this.manager = manager; + this.dryrun = dryrun; + results = new HashMap<>(); } - // 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); + @Override + public void close() { + manager.close(); } - boolean ok = true; - for (ReceiveCommand cmd : batchRefUpdate.getCommands()) { - if (cmd.getResult() != ReceiveCommand.Result.OK) { - ok = false; - break; + + 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); + } + + List> startIndexFutures() { + if (dryrun) { + return ImmutableList.of(); } - } - if (!ok) { - throw new RestApiException("BatchRefUpdate failed: " + batchRefUpdate); + logDebug("Reindexing {} changes", results.size()); + List> indexFutures = new ArrayList<>(results.size()); + for (Map.Entry 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 Map executeChangeOps(boolean dryrun) throws Exception { + private ChangesHandle 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 = + + ChangesHandle handle = + new ChangesHandle( updateManagerFactory .create(project) - .setChangeRepo(repo, rw, ins, new ChainedReceiveCommands(repo))) { - if (user.isIdentifiedUser()) { - updateManager.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz)); + .setChangeRepo( + repoView.getRepository(), + repoView.getRevWalk(), + repoView.getInserter(), + repoView.getCommands()), + dryrun); + if (user.isIdentifiedUser()) { + handle.manager.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); } - 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 (!dirty) { + logDebug("No ops reported dirty, short-circuiting"); + handle.setResult(id, ChangeResult.SKIPPED); + continue; } - - if (!dryrun) { - logDebug("Executing NoteDb updates"); - updateManager.execute(); + for (ChangeUpdate u : ctx.updates.values()) { + handle.manager.add(u); + } + if (ctx.deleted) { + logDebug("Change {} was deleted", id); + handle.manager.deleteChange(id); + handle.setResult(id, ChangeResult.DELETED); + } else { + handle.setResult(id, ChangeResult.UPSERTED); } } - return result; + return handle; } private ChangeContextImpl newChangeContext(Change.Id id) throws OrmException { @@ -423,28 +440,6 @@ class NoteDbBatchUpdate extends BatchUpdate { 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()) {