opendev/gerrit
Code Issues Proposed changes

Merge changes from topic 'retry-helper-1'

Browse Source 
* changes:
  Add a helper for retrying BatchUpdates under NoteDb
  BatchUpdateOp: Clarify docs around reuse
  Pull BatchUpdate.Factory field up into AbstractDaemonTest
  AbstractDaemonTest: Reorder fields
This commit is contained in:
Dave Borowitz
2017-05-03 14:34:22 +00:00
committed by Gerrit Code Review
parent 406ea059d9 66ea2ed46e
commit c0ba40f6c9
12 changed files with 339 additions and 110 deletions
Download Patch File Download Diff File Expand all files Collapse all files

131
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -98,6 +98,7 @@ import com.google.gerrit.server.project.ProjectCache;
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 @@ public abstract class AbstractDaemonTest {
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 @@ public abstract class AbstractDaemonTest {
}
};
@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;
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 String resourcePrefix;
private List<Repository> toClose;
private String resourcePrefix;
private boolean useSsh;
@Before

4
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -145,8 +145,6 @@ import org.junit.Test;
public class ChangeIT extends AbstractDaemonTest {
private String systemTimeZone;
@Inject private BatchUpdate.Factory updateFactory;
@Inject private DynamicSet<ChangeMessageModifier> changeMessageModifiers;
@Before
@@ -2784,7 +2782,7 @@ public class ChangeIT extends AbstractDaemonTest {
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();
}

4
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
View File

@@ -114,8 +114,6 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
@Inject private IdentifiedUser.GenericFactory userFactory;
@Inject private BatchUpdate.Factory updateFactory;
@Inject private DynamicSet<OnSubmitValidationListener> onSubmitValidationListeners;
private RegistrationHandle onSubmitValidatorHandle;
@@ -807,7 +805,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
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() {

7
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java
View File

@@ -45,7 +45,6 @@ import com.google.gerrit.server.update.BatchUpdate;
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 @@ public class DraftChangeIT extends AbstractDaemonTest {
return allowDraftsDisabledConfig();
}
@Inject private BatchUpdate.Factory updateFactory;
@Test
public void deleteDraftChange() throws Exception {
assume().that(isAllowDrafts()).isTrue();
@@ -244,7 +241,7 @@ public class DraftChangeIT extends AbstractDaemonTest {
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 @@ public class DraftChangeIT extends AbstractDaemonTest {
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();
}

4
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
View File

@@ -79,8 +79,6 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
@Inject private IdentifiedUser.GenericFactory userFactory;
@Inject private BatchUpdate.Factory updateFactory;
@Inject private ChangeInserter.Factory changeInserterFactory;
@Inject private PatchSetInserter.Factory patchSetInserterFactory;
@@ -784,7 +782,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
}
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 {

4
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
View File

@@ -64,8 +64,6 @@ public class GetRelatedIT extends AbstractDaemonTest {
System.setProperty("user.timezone", systemTimeZone);
}
@Inject private BatchUpdate.Factory updateFactory;
@Inject private ChangesCollection changes;
@Test
@@ -578,7 +576,7 @@ public class GetRelatedIT extends AbstractDaemonTest {
}
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() {

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/ChangeRebuilderIT.java
View File

@@ -139,8 +139,6 @@ public class ChangeRebuilderIT extends AbstractDaemonTest {
@Inject private TestChangeRebuilderWrapper rebuilderWrapper;
@Inject private BatchUpdate.Factory batchUpdateFactory;
@Inject private Sequences seq;
@Inject private ChangeBundleReader bundleReader;

204
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbOnlyIT.java
View File

@@ -20,30 +20,45 @@ import static com.google.common.truth.Truth8.assertThat;
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 @@ public class NoteDbOnlyIT extends AbstractDaemonTest {
}
};
try (BatchUpdate bu = newBatchUpdate()) {
try (BatchUpdate bu = newBatchUpdate(batchUpdateFactory)) {
bu.addOp(id, backupMasterOp);
bu.execute();
}
@@ -97,7 +112,7 @@ public class NoteDbOnlyIT extends AbstractDaemonTest {
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 @@ public class NoteDbOnlyIT extends AbstractDaemonTest {
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 @@ public class NoteDbOnlyIT extends AbstractDaemonTest {
.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());
}
}
}

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java
View File

@@ -62,7 +62,6 @@ import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
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 @@ public class NoteDbPrimaryIT extends AbstractDaemonTest {
}
@Inject private AllUsersName allUsers;
@Inject private BatchUpdate.Factory batchUpdateFactory;
@Inject private ChangeBundleReader bundleReader;
@Inject private CommentsUtil commentsUtil;
@Inject private TestChangeRebuilderWrapper rebuilderWrapper;

1
gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdate.java
View File

@@ -107,6 +107,7 @@ public abstract class BatchUpdate implements AutoCloseable {
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,

7
gerrit-server/src/main/java/com/google/gerrit/server/update/BatchUpdateOp.java
View File

@@ -22,8 +22,11 @@ package com.google.gerrit.server.update;
* 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 {
/**

79
gerrit-server/src/main/java/com/google/gerrit/server/update/RetryHelper.java Normal file
View File

@@ -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;
}
}