Merge changes from topic 'retry-helper-1'
* changes:
Add a helper for retrying BatchUpdates under NoteDb
BatchUpdateOp: Clarify docs around reuse
Pull BatchUpdate.Factory field up into AbstractDaemonTest
AbstractDaemonTest: Reorder fields
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 f4ee56e..e7a9568 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
@@ -98,6 +98,7 @@
import com.google.gerrit.server.project.Util;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
+import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gerrit.testutil.FakeEmailSender;
import com.google.gerrit.testutil.FakeEmailSender.Message;
@@ -164,84 +165,9 @@
private static GerritServer commonServer;
@ConfigSuite.Parameter public Config baseConfig;
-
@ConfigSuite.Name private String configName;
- @Inject protected AllProjectsName allProjects;
-
- @Inject protected AccountCreator accounts;
-
- @Inject private SchemaFactory<ReviewDb> reviewDbProvider;
-
- @Inject protected GerritApi gApi;
-
- @Inject protected AcceptanceTestRequestScope atrScope;
-
- @Inject protected AccountCache accountCache;
-
- @Inject protected IdentifiedUser.GenericFactory identifiedUserFactory;
-
- @Inject protected PushOneCommit.Factory pushFactory;
-
- @Inject protected MetaDataUpdate.Server metaDataUpdateFactory;
-
- @Inject protected ProjectCache projectCache;
-
- @Inject protected GroupCache groupCache;
-
- @Inject protected GitRepositoryManager repoManager;
-
- @Inject protected ChangeIndexer indexer;
-
- @Inject protected Provider<InternalChangeQuery> queryProvider;
-
- @Inject @CanonicalWebUrl protected Provider<String> canonicalWebUrl;
-
- @Inject @GerritServerConfig protected Config cfg;
-
- @Inject private InProcessProtocol inProcessProtocol;
-
- @Inject private Provider<AnonymousUser> anonymousUser;
-
- @Inject @GerritPersonIdent protected Provider<PersonIdent> serverIdent;
-
- @Inject protected ChangeData.Factory changeDataFactory;
-
- @Inject protected PatchSetUtil psUtil;
-
- @Inject protected ChangeFinder changeFinder;
-
- @Inject protected Revisions revisions;
-
- @Inject protected FakeEmailSender sender;
-
- @Inject protected ChangeNoteUtil changeNoteUtil;
-
- @Inject protected ChangeResource.Factory changeResourceFactory;
-
- @Inject protected SystemGroupBackend systemGroupBackend;
-
- @Inject private EventRecorder.Factory eventRecorderFactory;
-
- @Inject private ChangeIndexCollection changeIndexes;
-
- protected TestRepository<InMemoryRepository> testRepo;
- protected GerritServer server;
- protected TestAccount admin;
- protected TestAccount user;
- protected RestSession adminRestSession;
- protected RestSession userRestSession;
- protected SshSession adminSshSession;
- protected SshSession userSshSession;
- protected ReviewDb db;
- protected Project.NameKey project;
- protected EventRecorder eventRecorder;
-
- @Inject protected TestNotesMigration notesMigration;
-
- @Inject protected ChangeNotes.Factory notesFactory;
-
- @Inject protected Abandon changeAbandoner;
+ @Rule public ExpectedException exception = ExpectedException.none();
protected final TemporaryFolder tempSiteDir = new TemporaryFolder();
@@ -263,14 +189,57 @@
}
};
- @Rule
- public RuleChain ruleChain = RuleChain.outerRule(tempSiteDir).around(testRunner);
+ @Rule public RuleChain ruleChain = RuleChain.outerRule(tempSiteDir).around(testRunner);
- @Rule
- public ExpectedException exception = ExpectedException.none();
+ @Inject @CanonicalWebUrl protected Provider<String> canonicalWebUrl;
+ @Inject @GerritPersonIdent protected Provider<PersonIdent> serverIdent;
+ @Inject @GerritServerConfig protected Config cfg;
+ @Inject protected AcceptanceTestRequestScope atrScope;
+ @Inject protected AccountCache accountCache;
+ @Inject protected AccountCreator accounts;
+ @Inject protected AllProjectsName allProjects;
+ @Inject protected BatchUpdate.Factory batchUpdateFactory;
+ @Inject protected ChangeData.Factory changeDataFactory;
+ @Inject protected ChangeFinder changeFinder;
+ @Inject protected ChangeIndexer indexer;
+ @Inject protected ChangeNoteUtil changeNoteUtil;
+ @Inject protected ChangeResource.Factory changeResourceFactory;
+ @Inject protected FakeEmailSender sender;
+ @Inject protected GerritApi gApi;
+ @Inject protected GitRepositoryManager repoManager;
+ @Inject protected GroupCache groupCache;
+ @Inject protected IdentifiedUser.GenericFactory identifiedUserFactory;
+ @Inject protected MetaDataUpdate.Server metaDataUpdateFactory;
+ @Inject protected PatchSetUtil psUtil;
+ @Inject protected ProjectCache projectCache;
+ @Inject protected Provider<InternalChangeQuery> queryProvider;
+ @Inject protected PushOneCommit.Factory pushFactory;
+ @Inject protected Revisions revisions;
+ @Inject protected SystemGroupBackend systemGroupBackend;
+ @Inject protected TestNotesMigration notesMigration;
+ @Inject protected ChangeNotes.Factory notesFactory;
+ @Inject protected Abandon changeAbandoner;
- private String resourcePrefix;
+ protected EventRecorder eventRecorder;
+ protected GerritServer server;
+ protected Project.NameKey project;
+ protected RestSession adminRestSession;
+ protected RestSession userRestSession;
+ protected ReviewDb db;
+ protected SshSession adminSshSession;
+ protected SshSession userSshSession;
+ protected TestAccount admin;
+ protected TestAccount user;
+ protected TestRepository<InMemoryRepository> testRepo;
+
+ @Inject private ChangeIndexCollection changeIndexes;
+ @Inject private EventRecorder.Factory eventRecorderFactory;
+ @Inject private InProcessProtocol inProcessProtocol;
+ @Inject private Provider<AnonymousUser> anonymousUser;
+ @Inject private SchemaFactory<ReviewDb> reviewDbProvider;
+
private List<Repository> toClose;
+ private String resourcePrefix;
private boolean useSsh;
@Before
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index d790365..41b8f02 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -145,8 +145,6 @@
public class ChangeIT extends AbstractDaemonTest {
private String systemTimeZone;
- @Inject private BatchUpdate.Factory updateFactory;
-
@Inject private DynamicSet<ChangeMessageModifier> changeMessageModifiers;
@Before
@@ -2784,7 +2782,7 @@
private void setChangeStatus(Change.Id id, Change.Status newStatus) throws Exception {
try (BatchUpdate batchUpdate =
- updateFactory.create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
+ batchUpdateFactory.create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
batchUpdate.addOp(id, new ChangeStatusUpdateOp(newStatus));
batchUpdate.execute();
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index 359883a..4111a3d 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -114,8 +114,6 @@
@Inject private IdentifiedUser.GenericFactory userFactory;
- @Inject private BatchUpdate.Factory updateFactory;
-
@Inject private DynamicSet<OnSubmitValidationListener> onSubmitValidationListeners;
private RegistrationHandle onSubmitValidatorHandle;
@@ -807,7 +805,7 @@
private void setChangeStatusToNew(PushOneCommit.Result... changes) throws Exception {
for (PushOneCommit.Result change : changes) {
try (BatchUpdate bu =
- updateFactory.create(db, project, userFactory.create(admin.id), TimeUtil.nowTs())) {
+ batchUpdateFactory.create(db, project, userFactory.create(admin.id), TimeUtil.nowTs())) {
bu.addOp(
change.getChange().getId(),
new BatchUpdateOp() {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
index 26e6847..c2822ce 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
@@ -45,7 +45,6 @@
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.testutil.ConfigSuite;
-import com.google.inject.Inject;
import java.util.Collection;
import java.util.EnumSet;
import java.util.List;
@@ -58,8 +57,6 @@
return allowDraftsDisabledConfig();
}
- @Inject private BatchUpdate.Factory updateFactory;
-
@Test
public void deleteDraftChange() throws Exception {
assume().that(isAllowDrafts()).isTrue();
@@ -244,7 +241,7 @@
private void markChangeAsDraft(Change.Id id) throws Exception {
try (BatchUpdate batchUpdate =
- updateFactory.create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
+ batchUpdateFactory.create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
batchUpdate.addOp(id, new MarkChangeAsDraftUpdateOp());
batchUpdate.execute();
}
@@ -256,7 +253,7 @@
private void setDraftStatusOfPatchSetsOfChange(Change.Id id, boolean draftStatus)
throws Exception {
try (BatchUpdate batchUpdate =
- updateFactory.create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
+ batchUpdateFactory.create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) {
batchUpdate.addOp(id, new DraftStatusOfPatchSetsUpdateOp(draftStatus));
batchUpdate.execute();
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
index 329716f..8f256be 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
@@ -79,8 +79,6 @@
@Inject private IdentifiedUser.GenericFactory userFactory;
- @Inject private BatchUpdate.Factory updateFactory;
-
@Inject private ChangeInserter.Factory changeInserterFactory;
@Inject private PatchSetInserter.Factory patchSetInserterFactory;
@@ -784,7 +782,7 @@
}
private BatchUpdate newUpdate(Account.Id owner) {
- return updateFactory.create(db, project, userFactory.create(owner), TimeUtil.nowTs());
+ return batchUpdateFactory.create(db, project, userFactory.create(owner), TimeUtil.nowTs());
}
private ChangeControl insertChange() throws Exception {
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 fcbad4f..e957c88 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
@@ -64,8 +64,6 @@
System.setProperty("user.timezone", systemTimeZone);
}
- @Inject private BatchUpdate.Factory updateFactory;
-
@Inject private ChangesCollection changes;
@Test
@@ -578,7 +576,7 @@
}
private void clearGroups(final PatchSet.Id psId) throws Exception {
- try (BatchUpdate bu = updateFactory.create(db, project, user(user), TimeUtil.nowTs())) {
+ try (BatchUpdate bu = batchUpdateFactory.create(db, project, user(user), TimeUtil.nowTs())) {
bu.addOp(
psId.getParentKey(),
new BatchUpdateOp() {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
index 15b74bd..9560a44 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
@@ -139,8 +139,6 @@
@Inject private TestChangeRebuilderWrapper rebuilderWrapper;
- @Inject private BatchUpdate.Factory batchUpdateFactory;
-
@Inject private Sequences seq;
@Inject private ChangeBundleReader bundleReader;
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
index 4a5d496..2c61e64 100644
--- 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
@@ -20,30 +20,45 @@
import static com.google.common.truth.TruthJUnit.assume;
import static java.util.stream.Collectors.toList;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Streams;
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.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.BatchUpdateListener;
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.server.update.RetryHelper;
import com.google.inject.Inject;
import java.io.IOException;
+import java.util.Collections;
import java.util.EnumSet;
import java.util.List;
import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevSort;
+import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Before;
import org.junit.Test;
public class NoteDbOnlyIT extends AbstractDaemonTest {
- @Inject private BatchUpdate.Factory batchUpdateFactory;
+ @Inject private RetryHelper retryHelper;
@Before
public void setUp() throws Exception {
@@ -81,7 +96,7 @@
}
};
- try (BatchUpdate bu = newBatchUpdate()) {
+ try (BatchUpdate bu = newBatchUpdate(batchUpdateFactory)) {
bu.addOp(id, backupMasterOp);
bu.execute();
}
@@ -97,7 +112,7 @@
assertThat(master2).isNotEqualTo(master1);
int msgCount = getMessages(id).size();
- try (BatchUpdate bu = newBatchUpdate()) {
+ try (BatchUpdate bu = newBatchUpdate(batchUpdateFactory)) {
// This time, we attempt to back up master, but we fail during updateChange.
bu.addOp(id, backupMasterOp);
String msg = "Change is bad";
@@ -122,9 +137,175 @@
assertThat(getMessages(id)).hasSize(msgCount);
}
- private BatchUpdate newBatchUpdate() {
- return batchUpdateFactory.create(
- db, project, identifiedUserFactory.create(user.getId()), TimeUtil.nowTs());
+ @Test
+ public void retryOnLockFailureWithAtomicUpdates() throws Exception {
+ assume().that(notesMigration.fuseUpdates()).isTrue();
+ PushOneCommit.Result r = createChange();
+ Change.Id id = r.getChange().getId();
+ String master = "refs/heads/master";
+ ObjectId initial;
+ try (Repository repo = repoManager.openRepository(project)) {
+ ((InMemoryRepository) repo).setPerformsAtomicTransactions(true);
+ initial = repo.exactRef(master).getObjectId();
+ }
+
+ AtomicInteger updateRepoCalledCount = new AtomicInteger();
+ AtomicInteger updateChangeCalledCount = new AtomicInteger();
+ AtomicInteger afterUpdateReposCalledCount = new AtomicInteger();
+
+ String result =
+ retryHelper.execute(
+ batchUpdateFactory -> {
+ try (BatchUpdate bu = newBatchUpdate(batchUpdateFactory)) {
+ bu.addOp(
+ id,
+ new UpdateRefAndAddMessageOp(updateRepoCalledCount, updateChangeCalledCount));
+ bu.execute(new ConcurrentWritingListener(afterUpdateReposCalledCount));
+ }
+ return "Done";
+ });
+
+ assertThat(result).isEqualTo("Done");
+ assertThat(updateRepoCalledCount.get()).isEqualTo(2);
+ assertThat(afterUpdateReposCalledCount.get()).isEqualTo(2);
+ assertThat(updateChangeCalledCount.get()).isEqualTo(2);
+
+ List<String> messages = getMessages(id);
+ assertThat(Iterables.getLast(messages)).isEqualTo(UpdateRefAndAddMessageOp.CHANGE_MESSAGE);
+ assertThat(Collections.frequency(messages, UpdateRefAndAddMessageOp.CHANGE_MESSAGE))
+ .isEqualTo(1);
+
+ try (Repository repo = repoManager.openRepository(project)) {
+ // Op lost the race, so the other writer's commit happened first. Then op retried and wrote
+ // its commit with the other writer's commit as parent.
+ assertThat(commitMessages(repo, initial, repo.exactRef(master).getObjectId()))
+ .containsExactly(
+ ConcurrentWritingListener.MSG_PREFIX + "1", UpdateRefAndAddMessageOp.COMMIT_MESSAGE)
+ .inOrder();
+ }
+ }
+
+ @Test
+ public void noRetryOnLockFailureWithoutAtomicUpdates() throws Exception {
+ assume().that(notesMigration.fuseUpdates()).isFalse();
+
+ PushOneCommit.Result r = createChange();
+ Change.Id id = r.getChange().getId();
+ String master = "refs/heads/master";
+ ObjectId initial;
+ try (Repository repo = repoManager.openRepository(project)) {
+ initial = repo.exactRef(master).getObjectId();
+ }
+
+ AtomicInteger updateRepoCalledCount = new AtomicInteger();
+ AtomicInteger updateChangeCalledCount = new AtomicInteger();
+ AtomicInteger afterUpdateReposCalledCount = new AtomicInteger();
+
+ try {
+ retryHelper.execute(
+ batchUpdateFactory -> {
+ try (BatchUpdate bu = newBatchUpdate(batchUpdateFactory)) {
+ bu.addOp(
+ id, new UpdateRefAndAddMessageOp(updateRepoCalledCount, updateChangeCalledCount));
+ bu.execute(new ConcurrentWritingListener(afterUpdateReposCalledCount));
+ }
+ return null;
+ });
+ assert_().fail("expected RestApiException");
+ } catch (RestApiException e) {
+ // Expected.
+ }
+
+ assertThat(updateRepoCalledCount.get()).isEqualTo(1);
+ assertThat(afterUpdateReposCalledCount.get()).isEqualTo(1);
+ assertThat(updateChangeCalledCount.get()).isEqualTo(0);
+
+ // updateChange was never called, so no message was ever added.
+ assertThat(getMessages(id)).doesNotContain(UpdateRefAndAddMessageOp.CHANGE_MESSAGE);
+
+ try (Repository repo = repoManager.openRepository(project)) {
+ // Op lost the race, so the other writer's commit happened first. Op didn't retry, because the
+ // ref updates weren't atomic, so it didn't throw LockFailureException on failure.
+ assertThat(commitMessages(repo, initial, repo.exactRef(master).getObjectId()))
+ .containsExactly(ConcurrentWritingListener.MSG_PREFIX + "1");
+ }
+ }
+
+ private class ConcurrentWritingListener implements BatchUpdateListener {
+ static final String MSG_PREFIX = "Other writer ";
+
+ private final AtomicInteger calledCount;
+
+ private ConcurrentWritingListener(AtomicInteger calledCount) {
+ this.calledCount = calledCount;
+ }
+
+ @Override
+ public void afterUpdateRepos() throws Exception {
+ // Reopen repo and update ref, to simulate a concurrent write in another
+ // thread. Only do this the first time the listener is called.
+ if (calledCount.getAndIncrement() > 0) {
+ return;
+ }
+ try (Repository repo = repoManager.openRepository(project);
+ RevWalk rw = new RevWalk(repo);
+ ObjectInserter ins = repo.newObjectInserter()) {
+ String master = "refs/heads/master";
+ ObjectId oldId = repo.exactRef(master).getObjectId();
+ ObjectId newId = newCommit(rw, ins, oldId, MSG_PREFIX + calledCount.get());
+ ins.flush();
+ RefUpdate ru = repo.updateRef(master);
+ ru.setExpectedOldObjectId(oldId);
+ ru.setNewObjectId(newId);
+ assertThat(ru.update(rw)).isEqualTo(RefUpdate.Result.FAST_FORWARD);
+ }
+ }
+ }
+
+ private class UpdateRefAndAddMessageOp implements BatchUpdateOp {
+ static final String COMMIT_MESSAGE = "A commit";
+ static final String CHANGE_MESSAGE = "A change message";
+
+ private final AtomicInteger updateRepoCalledCount;
+ private final AtomicInteger updateChangeCalledCount;
+
+ private UpdateRefAndAddMessageOp(
+ AtomicInteger updateRepoCalledCount, AtomicInteger updateChangeCalledCount) {
+ this.updateRepoCalledCount = updateRepoCalledCount;
+ this.updateChangeCalledCount = updateChangeCalledCount;
+ }
+
+ @Override
+ public void updateRepo(RepoContext ctx) throws Exception {
+ String master = "refs/heads/master";
+ ObjectId oldId = ctx.getRepoView().getRef(master).get();
+ ObjectId newId = newCommit(ctx.getRevWalk(), ctx.getInserter(), oldId, COMMIT_MESSAGE);
+ ctx.addRefUpdate(oldId, newId, master);
+ updateRepoCalledCount.incrementAndGet();
+ }
+
+ @Override
+ public boolean updateChange(ChangeContext ctx) throws Exception {
+ ctx.getUpdate(ctx.getChange().currentPatchSetId()).setChangeMessage(CHANGE_MESSAGE);
+ updateChangeCalledCount.incrementAndGet();
+ return true;
+ }
+ }
+
+ private ObjectId newCommit(RevWalk rw, ObjectInserter ins, ObjectId parent, String msg)
+ throws IOException {
+ PersonIdent ident = serverIdent.get();
+ CommitBuilder cb = new CommitBuilder();
+ cb.setParentId(parent);
+ cb.setTreeId(rw.parseCommit(parent).getTree());
+ cb.setMessage(msg);
+ cb.setAuthor(ident);
+ cb.setCommitter(ident);
+ return ins.insert(Constants.OBJ_COMMIT, cb.build());
+ }
+
+ private BatchUpdate newBatchUpdate(BatchUpdate.Factory buf) {
+ return buf.create(db, project, identifiedUserFactory.create(user.getId()), TimeUtil.nowTs());
}
private Optional<ObjectId> getRef(String name) throws Exception {
@@ -142,4 +323,15 @@
.map(m -> m.message)
.collect(toList());
}
+
+ private static List<String> commitMessages(
+ Repository repo, ObjectId fromExclusive, ObjectId toInclusive) throws Exception {
+ try (RevWalk rw = new RevWalk(repo)) {
+ rw.markStart(rw.parseCommit(toInclusive));
+ rw.markUninteresting(rw.parseCommit(fromExclusive));
+ rw.sort(RevSort.REVERSE);
+ rw.setRetainBody(true);
+ return Streams.stream(rw).map(c -> c.getShortMessage()).collect(toList());
+ }
+ }
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java
index dd2e8af..d628268 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java
@@ -62,7 +62,6 @@
import com.google.gerrit.server.notedb.PrimaryStorageMigrator;
import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper;
import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gerrit.testutil.NoteDbMode;
import com.google.gerrit.testutil.TestTimeUtil;
@@ -95,7 +94,6 @@
}
@Inject private AllUsersName allUsers;
- @Inject private BatchUpdate.Factory batchUpdateFactory;
@Inject private ChangeBundleReader bundleReader;
@Inject private CommentsUtil commentsUtil;
@Inject private TestChangeRebuilderWrapper rebuilderWrapper;
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 22329fd..1d420bf 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
@@ -107,6 +107,7 @@
private final FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory;
private final UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory;
+ // TODO(dborowitz): Make this non-injectable to force all callers to use RetryHelper.
@Inject
Factory(
NotesMigration migration,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdateOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdateOp.java
index 39e25dd..87a43a3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdateOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdateOp.java
@@ -22,8 +22,11 @@
* BatchUpdate#addOp(com.google.gerrit.reviewdb.client.Change.Id, BatchUpdateOp)}.
*
* <p>Usually, a single {@code BatchUpdateOp} instance is only associated with a single change, i.e.
- * {@code addOp} is only called once with that instance. This allows an instance to communicate
- * between phases by storing data in private fields.
+ * {@code addOp} is only called once with that instance. Additionally, each method in {@code
+ * BatchUpdateOp} is called at most once per {@link BatchUpdate} execution.
+ *
+ * <p>Taken together, these two properties mean an instance may communicate between phases by
+ * storing data in private fields, and a single instance must not be reused.
*/
public interface BatchUpdateOp extends RepoOnlyOp {
/**
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java
new file mode 100644
index 0000000..dbbce2b
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java
@@ -0,0 +1,79 @@
+// 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;
+
+import com.github.rholder.retry.RetryException;
+import com.github.rholder.retry.RetryerBuilder;
+import com.github.rholder.retry.StopStrategies;
+import com.github.rholder.retry.WaitStrategies;
+import com.google.common.base.Throwables;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.git.LockFailureException;
+import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+
+@Singleton
+public class RetryHelper {
+ public interface Action<T> {
+ T call(BatchUpdate.Factory updateFactory) throws Exception;
+ }
+
+ private final BatchUpdate.Factory updateFactory;
+
+ @Inject
+ RetryHelper(
+ NotesMigration migration,
+ ReviewDbBatchUpdate.AssistedFactory reviewDbBatchUpdateFactory,
+ FusedNoteDbBatchUpdate.AssistedFactory fusedNoteDbBatchUpdateFactory,
+ UnfusedNoteDbBatchUpdate.AssistedFactory unfusedNoteDbBatchUpdateFactory) {
+ this.updateFactory =
+ new BatchUpdate.Factory(
+ migration,
+ reviewDbBatchUpdateFactory,
+ fusedNoteDbBatchUpdateFactory,
+ unfusedNoteDbBatchUpdateFactory);
+ }
+
+ public <T> T execute(Action<T> action) throws RestApiException, UpdateException {
+ try {
+ // TODO(dborowitz): Make configurable.
+ return RetryerBuilder.<T>newBuilder()
+ .withStopStrategy(StopStrategies.stopAfterDelay(20, TimeUnit.SECONDS))
+ .withWaitStrategy(
+ WaitStrategies.join(
+ WaitStrategies.exponentialWait(5, TimeUnit.SECONDS),
+ WaitStrategies.randomWait(50, TimeUnit.MILLISECONDS)))
+ .retryIfException(RetryHelper::isLockFailure)
+ .build()
+ .call(() -> action.call(updateFactory));
+ } catch (ExecutionException | RetryException e) {
+ if (e.getCause() != null) {
+ Throwables.throwIfInstanceOf(e.getCause(), UpdateException.class);
+ Throwables.throwIfInstanceOf(e.getCause(), RestApiException.class);
+ }
+ throw new UpdateException(e);
+ }
+ }
+
+ private static boolean isLockFailure(Throwable t) {
+ if (t instanceof UpdateException) {
+ t = t.getCause();
+ }
+ return t instanceof LockFailureException;
+ }
+}
