Merge "Fix AbstractSubmit test flakiness"
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 0c93b52..ade1287 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.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 @@
/**
* Fetches each bundle into a newly cloned repository, then it applies the bundle, and returns the
* resulting tree id.
+ *
+ * <p>Omits NoteDb meta refs.
*/
protected Map<Branch.NameKey, ObjectId> fetchFromBundles(BinaryResult bundles) throws Exception {
assertThat(bundles.getContentType()).isEqualTo("application/x-zip");
@@ -1105,11 +1108,12 @@
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 0250db1..edf5420 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 @@
@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 d8aa35c..b94b062 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 @@
@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 26a91aa..5235b14 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 @@
@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 65ad499..b65cc0e 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 @@
@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 0000000..9850f2e
--- /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<ObjectId> getRef(String name) throws Exception {
+ try (Repository repo = repoManager.openRepository(project)) {
+ return Optional.ofNullable(repo.exactRef(name)).map(Ref::getObjectId);
+ }
+ }
+
+ private List<String> 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 b892e3d..8407bc6 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 @@
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 093ff21..7dec0fe 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.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 @@
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 194372f..d23e856 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 @@
}
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 @@
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 @@
Set<Change.Id> 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<ObjectId> metaId = Optional.of(cmd.getNewId());
staged.put(
@@ -450,13 +464,19 @@
}
}
- 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 @@
// 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 674ecb1..f7988cf 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 @@
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.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 @@
setRequestIds(updates, requestId);
try {
+ List<CheckedFuture<?, IOException>> indexFutures = new ArrayList<>();
+ List<ChangesHandle> 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 @@
private final GitReferenceUpdated gitRefUpdated;
private final ReviewDb db;
- private List<CheckedFuture<?, IOException>> indexFutures;
-
@Inject
NoteDbBatchUpdate(
GitRepositoryManager repoManager,
@@ -275,7 +290,6 @@
this.indexer = indexer;
this.gitRefUpdated = gitRefUpdated;
this.db = db;
- this.indexFutures = new ArrayList<>();
}
@Override
@@ -309,101 +323,104 @@
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<Change.Id, ChangeResult> 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<CheckedFuture<?, IOException>> startIndexFutures() {
+ if (dryrun) {
+ return ImmutableList.of();
}
- }
- if (!ok) {
- throw new RestApiException("BatchRefUpdate failed: " + batchRefUpdate);
+ logDebug("Reindexing {} changes", results.size());
+ List<CheckedFuture<?, IOException>> indexFutures = new ArrayList<>(results.size());
+ for (Map.Entry<Change.Id, ChangeResult> e : results.entrySet()) {
+ Change.Id id = e.getKey();
+ switch (e.getValue()) {
+ case UPSERTED:
+ indexFutures.add(indexer.indexAsync(project, id));
+ break;
+ case DELETED:
+ indexFutures.add(indexer.deleteAsync(id));
+ break;
+ case SKIPPED:
+ break;
+ default:
+ throw new IllegalStateException("unexpected result: " + e.getValue());
+ }
+ }
+ return indexFutures;
}
}
- private Map<Change.Id, ChangeResult> executeChangeOps(boolean dryrun) throws Exception {
+ private ChangesHandle executeChangeOps(boolean dryrun) throws Exception {
logDebug("Executing change ops");
- Map<Change.Id, ChangeResult> 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<Change.Id, Collection<BatchUpdateOp>> 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<Change.Id, Collection<BatchUpdateOp>> 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 @@
return new ChangeContextImpl(ctl);
}
- private void reindexChanges(Map<Change.Id, ChangeResult> updateResults, boolean dryrun) {
- if (dryrun) {
- return;
- }
- logDebug("Reindexing {} changes", updateResults.size());
- for (Map.Entry<Change.Id, ChangeResult> 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()) {
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index f5eff27..8a317ab 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -528,7 +528,6 @@
if (this.viewState.showReplyDialog) {
this._openReplyDialog();
- this.async(function() { this.$.replyOverlay.center(); }, 1);
this.set('viewState.showReplyDialog', false);
}
}.bind(this));
@@ -542,7 +541,7 @@
// another, so that the user's preference is restored.
this.set('viewState.diffMode', null);
this.set('_numFilesShown', DEFAULT_NUM_FILES_SHOWN);
- }
+ }
this.set('viewState.changeNum', this._changeNum);
this.set('viewState.patchRange', this._patchRange);
},
@@ -838,6 +837,8 @@
this.$.replyOverlay.open().then(function() {
this.$.replyOverlay.setFocusStops(this.$.replyDialog.getFocusStops());
this.$.replyDialog.open(opt_section);
+ Polymer.dom.flush();
+ this.$.replyOverlay.center();
}.bind(this));
},
