Remove batch update Order
This was created in Ic41cbe12 for the express purpose of deleting
changes and patch sets, when updates to the ReviewDb database happened
before updates to the Git storage. Deleting patch sets is no longer
possible, and now that we are NoteDb-only there is only a single
BatchRefUpdate and it doesn't matter what order we add commands to the
update. The only reason order might matter is if the BatchUpdateOp
implementation has internal state dependencies between stages.
DeleteChangeOp doesn't, therefore we can use the default order.
Change-Id: Ie15981b75de120752378cb610ed009f971533412
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteChange.java b/java/com/google/gerrit/server/restapi/change/DeleteChange.java
index aec1373..ef19c56 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteChange.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteChange.java
@@ -27,7 +27,6 @@
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.update.BatchUpdate;
-import com.google.gerrit.server.update.Order;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
import com.google.gerrit.server.update.UpdateException;
@@ -59,7 +58,6 @@
try (BatchUpdate bu =
updateFactory.create(rsrc.getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
Change.Id id = rsrc.getChange().getId();
- bu.setOrder(Order.DB_BEFORE_REPO);
bu.addOp(id, opFactory.create(id));
bu.execute();
}
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteChangeOp.java b/java/com/google/gerrit/server/restapi/change/DeleteChangeOp.java
index 6f035b6..05de507 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteChangeOp.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteChangeOp.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.restapi.change;
-import static com.google.common.base.Preconditions.checkState;
-
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -29,7 +27,6 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
-import com.google.gerrit.server.update.Order;
import com.google.gerrit.server.update.RepoContext;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -66,12 +63,12 @@
this.id = id;
}
+ // The relative order of updateChange and updateRepo doesn't matter as long as all operations are
+ // executed in a single atomic BatchRefUpdate. Actually deleting the change refs first would not
+ // fail gracefully if the second delete fails, but fortunately that's not what happens.
@Override
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException, IOException {
- checkState(
- ctx.getOrder() == Order.DB_BEFORE_REPO, "must use DeleteChangeOp with DB_BEFORE_REPO");
-
Collection<PatchSet> patchSets = psUtil.byChange(ctx.getNotes());
ensureDeletable(ctx, id, patchSets);
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 0ed031e..06397d7 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -127,55 +127,20 @@
List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures =
new ArrayList<>();
List<ChangesHandle> handles = new ArrayList<>(updates.size());
- Order order = getOrder(updates, listener);
try {
- switch (order) {
- case REPO_BEFORE_DB:
- for (BatchUpdate u : updates) {
- u.executeUpdateRepo();
- }
- listener.afterUpdateRepos();
- for (BatchUpdate u : updates) {
- handles.add(u.executeChangeOps(dryrun));
- }
- for (ChangesHandle h : handles) {
- h.execute();
- indexFutures.addAll(h.startIndexFutures());
- }
- listener.afterUpdateRefs();
- listener.afterUpdateChanges();
- break;
-
- case DB_BEFORE_REPO:
- // Call updateChange for each op before updateRepo, but defer executing the
- // NoteDbUpdateManager until after calling updateRepo. They share an inserter and
- // BatchRefUpdate, so it will all execute as a single batch. But we have to let
- // NoteDbUpdateManager actually execute the update, since it has to interleave it
- // properly with All-Users updates.
- //
- // TODO(dborowitz): This may still result in multiple updates to All-Users, but that's
- // currently not a big deal because multi-change batches generally aren't affecting
- // drafts anyway.
- for (BatchUpdate u : updates) {
- handles.add(u.executeChangeOps(dryrun));
- }
- for (BatchUpdate u : updates) {
- u.executeUpdateRepo();
- }
- for (ChangesHandle h : handles) {
- // TODO(dborowitz): This isn't quite good enough: in theory updateRepo may want to
- // see the results of change meta commands, but they aren't actually added to the
- // BatchUpdate until the body of execute. To fix this, execute needs to be split up
- // into a method that returns a BatchRefUpdate before execution. Not a big deal at the
- // moment, because this order is only used for deleting changes, and those updateRepo
- // implementations definitely don't need to observe the updated change meta refs.
- h.execute();
- indexFutures.addAll(h.startIndexFutures());
- }
- break;
- default:
- throw new IllegalStateException("invalid execution order: " + order);
+ for (BatchUpdate u : updates) {
+ u.executeUpdateRepo();
}
+ listener.afterUpdateRepos();
+ for (BatchUpdate u : updates) {
+ handles.add(u.executeChangeOps(dryrun));
+ }
+ for (ChangesHandle h : handles) {
+ h.execute();
+ indexFutures.addAll(h.startIndexFutures());
+ }
+ listener.afterUpdateRefs();
+ listener.afterUpdateChanges();
} finally {
for (ChangesHandle h : handles) {
h.close();
@@ -212,25 +177,6 @@
projectCounts);
}
- private static Order getOrder(
- Collection<? extends BatchUpdate> updates, BatchUpdateListener listener) {
- Order o = null;
- for (BatchUpdate u : updates) {
- if (o == null) {
- o = u.order;
- } else if (u.order != o) {
- throw new IllegalArgumentException("cannot mix execution orders");
- }
- }
- if (o != Order.REPO_BEFORE_DB) {
- checkArgument(
- listener == BatchUpdateListener.NONE,
- "BatchUpdateListener not supported for order %s",
- o);
- }
- return o;
- }
-
private static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException {
Throwables.throwIfUnchecked(e);
@@ -283,11 +229,6 @@
public CurrentUser getUser() {
return user;
}
-
- @Override
- public Order getOrder() {
- return order;
- }
}
private class RepoContextImpl extends ContextImpl implements RepoContext {
@@ -364,7 +305,6 @@
private RepoView repoView;
private BatchRefUpdate batchRefUpdate;
- private Order order;
private OnSubmitValidators onSubmitValidators;
private PushCertificate pushCert;
private String refLogMessage;
@@ -391,7 +331,6 @@
this.user = user;
this.when = when;
tz = serverIdent.getTimeZone();
- order = Order.REPO_BEFORE_DB;
}
@Override
@@ -425,11 +364,6 @@
return this;
}
- public BatchUpdate setOrder(Order order) {
- this.order = order;
- return this;
- }
-
/**
* Add a validation step for intended ref operations, which will be performed at the end of {@link
* RepoOnlyOp#updateRepo(RepoContext)} step.
diff --git a/java/com/google/gerrit/server/update/BatchUpdateListener.java b/java/com/google/gerrit/server/update/BatchUpdateListener.java
index 847a7ca..765bba1 100644
--- a/java/com/google/gerrit/server/update/BatchUpdateListener.java
+++ b/java/com/google/gerrit/server/update/BatchUpdateListener.java
@@ -19,8 +19,6 @@
*
* <p>When used during execution of multiple batch updates, the {@code after*} methods are called
* after that phase has been completed for <em>all</em> updates.
- *
- * <p>Listeners are only supported for the {@link Order#REPO_BEFORE_DB} order.
*/
public interface BatchUpdateListener {
public static final BatchUpdateListener NONE = new BatchUpdateListener() {};
diff --git a/java/com/google/gerrit/server/update/Context.java b/java/com/google/gerrit/server/update/Context.java
index aa17032..c24c650 100644
--- a/java/com/google/gerrit/server/update/Context.java
+++ b/java/com/google/gerrit/server/update/Context.java
@@ -86,13 +86,6 @@
CurrentUser getUser();
/**
- * Get the order in which operations are executed in this update.
- *
- * @return order of operations.
- */
- Order getOrder();
-
- /**
* Get the identified user performing the update.
*
* <p>Convenience method for {@code getUser().asIdentifiedUser()}.
diff --git a/java/com/google/gerrit/server/update/Order.java b/java/com/google/gerrit/server/update/Order.java
deleted file mode 100644
index fb64b7b..0000000
--- a/java/com/google/gerrit/server/update/Order.java
+++ /dev/null
@@ -1,34 +0,0 @@
-// Copyright (C) 2017 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.update;
-
-/** Order of execution of the various phases of a {@link BatchUpdate}. */
-public enum Order {
- /**
- * Update the repository and execute all ref updates before touching the database.
- *
- * <p>The default and most common, as Gerrit does not behave well when a patch set has no
- * corresponding ref in the repo.
- */
- REPO_BEFORE_DB,
-
- /**
- * Update the database before touching the repository.
- *
- * <p>Generally only used when deleting patch sets, which should be deleted first from the
- * database (for the same reason as above.)
- */
- DB_BEFORE_REPO;
-}
