From 7185eeb1be15d31898e31ef17941004b694909e7 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 21 Mar 2017 08:40:52 -0700 Subject: [PATCH 1/5] ChangeContext: Remove arg from bumpLastUpdatedOn This method is only called in the false case, so it's clearer if it doesn't take an argument. Change-Id: Id7700ffabdfe380e868d1365eda7fd1f7dabd339 --- .../gerrit/acceptance/server/change/GetRelatedIT.java | 2 +- .../gerrit/server/change/CreateDraftComment.java | 2 +- .../gerrit/server/change/DeleteDraftComment.java | 2 +- .../google/gerrit/server/change/PutDraftComment.java | 2 +- .../google/gerrit/server/update/ChangeContext.java | 11 +++++++---- .../gerrit/server/update/ReviewDbBatchUpdate.java | 4 ++-- 6 files changed, 13 insertions(+), 10 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java index 8d9885cee6..b4f68fa871 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java @@ -586,7 +586,7 @@ public class GetRelatedIT extends AbstractDaemonTest { public boolean updateChange(ChangeContext ctx) throws OrmException { PatchSet ps = psUtil.get(ctx.getDb(), ctx.getNotes(), psId); psUtil.setGroups(ctx.getDb(), ctx.getUpdate(psId), ps, ImmutableList.of()); - ctx.bumpLastUpdatedOn(false); + ctx.dontBumpLastUpdatedOn(); return true; } }); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java index 5032e573eb..6536550fa2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateDraftComment.java @@ -122,7 +122,7 @@ public class CreateDraftComment implements RestModifyViewIf called, don't bump the timestamp before storing to ReviewDb. Only has an effect in + * ReviewDb, and the only usage should be to match the behavior of NoteDb. Specifically, in NoteDb + * the timestamp is updated if and only if the change meta graph is updated, and is not updated + * when only drafts are modified. */ - void bumpLastUpdatedOn(boolean bump); + void dontBumpLastUpdatedOn(); /** * Instruct {@link BatchUpdate} to delete this change. diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java index 6d22dc24ad..49e80126de 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java @@ -236,8 +236,8 @@ class ReviewDbBatchUpdate extends BatchUpdate { } @Override - public void bumpLastUpdatedOn(boolean bump) { - bumpLastUpdatedOn = bump; + public void dontBumpLastUpdatedOn() { + bumpLastUpdatedOn = false; } @Override From dc84e4acd73fb4fda7c98895780397faab7b4ffb Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 21 Mar 2017 08:30:01 -0700 Subject: [PATCH 2/5] Expand javadoc for BatchUpdate contexts All javadocs should contain a summary per the style guide[1]. Include some relevant links to CurrentUser documentation as well. [1] https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment Change-Id: I1dffeddd7542356e60142fa52ca00f85d8f1b2a8 --- .../gerrit/server/update/ChangeContext.java | 14 +++- .../google/gerrit/server/update/Context.java | 66 ++++++++++++++----- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/ChangeContext.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/ChangeContext.java index 8cbccb8892..ca39763b26 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/ChangeContext.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/ChangeContext.java @@ -44,8 +44,12 @@ public interface ChangeContext extends Context { ChangeUpdate getUpdate(PatchSet.Id psId); /** - * @return control for this change. The user will be the same as {@link #getUser()}, and the - * change data is read within the same transaction that {@code updateChange} is executing. + * Get the control for this change, encapsulating the user and up-to-date change data. + * + *

The user will be the same as {@link #getUser()}, and the change data is read within the same + * transaction that {@link BatchUpdateOp#updateChange(ChangeContext)} is executing. + * + * @return control for this change. */ ChangeControl getControl(); @@ -66,7 +70,11 @@ public interface ChangeContext extends Context { */ void deleteChange(); - /** @return notes corresponding to {@link #getControl()}. */ + /** + * Get notes corresponding to {@link #getControl()}. + * + * @return loaded notes instance. + */ default ChangeNotes getNotes() { return checkNotNull(getControl().getNotes()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java index 497b7ab270..db9239f530 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/Context.java @@ -33,7 +33,11 @@ import org.eclipse.jgit.revwalk.RevWalk; *

A single update may span multiple changes, but they all belong to a single repo. */ public interface Context { - /** @return the project name this update operates on. */ + /** + * Get the project name this update operates on. + * + * @return project. + */ Project.NameKey getProject(); /** @@ -57,50 +61,80 @@ public interface Context { */ RevWalk getRevWalk() throws IOException; - /** @return the timestamp at which this update takes place. */ + /** + * Get the timestamp at which this update takes place. + * + * @return timestamp. + */ Timestamp getWhen(); /** - * @return the time zone in which this update takes place. In the current implementation, this is - * always the time zone of the server. + * Get the time zone in which this update takes place. + * + *

In the current implementation, this is always the time zone of the server. + * + * @return time zone. */ TimeZone getTimeZone(); /** - * @return an open ReviewDb database. Callers should not manage transactions or call mutating - * methods on the Changes table. Mutations on other tables (including other entities in the - * change entity group) are fine. + * Get the ReviewDb database. + * + *

Callers should not manage transactions or call mutating methods on the Changes table. + * Mutations on other tables (including other entities in the change entity group) are fine. + * + * @return open database instance. */ ReviewDb getDb(); /** - * @return user performing the update. In the current implementation, this is always an {@link - * IdentifiedUser} or {@link com.google.gerrit.server.InternalUser}. + * Get the user performing the update. + * + *

In the current implementation, this is always an {@link IdentifiedUser} or {@link + * com.google.gerrit.server.InternalUser}. + * + * @return user. */ CurrentUser getUser(); - /** @return order in which operations are executed in this update. */ + /** + * Get the order in which operations are executed in this update. + * + * @return order of operations. + */ Order getOrder(); /** - * @return identified user performing the update; throws an unchecked exception if the user is not - * an {@link IdentifiedUser} + * Get the identified user performing the update. + * + *

Convenience method for {@code getUser().asIdentifiedUser()}. + * + * @see CurrentUser#asIdentifiedUser() + * @return user. */ default IdentifiedUser getIdentifiedUser() { return checkNotNull(getUser()).asIdentifiedUser(); } /** - * @return account of the user performing the update; throws if the user is not an {@link - * IdentifiedUser} + * Get the account of the user performing the update. + * + *

Convenience method for {@code getIdentifiedUser().getAccount()}. + * + * @see CurrentUser#asIdentifiedUser() + * @return account. */ default Account getAccount() { return getIdentifiedUser().getAccount(); } /** - * @return account ID of the user performing the update; throws if the user is not an {@link - * IdentifiedUser} + * Get the account ID of the user performing the update. + * + *

Convenience method for {@code getUser().getAccountId()} + * + * @see CurrentUser#getAccountId() + * @return account ID. */ default Account.Id getAccountId() { return getIdentifiedUser().getAccountId(); From 34bc11a6e090d948c7b602a9d524193120b52248 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 21 Mar 2017 08:03:06 -0700 Subject: [PATCH 3/5] BatchUpdate: Remove protected from static methods This modifier is not necessary for the methods to be used by implementations in the same package. Change-Id: Iafc1152b48bf9097aa9d7e064b6685036167990a --- .../java/com/google/gerrit/server/update/BatchUpdate.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 2764bd8c06..9dccc8d6d4 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 @@ -117,7 +117,7 @@ public abstract class BatchUpdate implements AutoCloseable { } } - protected static Order getOrder(Collection updates) { + static Order getOrder(Collection updates) { Order o = null; for (BatchUpdate u : updates) { if (o == null) { @@ -129,7 +129,7 @@ public abstract class BatchUpdate implements AutoCloseable { return o; } - protected static boolean getUpdateChangesInParallel(Collection updates) { + static boolean getUpdateChangesInParallel(Collection updates) { checkArgument(!updates.isEmpty()); Boolean p = null; for (BatchUpdate u : updates) { From 00e0daac1b427983adbd8d28a52b6e82da4cdcee Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 21 Mar 2017 07:58:12 -0700 Subject: [PATCH 4/5] ReviewDbBatchUpdate: Simplify some loop code Extract a getAccount() method into BatchUpdate. Eventually this might move a step further up into CurrentUser, but not in this cleanup. Change-Id: Ia43e917c6cee67dfdf22fcc7aea7a9f44b49fd90 --- .../gerrit/server/update/BatchUpdate.java | 8 +++++ .../server/update/ReviewDbBatchUpdate.java | 30 ++++++++----------- 2 files changed, 21 insertions(+), 17 deletions(-) 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 9dccc8d6d4..c7ef315d79 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 @@ -24,6 +24,7 @@ import com.google.common.collect.MultimapBuilder; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.config.FactoryModule; import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -41,6 +42,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.TimeZone; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.ObjectInserter; @@ -251,6 +253,12 @@ public abstract class BatchUpdate implements AutoCloseable { return user; } + protected Optional getAccount() { + return user.isIdentifiedUser() + ? Optional.of(user.asIdentifiedUser().getAccount()) + : Optional.empty(); + } + protected Repository getRepository() throws IOException { initRepository(); return repo; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java index 49e80126de..4897c0a795 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java @@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static java.util.Comparator.comparing; import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.stream.Collectors.toList; import com.google.common.base.Stopwatch; import com.google.common.base.Throwables; @@ -317,24 +318,19 @@ class ReviewDbBatchUpdate extends BatchUpdate { throw new IllegalStateException("invalid execution order: " + order); } - List> indexFutures = new ArrayList<>(); - for (ReviewDbBatchUpdate u : updates) { - indexFutures.addAll(u.indexFutures); - } - ChangeIndexer.allAsList(indexFutures).get(); + 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))); - for (ReviewDbBatchUpdate u : updates) { - if (u.batchRefUpdate != null) { - // 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. - u.gitRefUpdated.fire( - u.project, - u.batchRefUpdate, - u.getUser().isIdentifiedUser() ? u.getUser().asIdentifiedUser().getAccount() : null); - } - } if (!dryrun) { for (ReviewDbBatchUpdate u : updates) { u.executePostOps(); From d80e91dc8a9cda72b12c5a1edd95daec36f31bbd Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 21 Mar 2017 08:01:10 -0700 Subject: [PATCH 5/5] Extract more helper methods into BatchUpdate Change-Id: Ief1a92c62ff77e18237a147f7b5fcd769e2ff601 --- .../gerrit/server/update/BatchUpdate.java | 47 ++++++++++++++++++- .../server/update/ReviewDbBatchUpdate.java | 38 +-------------- 2 files changed, 48 insertions(+), 37 deletions(-) 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 c7ef315d79..ba4d93ba5a 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 @@ -18,11 +18,14 @@ 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 com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ListMultimap; import com.google.common.collect.MultimapBuilder; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.config.FactoryModule; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; @@ -31,6 +34,10 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.validators.OnSubmitValidators; +import com.google.gerrit.server.project.InvalidChangeOperationException; +import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.project.NoSuchProjectException; +import com.google.gerrit.server.project.NoSuchRefException; import com.google.gerrit.server.util.RequestId; import com.google.inject.Inject; import com.google.inject.Module; @@ -119,6 +126,20 @@ public abstract class BatchUpdate implements AutoCloseable { } } + static void setRequestIds( + Collection updates, @Nullable RequestId requestId) { + if (requestId != null) { + for (BatchUpdate u : updates) { + checkArgument( + u.requestId == null || u.requestId == requestId, + "refusing to overwrite RequestId %s in update with %s", + u.requestId, + requestId); + u.setRequestId(requestId); + } + } + } + static Order getOrder(Collection updates) { Order o = null; for (BatchUpdate u : updates) { @@ -150,6 +171,28 @@ public abstract class BatchUpdate implements AutoCloseable { return p; } + static void wrapAndThrowException(Exception e) throws UpdateException, RestApiException { + Throwables.throwIfUnchecked(e); + + // Propagate REST API exceptions thrown by operations; they commonly throw exceptions like + // ResourceConflictException to indicate an atomic update failure. + Throwables.throwIfInstanceOf(e, UpdateException.class); + Throwables.throwIfInstanceOf(e, RestApiException.class); + + // Convert other common non-REST exception types with user-visible messages to corresponding + // REST exception types + if (e instanceof InvalidChangeOperationException) { + throw new ResourceConflictException(e.getMessage(), e); + } else if (e instanceof NoSuchChangeException + || e instanceof NoSuchRefException + || e instanceof NoSuchProjectException) { + throw new ResourceNotFoundException(e.getMessage(), e); + } + + // Otherwise, wrap in a generic UpdateException, which does not include a user-visible message. + throw new UpdateException(e); + } + protected GitRepositoryManager repoManager; protected final Project.NameKey project; @@ -200,7 +243,9 @@ public abstract class BatchUpdate implements AutoCloseable { public abstract void execute(BatchUpdateListener listener) throws UpdateException, RestApiException; - public abstract void execute() throws UpdateException, RestApiException; + public void execute() throws UpdateException, RestApiException { + execute(BatchUpdateListener.NONE); + } protected abstract Context newContext(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java index 4897c0a795..abbc1d4a7f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java @@ -14,7 +14,6 @@ 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; @@ -31,7 +30,6 @@ import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.restapi.ResourceConflictException; -import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.metrics.Description; import com.google.gerrit.metrics.Description.Units; @@ -60,10 +58,6 @@ import com.google.gerrit.server.notedb.NoteDbUpdateManager; import com.google.gerrit.server.notedb.NoteDbUpdateManager.MismatchedStateException; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ChangeControl; -import com.google.gerrit.server.project.InvalidChangeOperationException; -import com.google.gerrit.server.project.NoSuchChangeException; -import com.google.gerrit.server.project.NoSuchProjectException; -import com.google.gerrit.server.project.NoSuchRefException; import com.google.gerrit.server.util.RequestId; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; @@ -272,16 +266,7 @@ class ReviewDbBatchUpdate extends BatchUpdate { if (updates.isEmpty()) { return; } - if (requestId != null) { - for (BatchUpdate u : updates) { - checkArgument( - u.requestId == null || u.requestId == requestId, - "refusing to overwrite RequestId %s in update with %s", - u.requestId, - requestId); - u.setRequestId(requestId); - } - } + setRequestIds(updates, requestId); try { Order order = getOrder(updates); boolean updateChangesInParallel = getUpdateChangesInParallel(updates); @@ -336,22 +321,8 @@ class ReviewDbBatchUpdate extends BatchUpdate { u.executePostOps(); } } - } catch (UpdateException | RestApiException e) { - // Propagate REST API exceptions thrown by operations; they commonly throw - // exceptions like ResourceConflictException to indicate an atomic update - // failure. - throw e; - - // Convert other common non-REST exception types with user-visible - // messages to corresponding REST exception types - } catch (InvalidChangeOperationException e) { - throw new ResourceConflictException(e.getMessage(), e); - } catch (NoSuchChangeException | NoSuchRefException | NoSuchProjectException e) { - throw new ResourceNotFoundException(e.getMessage(), e); - } catch (Exception e) { - Throwables.throwIfUnchecked(e); - throw new UpdateException(e); + wrapAndThrowException(e); } } @@ -406,11 +377,6 @@ class ReviewDbBatchUpdate extends BatchUpdate { skewMs = NoteDbChangeState.getReadOnlySkew(cfg); } - @Override - public void execute() throws UpdateException, RestApiException { - execute(BatchUpdateListener.NONE); - } - @Override public void execute(BatchUpdateListener listener) throws UpdateException, RestApiException { execute(ImmutableList.of(this), listener, requestId, false);