From 1635b5d6a48adc2cb0e14e691fdfa21bd961dfbf Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 16 Jun 2016 11:38:46 -0400 Subject: [PATCH] Return more structured result from ChangeRebuilder Refactors ChangeRebuilder and NoteDbUpdateManager to return a new type encapsulating both the new NoteDbChangeState (which is intentionally not part of the original staged result) and the underlying staged result with its possibly-not-yet-written NoteDb data. Change-Id: I2ff014e9fe01910a2b77b1f5417f1f11dc84b610 --- .../google/gerrit/server/git/BatchUpdate.java | 3 +-- .../gerrit/server/notedb/ChangeNotes.java | 5 ++++- .../gerrit/server/notedb/ChangeRebuilder.java | 11 +++++----- .../server/notedb/ChangeRebuilderImpl.java | 14 ++++++------- .../gerrit/server/notedb/ChangeUpdate.java | 2 +- .../server/notedb/DraftCommentNotes.java | 6 +++--- .../server/notedb/NoteDbChangeState.java | 6 ------ .../gerrit/server/notedb/NoteDbModule.java | 17 +++++----------- .../server/notedb/NoteDbUpdateManager.java | 20 +++++++++++++++++++ .../notedb/TestChangeRebuilderWrapper.java | 7 ++++--- 10 files changed, 51 insertions(+), 40 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java index 8368bc484b..e45ea5e34c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java @@ -44,7 +44,6 @@ import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.InsertedObject; -import com.google.gerrit.server.notedb.NoteDbChangeState; import com.google.gerrit.server.notedb.NoteDbUpdateManager; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.project.ChangeControl; @@ -874,7 +873,7 @@ public class BatchUpdate implements AutoCloseable { updateManager.deleteChange(ctx.getChange().getId()); } try { - NoteDbChangeState.applyDelta(ctx.getChange(), updateManager.stage()); + updateManager.stageAndApplyDelta(ctx.getChange()); } catch (OrmConcurrencyException ex) { // Refused to apply update because NoteDb was out of sync. Go ahead with // this ReviewDb update; it's still out of sync, but this is no worse diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index 55350e0773..7f385c085c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -53,6 +53,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.git.RefCache; import com.google.gerrit.server.git.RepoRefCache; +import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.query.change.ChangeData; @@ -595,7 +596,9 @@ public class ChangeNotes extends AbstractChangeNotes { try { NoteDbChangeState newState; try { - newState = args.rebuilder.get().rebuild(args.db.get(), getChangeId()); + NoteDbUpdateManager.Result r = + args.rebuilder.get().rebuild(args.db.get(), getChangeId()); + newState = r != null ? r.newState() : null; repo.scanForRepoChanges(); } catch (IOException e) { newState = recheckUpToDate(repo, e); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java index e3f73c5b5b..d299d6cb0e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java @@ -20,6 +20,7 @@ import com.google.common.util.concurrent.ListeningExecutorService; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; @@ -37,11 +38,11 @@ public abstract class ChangeRebuilder { this.schemaFactory = schemaFactory; } - public final ListenableFuture rebuildAsync( + public final ListenableFuture rebuildAsync( final Change.Id id, ListeningExecutorService executor) { - return executor.submit(new Callable() { + return executor.submit(new Callable() { @Override - public NoteDbChangeState call() throws Exception { + public Result call() throws Exception { try (ReviewDb db = schemaFactory.open()) { return rebuild(db, id); } @@ -49,11 +50,11 @@ public abstract class ChangeRebuilder { }); } - public abstract NoteDbChangeState rebuild(ReviewDb db, Change.Id changeId) + public abstract Result rebuild(ReviewDb db, Change.Id changeId) throws NoSuchChangeException, IOException, OrmException, ConfigInvalidException; - public abstract NoteDbChangeState rebuild(NoteDbUpdateManager manager, + public abstract Result rebuild(NoteDbUpdateManager manager, ChangeBundle bundle) throws NoSuchChangeException, IOException, OrmException, ConfigInvalidException; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java index 249ad2c839..a709fd63c2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java @@ -57,6 +57,7 @@ import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.git.ChainedReceiveCommands; import com.google.gerrit.server.notedb.NoteDbUpdateManager.OpenRepo; +import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.ProjectCache; @@ -150,7 +151,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { } @Override - public NoteDbChangeState rebuild(ReviewDb db, Change.Id changeId) + public Result rebuild(ReviewDb db, Change.Id changeId) throws NoSuchChangeException, IOException, OrmException, ConfigInvalidException { db = unwrapDb(db); @@ -164,7 +165,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { return execute(db, changeId, manager); } - private NoteDbChangeState execute(ReviewDb db, Change.Id changeId, + private Result execute(ReviewDb db, Change.Id changeId, NoteDbUpdateManager manager) throws NoSuchChangeException, OrmException, IOException { Change change = db.changes().get(changeId); @@ -173,8 +174,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { } final String oldNoteDbState = change.getNoteDbState(); - NoteDbChangeState newState = - NoteDbChangeState.applyDelta(change, manager.stage()); + Result r = manager.stageAndApplyDelta(change); final String newNoteDbState = change.getNoteDbState(); try { db.changes().atomicUpdate(changeId, new AtomicUpdate() { @@ -191,7 +191,7 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { } catch (AbortUpdateException e) { // Drop this rebuild; another thread completed it. } - return newState; + return r; } private static class AbortUpdateException extends OrmRuntimeException { @@ -203,12 +203,12 @@ public class ChangeRebuilderImpl extends ChangeRebuilder { } @Override - public NoteDbChangeState rebuild(NoteDbUpdateManager manager, + public Result rebuild(NoteDbUpdateManager manager, ChangeBundle bundle) throws NoSuchChangeException, IOException, OrmException, ConfigInvalidException { Change change = new Change(bundle.getChange()); buildUpdates(manager, bundle); - return NoteDbChangeState.applyDelta(change, manager.stage()); + return manager.stageAndApplyDelta(change); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 4ab598d0a0..d9aa53210f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -221,7 +221,7 @@ public class ChangeUpdate extends AbstractChangeUpdate { NoteDbUpdateManager updateManager = updateManagerFactory.create(getProjectName()); updateManager.add(this); - NoteDbChangeState.applyDelta(getChange(), updateManager.stage()); + updateManager.stageAndApplyDelta(getChange()); updateManager.execute(); return getResult(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java index 74c27bde40..ae741f6e73 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java @@ -160,12 +160,12 @@ public class DraftCommentNotes extends AbstractChangeNotes { private LoadHandle rebuildAndOpen(Repository repo) throws IOException { try { - NoteDbChangeState newState = + NoteDbUpdateManager.Result r = args.rebuilder.get().rebuild(args.db.get(), getChangeId()); - if (newState == null) { + if (r == null) { return super.openHandle(repo); // May be null in tests. } - ObjectId draftsId = newState.getDraftIds().get(author); + ObjectId draftsId = r.newState().getDraftIds().get(author); repo.scanForRepoChanges(); return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), draftsId); } catch (NoSuchChangeException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java index e72362a443..2f1e0bdc95 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbChangeState.java @@ -100,12 +100,6 @@ public class NoteDbChangeState { return new NoteDbChangeState(id, changeMetaId, draftIds); } - public static NoteDbChangeState applyDelta(Change change, - Map stagedResults) { - NoteDbUpdateManager.StagedResult r = stagedResults.get(change.getId()); - return applyDelta(change, r != null ? r.delta() : null); - } - public static NoteDbChangeState applyDelta(Change change, Delta delta) { if (delta == null) { return null; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java index 5074bd3a73..285b6fdfd0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java @@ -22,17 +22,13 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change.Id; import com.google.gerrit.reviewdb.client.Project.NameKey; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.project.NoSuchChangeException; -import com.google.gwtorm.server.OrmException; +import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result; import com.google.inject.TypeLiteral; import com.google.inject.name.Names; -import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Repository; -import java.io.IOException; - public class NoteDbModule extends FactoryModule { private final Config cfg; private final boolean useTestBindings; @@ -68,23 +64,20 @@ public class NoteDbModule extends FactoryModule { } else { bind(ChangeRebuilder.class).toInstance(new ChangeRebuilder(null) { @Override - public NoteDbChangeState rebuild(ReviewDb db, Change.Id changeId) - throws OrmException { + public Result rebuild(ReviewDb db, Change.Id changeId) { return null; } @Override - public NoteDbChangeState rebuild(NoteDbUpdateManager manager, - ChangeBundle bundle) throws NoSuchChangeException, IOException, - OrmException, ConfigInvalidException { + public Result rebuild(NoteDbUpdateManager manager, + ChangeBundle bundle) { return null; } @Override public boolean rebuildProject(ReviewDb db, ImmutableMultimap allChanges, NameKey project, - Repository allUsersRepo) throws NoSuchChangeException, IOException, - OrmException, ConfigInvalidException { + Repository allUsersRepo) { return false; } }); 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 ec4daf7d87..68c116df87 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 @@ -103,6 +103,18 @@ public class NoteDbUpdateManager { public abstract ImmutableList allUsersObjects(); } + @AutoValue + public abstract static class Result { + static Result create(NoteDbUpdateManager.StagedResult staged, + NoteDbChangeState newState) { + return new AutoValue_NoteDbUpdateManager_Result(newState, staged); + } + + @Nullable public abstract NoteDbChangeState newState(); + + @Nullable abstract NoteDbUpdateManager.StagedResult staged(); + } + static class OpenRepo implements AutoCloseable { final Repository repo; final RevWalk rw; @@ -332,6 +344,14 @@ public class NoteDbUpdateManager { } } + public Result stageAndApplyDelta(Change change) + throws OrmException, IOException { + StagedResult sr = stage().get(change.getId()); + NoteDbChangeState newState = + NoteDbChangeState.applyDelta(change, sr != null ? sr.delta() : null); + return Result.create(sr, newState); + } + private Table getDraftIds() { Table draftIds = HashBasedTable.create(); if (allUsersRepo == null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java index f6351be055..666bf5adee 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/TestChangeRebuilderWrapper.java @@ -19,6 +19,7 @@ import com.google.common.collect.ImmutableMultimap; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.notedb.NoteDbUpdateManager.Result; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; @@ -50,10 +51,10 @@ public class TestChangeRebuilderWrapper extends ChangeRebuilder { } @Override - public NoteDbChangeState rebuild(ReviewDb db, Change.Id changeId) + public Result rebuild(ReviewDb db, Change.Id changeId) throws NoSuchChangeException, IOException, OrmException, ConfigInvalidException { - NoteDbChangeState result = delegate.rebuild(db, changeId); + Result result = delegate.rebuild(db, changeId); if (stealNextUpdate.getAndSet(false)) { throw new IOException("Update stolen"); } @@ -61,7 +62,7 @@ public class TestChangeRebuilderWrapper extends ChangeRebuilder { } @Override - public NoteDbChangeState rebuild(NoteDbUpdateManager manager, + public Result rebuild(NoteDbUpdateManager manager, ChangeBundle bundle) throws NoSuchChangeException, IOException, OrmException, ConfigInvalidException { // stealNextUpdate doesn't really apply in this case because the IOException