diff options
Diffstat (limited to 'gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java')
| -rw-r--r-- | gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java | 183 |
1 files changed, 63 insertions, 120 deletions
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 63d83ac1f5..0cc994e477 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,11 +14,11 @@ package com.google.gerrit.server.update; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static java.util.Comparator.comparing; import 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; @@ -29,7 +29,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; @@ -57,18 +56,12 @@ import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; 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; import com.google.inject.Inject; import com.google.inject.Singleton; import com.google.inject.assistedinject.Assisted; -import com.google.inject.assistedinject.AssistedInject; import java.io.IOException; import java.sql.Timestamp; import java.util.ArrayList; @@ -84,7 +77,6 @@ import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.Config; 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; @@ -117,19 +109,14 @@ class ReviewDbBatchUpdate extends BatchUpdate { } class ContextImpl implements Context { - private Repository repoWrapper; - @Override - public Repository getRepository() throws IOException { - if (repoWrapper == null) { - repoWrapper = new ReadOnlyRepository(ReviewDbBatchUpdate.this.getRepository()); - } - return repoWrapper; + public RepoView getRepoView() throws IOException { + return ReviewDbBatchUpdate.this.getRepoView(); } @Override public RevWalk getRevWalk() throws IOException { - return ReviewDbBatchUpdate.this.getRevWalk(); + return getRepoView().getRevWalk(); } @Override @@ -165,24 +152,19 @@ class ReviewDbBatchUpdate extends BatchUpdate { private class RepoContextImpl extends ContextImpl implements RepoContext { @Override - public Repository getRepository() throws IOException { - return ReviewDbBatchUpdate.this.getRepository(); - } - - @Override public ObjectInserter getInserter() throws IOException { - return ReviewDbBatchUpdate.this.getObjectInserter(); + return getRepoView().getInserterWrapper(); } @Override public void addRefUpdate(ReceiveCommand cmd) throws IOException { initRepository(); - commands.add(cmd); + repoView.getCommands().add(cmd); } } private class ChangeContextImpl extends ContextImpl implements ChangeContext { - private final ChangeControl ctl; + private final ChangeNotes notes; private final Map<PatchSet.Id, ChangeUpdate> updates; private final ReviewDbWrapper dbWrapper; private final Repository threadLocalRepo; @@ -192,8 +174,8 @@ class ReviewDbBatchUpdate extends BatchUpdate { private boolean bumpLastUpdatedOn = true; protected ChangeContextImpl( - ChangeControl ctl, ReviewDbWrapper dbWrapper, Repository repo, RevWalk rw) { - this.ctl = ctl; + ChangeNotes notes, ReviewDbWrapper dbWrapper, Repository repo, RevWalk rw) { + this.notes = checkNotNull(notes); this.dbWrapper = dbWrapper; this.threadLocalRepo = repo; this.threadLocalRevWalk = rw; @@ -207,11 +189,6 @@ class ReviewDbBatchUpdate extends BatchUpdate { } @Override - public Repository getRepository() { - return threadLocalRepo; - } - - @Override public RevWalk getRevWalk() { return threadLocalRevWalk; } @@ -220,8 +197,8 @@ class ReviewDbBatchUpdate extends BatchUpdate { public ChangeUpdate getUpdate(PatchSet.Id psId) { ChangeUpdate u = updates.get(psId); if (u == null) { - u = changeUpdateFactory.create(ctl, when); - if (newChanges.containsKey(ctl.getId())) { + u = changeUpdateFactory.create(notes, user, when); + if (newChanges.containsKey(notes.getChangeId())) { u.setAllowWriteToNewRef(true); } u.setPatchSetId(psId); @@ -231,14 +208,13 @@ class ReviewDbBatchUpdate extends BatchUpdate { } @Override - public ChangeControl getControl() { - checkNotNull(ctl); - return ctl; + public ChangeNotes getNotes() { + return notes; } @Override - public void bumpLastUpdatedOn(boolean bump) { - bumpLastUpdatedOn = bump; + public void dontBumpLastUpdatedOn() { + bumpLastUpdatedOn = false; } @Override @@ -272,18 +248,9 @@ 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); + Order order = getOrder(updates, listener); boolean updateChangesInParallel = getUpdateChangesInParallel(updates); switch (order) { case REPO_BEFORE_DB: @@ -304,66 +271,40 @@ class ReviewDbBatchUpdate extends BatchUpdate { for (ReviewDbBatchUpdate u : updates) { u.reindexChanges(u.executeChangeOps(updateChangesInParallel, dryrun)); } - listener.afterUpdateChanges(); for (ReviewDbBatchUpdate u : updates) { u.executeUpdateRepo(); } - listener.afterUpdateRepos(); for (ReviewDbBatchUpdate u : updates) { u.executeRefUpdates(dryrun); } - listener.afterUpdateRefs(); break; default: throw new IllegalStateException("invalid execution order: " + order); } - @SuppressWarnings("deprecation") - List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures = - new ArrayList<>(); - for (ReviewDbBatchUpdate u : updates) { - indexFutures.addAll(u.indexFutures); - } - ChangeIndexer.allAsList(indexFutures).get(); - - 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); - } - } + ChangeIndexer.allAsList( + updates.stream().flatMap(u -> u.indexFutures.stream()).collect(toList())) + .get(); + + // Fire ref update events only after all mutations are finished, since callers may assume a + // patch set ref being created means the change was created, or a branch advancing meaning + // some changes were closed. + updates.stream() + .filter(u -> u.batchRefUpdate != null) + .forEach( + u -> u.gitRefUpdated.fire(u.project, u.batchRefUpdate, u.getAccount().orElse(null))); + if (!dryrun) { for (ReviewDbBatchUpdate u : updates) { 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); } } private final AllUsersName allUsers; - private final ChangeControl.GenericFactory changeControlFactory; private final ChangeIndexer indexer; private final ChangeNotes.Factory changeNotesFactory; private final ChangeUpdate.Factory changeUpdateFactory; @@ -380,11 +321,10 @@ class ReviewDbBatchUpdate extends BatchUpdate { private final List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures = new ArrayList<>(); - @AssistedInject + @Inject ReviewDbBatchUpdate( @GerritServerConfig Config cfg, AllUsersName allUsers, - ChangeControl.GenericFactory changeControlFactory, ChangeIndexer indexer, ChangeNotes.Factory changeNotesFactory, @ChangeUpdateExecutor ListeningExecutorService changeUpdateExector, @@ -402,7 +342,6 @@ class ReviewDbBatchUpdate extends BatchUpdate { @Assisted Timestamp when) { super(repoManager, serverIdent, project, user, when); this.allUsers = allUsers; - this.changeControlFactory = changeControlFactory; this.changeNotesFactory = changeNotesFactory; this.changeUpdateExector = changeUpdateExector; this.changeUpdateFactory = changeUpdateFactory; @@ -417,11 +356,6 @@ class ReviewDbBatchUpdate extends BatchUpdate { } @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); } @@ -444,20 +378,18 @@ class ReviewDbBatchUpdate extends BatchUpdate { op.updateRepo(ctx); } - if (onSubmitValidators != null && commands != null && !commands.isEmpty()) { - try (ObjectReader reader = ctx.getInserter().newReader()) { - // Validation of refs has to take place here and not at the beginning - // executeRefUpdates. Otherwise failing validation in a second BatchUpdate object will - // happen *after* first object's executeRefUpdates has finished, hence after first repo's - // refs have been updated, which is too late. - onSubmitValidators.validate( - project, new ReadOnlyRepository(getRepository()), reader, commands.getCommands()); - } + if (onSubmitValidators != null && !getRefUpdates().isEmpty()) { + // Validation of refs has to take place here and not at the beginning of executeRefUpdates. + // Otherwise, failing validation in a second BatchUpdate object will happen *after* the + // first update's executeRefUpdates has finished, hence after first repo's refs have been + // updated, which is too late. + onSubmitValidators.validate( + project, ctx.getRevWalk().getObjectReader(), repoView.getCommands()); } - if (inserter != null) { + if (repoView != null) { logDebug("Flushing inserter"); - inserter.flush(); + repoView.getInserter().flush(); } else { logDebug("No objects to flush"); } @@ -468,24 +400,31 @@ class ReviewDbBatchUpdate extends BatchUpdate { } private void executeRefUpdates(boolean dryrun) throws IOException, RestApiException { - if (commands == null || commands.isEmpty()) { + if (getRefUpdates().isEmpty()) { logDebug("No ref updates to execute"); return; } // May not be opened if the caller added ref updates but no new objects. + // TODO(dborowitz): Really? initRepository(); - batchRefUpdate = repo.getRefDatabase().newBatchUpdate(); + batchRefUpdate = repoView.getRepository().getRefDatabase().newBatchUpdate(); + batchRefUpdate.setPushCertificate(pushCert); batchRefUpdate.setRefLogMessage(refLogMessage, true); + batchRefUpdate.setAllowNonFastForwards(true); + repoView.getCommands().addTo(batchRefUpdate); if (user.isIdentifiedUser()) { batchRefUpdate.setRefLogIdent(user.asIdentifiedUser().newRefLogIdent(when, tz)); } - commands.addTo(batchRefUpdate); logDebug("Executing batch of {} ref updates", batchRefUpdate.getCommands().size()); if (dryrun) { return; } - batchRefUpdate.execute(revWalk, NullProgressMonitor.INSTANCE); + // Force BatchRefUpdate to read newly referenced objects using a new RevWalk, rather than one + // that might have access to unflushed objects. + try (RevWalk updateRw = new RevWalk(repoView.getRepository())) { + batchRefUpdate.execute(updateRw, NullProgressMonitor.INSTANCE); + } boolean ok = true; for (ReceiveCommand cmd : batchRefUpdate.getCommands()) { if (cmd.getResult() != ReceiveCommand.Result.OK) { @@ -510,11 +449,11 @@ class ReviewDbBatchUpdate extends BatchUpdate { tasks = new ArrayList<>(ops.keySet().size()); try { - if (notesMigration.commitChangeWrites() && repo != null) { + if (notesMigration.commitChangeWrites() && repoView != null) { // A NoteDb change may have been rebuilt since the repo was originally // opened, so make sure we see that. logDebug("Preemptively scanning for repo changes"); - repo.scanForRepoChanges(); + repoView.getRepository().scanForRepoChanges(); } if (!ops.isEmpty() && notesMigration.failChangeWrites()) { // Fail fast before attempting any writes if changes are read-only, as @@ -582,9 +521,10 @@ class ReviewDbBatchUpdate extends BatchUpdate { // updates on the change repo first. logDebug("Executing NoteDb updates for {} changes", tasks.size()); try { - BatchRefUpdate changeRefUpdate = getRepository().getRefDatabase().newBatchUpdate(); + initRepository(); + BatchRefUpdate changeRefUpdate = repoView.getRepository().getRefDatabase().newBatchUpdate(); boolean hasAllUsersCommands = false; - try (ObjectInserter ins = getRepository().newObjectInserter()) { + try (ObjectInserter ins = repoView.getRepository().newObjectInserter()) { int objs = 0; for (ChangeTask task : tasks) { if (task.noteDbResult == null) { @@ -691,7 +631,8 @@ class ReviewDbBatchUpdate extends BatchUpdate { public Void call() throws Exception { taskId = id.toString() + "-" + Thread.currentThread().getId(); if (Thread.currentThread() == mainThread) { - Repository repo = getRepository(); + initRepository(); + Repository repo = repoView.getRepository(); try (RevWalk rw = new RevWalk(repo)) { call(ReviewDbBatchUpdate.this.db, repo, rw); } @@ -839,8 +780,7 @@ class ReviewDbBatchUpdate extends BatchUpdate { NoteDbChangeState.checkNotReadOnly(c, skewMs); } ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew); - ChangeControl ctl = changeControlFactory.controlFor(notes, user); - return new ChangeContextImpl(ctl, new BatchUpdateReviewDb(db), repo, rw); + return new ChangeContextImpl(notes, new BatchUpdateReviewDb(db), repo, rw); } private NoteDbUpdateManager stageNoteDbUpdate(ChangeContextImpl ctx, boolean deleted) @@ -850,7 +790,10 @@ class ReviewDbBatchUpdate extends BatchUpdate { updateManagerFactory .create(ctx.getProject()) .setChangeRepo( - ctx.getRepository(), ctx.getRevWalk(), null, new ChainedReceiveCommands(repo)); + ctx.threadLocalRepo, + ctx.threadLocalRevWalk, + null, + new ChainedReceiveCommands(ctx.threadLocalRepo)); if (ctx.getUser().isIdentifiedUser()) { updateManager.setRefLogIdent( ctx.getUser().asIdentifiedUser().newRefLogIdent(ctx.getWhen(), tz)); |