opendev/gerrit
Code Issues Proposed changes

Revert "Convert MergeOp and related classes to use RetryHelper"

Browse Source 
This high-level approach fundamentally does not work for submitting
cross-project topics where some projects completely succeed prior to the
first lock failure.

For example, say we are submitting change 1 in project A and change 2 in
project B by clicking "Submit" on change 1. The submit to project A
succeeds, so change 1 is marked as merged, but the submit to project B
fails with a LockFailureException. We then retry the whole Submit#apply
using the normal RetryingRestModifyView magic. Unfortunately, this then
fails with the useless error message "Change 1 is merged".

This behavior is strictly worse than the old behavior of failing with an
opaque error message; at least in the case of an error message, the
server is telling the user that there was an error.

A more subtle approach is needed, and will be implemented in a followup.

This reverts commit 8e2729c091.

The revert is almost total, but required conflict resolution, and left
in a few TODO cleanups.

Change-Id: Ic527d9dc95139b16698795d6d7920032d1e70b48
This commit is contained in:
Dave Borowitz
2017-06-14 15:19:45 -04:00
parent 6c06b54f05
commit be2be20c83
7 changed files with 63 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files

71
gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java
View File

@@ -36,9 +36,6 @@ import com.google.gerrit.server.git.MergeOpRepoManager;
import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.UpdateException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -60,8 +57,7 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
private static final int MAX_DEFAULT_BUNDLE_SIZE = 100 * 1024 * 1024;
private final Provider<ReviewDb> dbProvider;
private final RetryHelper retryHelper;
private final MergeOp.Factory mergeOpFactory;
private final Provider<MergeOp> mergeOpProvider;
private final AllowedFormats allowedFormats;
private int maxBundleSize;
private String format;
@@ -74,52 +70,44 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
@Inject
PreviewSubmit(
Provider<ReviewDb> dbProvider,
RetryHelper retryHelper,
MergeOp.Factory mergeOpFactory,
Provider<MergeOp> mergeOpProvider,
AllowedFormats allowedFormats,
@GerritServerConfig Config cfg) {
this.dbProvider = dbProvider;
this.retryHelper = retryHelper;
this.mergeOpFactory = mergeOpFactory;
this.mergeOpProvider = mergeOpProvider;
this.allowedFormats = allowedFormats;
this.maxBundleSize = cfg.getInt("download", "maxBundleSize", MAX_DEFAULT_BUNDLE_SIZE);
}
@Override
public BinaryResult apply(RevisionResource rsrc) throws UpdateException, RestApiException {
// Shouldn't ever need to retry, since we are doing a dry-run BatchUpdate, but this is required
// to get access to a BatchUpdate.Factory.
return retryHelper.execute(
(updateFactory) -> {
if (Strings.isNullOrEmpty(format)) {
throw new BadRequestException("format is not specified");
}
ArchiveFormat f = allowedFormats.extensions.get("." + format);
if (f == null && format.equals("tgz")) {
// Always allow tgz, even when the allowedFormats doesn't contain it.
// Then we allow at least one format even if the list of allowed
// formats is empty.
f = ArchiveFormat.TGZ;
}
if (f == null) {
throw new BadRequestException("unknown archive format");
}
public BinaryResult apply(RevisionResource rsrc) throws OrmException, RestApiException {
if (Strings.isNullOrEmpty(format)) {
throw new BadRequestException("format is not specified");
}
ArchiveFormat f = allowedFormats.extensions.get("." + format);
if (f == null && format.equals("tgz")) {
// Always allow tgz, even when the allowedFormats doesn't contain it.
// Then we allow at least one format even if the list of allowed
// formats is empty.
f = ArchiveFormat.TGZ;
}
if (f == null) {
throw new BadRequestException("unknown archive format");
}
Change change = rsrc.getChange();
if (!change.getStatus().isOpen()) {
throw new PreconditionFailedException("change is " + ChangeUtil.status(change));
}
ChangeControl control = rsrc.getControl();
if (!control.getUser().isIdentifiedUser()) {
throw new MethodNotAllowedException("Anonymous users cannot submit");
}
Change change = rsrc.getChange();
if (!change.getStatus().isOpen()) {
throw new PreconditionFailedException("change is " + ChangeUtil.status(change));
}
ChangeControl control = rsrc.getControl();
if (!control.getUser().isIdentifiedUser()) {
throw new MethodNotAllowedException("Anonymous users cannot submit");
}
return getBundles(updateFactory, rsrc, f);
});
return getBundles(rsrc, f);
}
private BinaryResult getBundles(
BatchUpdate.Factory updateFactory, RevisionResource rsrc, ArchiveFormat f)
private BinaryResult getBundles(RevisionResource rsrc, ArchiveFormat f)
throws OrmException, RestApiException {
ReviewDb db = dbProvider.get();
ChangeControl control = rsrc.getControl();
@@ -127,7 +115,7 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
Change change = rsrc.getChange();
@SuppressWarnings("resource") // Returned BinaryResult takes ownership and handles closing.
MergeOp op = mergeOpFactory.create(updateFactory);
MergeOp op = mergeOpProvider.get();
try {
op.merge(db, change, caller, false, new SubmitInput(), true);
BinaryResult bin = new SubmitPreviewResult(op, f, maxBundleSize);
@@ -162,8 +150,7 @@ public class PreviewSubmit implements RestReadView<RevisionResource> {
BundleWriter bw = new BundleWriter(or.getCodeReviewRevWalk().getObjectReader());
bw.setObjectCountCallback(null);
bw.setPackConfig(new PackConfig(or.getRepo()));
Collection<ReceiveCommand> refs =
or.getUpdate(mergeOp.getBatchUpdateFactory()).getRefUpdates().values();
Collection<ReceiveCommand> refs = or.getUpdate().getRefUpdates().values();
for (ReceiveCommand r : refs) {
bw.include(r.getRefName(), r.getNewId());
ObjectId oldId = r.getOldId();

40
gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
View File

@@ -29,6 +29,7 @@ import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Branch;
@@ -45,7 +46,6 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.ProjectUtil;
import com.google.gerrit.server.account.AccountsCollection;
import com.google.gerrit.server.change.Submit.Output;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.ChangeSet;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -58,9 +58,6 @@ import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException;
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.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.OrmRuntimeException;
import com.google.inject.Inject;
@@ -84,8 +81,8 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Singleton
public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput, Output>
implements UiAction<RevisionResource> {
public class Submit
implements RestModifyView<RevisionResource, SubmitInput>, UiAction<RevisionResource> {
private static final Logger log = LoggerFactory.getLogger(Submit.class);
private static final String DEFAULT_TOOLTIP = "Submit patch set ${patchSet} into ${branch}";
@@ -128,7 +125,7 @@ public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput
private final ChangeData.Factory changeDataFactory;
private final ChangeMessagesUtil cmUtil;
private final ChangeNotes.Factory changeNotesFactory;
private final MergeOp.Factory mergeOpFactory;
private final Provider<MergeOp> mergeOpProvider;
private final Provider<MergeSuperSet> mergeSuperSet;
private final AccountsCollection accounts;
private final String label;
@@ -146,24 +143,22 @@ public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput
Provider<ReviewDb> dbProvider,
GitRepositoryManager repoManager,
PermissionBackend permissionBackend,
RetryHelper retryHelper,
ChangeData.Factory changeDataFactory,
ChangeMessagesUtil cmUtil,
ChangeNotes.Factory changeNotesFactory,
MergeOp.Factory mergeOpFactory,
Provider<MergeOp> mergeOpProvider,
Provider<MergeSuperSet> mergeSuperSet,
AccountsCollection accounts,
@GerritServerConfig Config cfg,
Provider<InternalChangeQuery> queryProvider,
PatchSetUtil psUtil) {
super(retryHelper);
this.dbProvider = dbProvider;
this.repoManager = repoManager;
this.permissionBackend = permissionBackend;
this.changeDataFactory = changeDataFactory;
this.cmUtil = cmUtil;
this.changeNotesFactory = changeNotesFactory;
this.mergeOpFactory = mergeOpFactory;
this.mergeOpProvider = mergeOpProvider;
this.mergeSuperSet = mergeSuperSet;
this.accounts = accounts;
this.label =
@@ -196,8 +191,7 @@ public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput
}
@Override
protected Output applyImpl(
BatchUpdate.Factory updateFactory, RevisionResource rsrc, SubmitInput input)
public Output apply(RevisionResource rsrc, SubmitInput input)
throws RestApiException, RepositoryNotFoundException, IOException, OrmException,
PermissionBackendException {
input.onBehalfOf = Strings.emptyToNull(input.onBehalfOf);
@@ -209,14 +203,10 @@ public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput
submitter = rsrc.getUser().asIdentifiedUser();
}
return new Output(mergeChange(updateFactory, rsrc, submitter, input));
return new Output(mergeChange(rsrc, submitter, input));
}
public Change mergeChange(
BatchUpdate.Factory updateFactory,
RevisionResource rsrc,
IdentifiedUser submitter,
SubmitInput input)
public Change mergeChange(RevisionResource rsrc, IdentifiedUser submitter, SubmitInput input)
throws OrmException, RestApiException, IOException {
Change change = rsrc.getChange();
if (!change.getStatus().isOpen()) {
@@ -231,7 +221,7 @@ public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput
"revision %s is not current revision", rsrc.getPatchSet().getRevision().get()));
}
try (MergeOp op = mergeOpFactory.create(updateFactory)) {
try (MergeOp op = mergeOpProvider.get()) {
ReviewDb db = dbProvider.get();
op.merge(db, change, submitter, true, input, false);
try {
@@ -507,8 +497,7 @@ public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput
}
}
public static class CurrentRevision
extends RetryingRestModifyView<ChangeResource, SubmitInput, ChangeInfo> {
public static class CurrentRevision implements RestModifyView<ChangeResource, SubmitInput> {
private final Provider<ReviewDb> dbProvider;
private final Submit submit;
private final ChangeJson.Factory json;
@@ -517,11 +506,9 @@ public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput
@Inject
CurrentRevision(
Provider<ReviewDb> dbProvider,
RetryHelper retryHelper,
Submit submit,
ChangeJson.Factory json,
PatchSetUtil psUtil) {
super(retryHelper);
this.dbProvider = dbProvider;
this.submit = submit;
this.json = json;
@@ -529,8 +516,7 @@ public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput
}
@Override
protected ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, SubmitInput input)
public ChangeInfo apply(ChangeResource rsrc, SubmitInput input)
throws RestApiException, RepositoryNotFoundException, IOException, OrmException,
PermissionBackendException {
PatchSet ps = psUtil.current(dbProvider.get(), rsrc.getNotes());
@@ -540,7 +526,7 @@ public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput
throw new AuthException("current revision not accessible");
}
Output out = submit.applyImpl(updateFactory, new RevisionResource(rsrc, ps), input);
Output out = submit.apply(new RevisionResource(rsrc, ps), input);
return json.noOptions().format(out.change);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
View File

@@ -115,7 +115,6 @@ import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.EmailMerge;
import com.google.gerrit.server.git.GitModule;
import com.google.gerrit.server.git.GitModules;
import com.google.gerrit.server.git.MergeOp;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.MergedByPushOp;
import com.google.gerrit.server.git.NotesBranchUtil;
@@ -401,7 +400,6 @@ public class GerritGlobalModule extends FactoryModule {
factory(MergedByPushOp.Factory.class);
factory(GitModules.Factory.class);
factory(VersionedAuthorizedKeys.Factory.class);
factory(MergeOp.Factory.class);
bind(AccountManager.class);
factory(ChangeUserName.Factory.class);

23
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -74,7 +74,6 @@ import com.google.gerrit.server.util.RequestId;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
@@ -109,10 +108,6 @@ public class MergeOp implements AutoCloseable {
private static final SubmitRuleOptions SUBMIT_RULE_OPTIONS = SubmitRuleOptions.defaults().build();
public interface Factory {
MergeOp create(BatchUpdate.Factory batchUpdateFactory);
}
public static class CommitStatus {
private final ImmutableMap<Change.Id, ChangeData> changes;
private final ImmutableSetMultimap<Branch.NameKey, Change.Id> byBranch;
@@ -244,6 +239,7 @@ public class MergeOp implements AutoCloseable {
@Inject
MergeOp(
ChangeMessagesUtil cmUtil,
BatchUpdate.Factory batchUpdateFactory,
InternalUser.Factory internalUserFactory,
MergeSuperSet mergeSuperSet,
MergeValidators.Factory mergeValidatorsFactory,
@@ -252,9 +248,9 @@ public class MergeOp implements AutoCloseable {
SubmoduleOp.Factory subOpFactory,
MergeOpRepoManager orm,
NotifyUtil notifyUtil,
TopicMetrics topicMetrics,
@Assisted BatchUpdate.Factory batchUpdateFactory) {
TopicMetrics topicMetrics) {
this.cmUtil = cmUtil;
this.batchUpdateFactory = batchUpdateFactory;
this.internalUserFactory = internalUserFactory;
this.mergeSuperSet = mergeSuperSet;
this.mergeValidatorsFactory = mergeValidatorsFactory;
@@ -263,7 +259,6 @@ public class MergeOp implements AutoCloseable {
this.subOpFactory = subOpFactory;
this.orm = orm;
this.notifyUtil = notifyUtil;
this.batchUpdateFactory = batchUpdateFactory;
this.topicMetrics = topicMetrics;
}
@@ -507,7 +502,7 @@ public class MergeOp implements AutoCloseable {
List<SubmitStrategy> strategies = getSubmitStrategies(toSubmit, submoduleOp, dryrun);
this.allProjects = submoduleOp.getProjectsInOrder();
batchUpdateFactory.execute(
orm.batchUpdates(batchUpdateFactory, allProjects),
orm.batchUpdates(allProjects),
new SubmitStrategyListener(submitInput, strategies, commitStatus),
submissionId,
dryrun);
@@ -545,10 +540,6 @@ public class MergeOp implements AutoCloseable {
return orm;
}
public BatchUpdate.Factory getBatchUpdateFactory() {
return batchUpdateFactory;
}
private List<SubmitStrategy> getSubmitStrategies(
Map<Branch.NameKey, BranchBatch> toSubmit, SubmoduleOp submoduleOp, boolean dryrun)
throws IntegrationException, NoSuchProjectException, IOException {
@@ -585,15 +576,15 @@ public class MergeOp implements AutoCloseable {
submoduleOp,
dryrun);
strategies.add(strategy);
strategy.addOps(or.getUpdate(batchUpdateFactory), commitsToSubmit);
strategy.addOps(or.getUpdate(), commitsToSubmit);
if (submitting.submitType().equals(SubmitType.FAST_FORWARD_ONLY)
&& submoduleOp.hasSubscription(branch)) {
submoduleOp.addOp(or.getUpdate(batchUpdateFactory), branch);
submoduleOp.addOp(or.getUpdate(), branch);
}
} else {
// no open change for this branch
// add submodule triggered op into BatchUpdate
submoduleOp.addOp(or.getUpdate(batchUpdateFactory), branch);
submoduleOp.addOp(or.getUpdate(), branch);
}
}
return strategies;

10
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
View File

@@ -101,7 +101,7 @@ public class MergeOpRepoManager implements AutoCloseable {
return rw;
}
public BatchUpdate getUpdate(BatchUpdate.Factory batchUpdateFactory) {
public BatchUpdate getUpdate() {
checkState(db != null, "call setContext before getUpdate");
if (update == null) {
update =
@@ -149,6 +149,7 @@ public class MergeOpRepoManager implements AutoCloseable {
}
private final Map<Project.NameKey, OpenRepo> openRepos;
private final BatchUpdate.Factory batchUpdateFactory;
private final OnSubmitValidators.Factory onSubmitValidatorsFactory;
private final GitRepositoryManager repoManager;
private final ProjectCache projectCache;
@@ -162,9 +163,11 @@ public class MergeOpRepoManager implements AutoCloseable {
MergeOpRepoManager(
GitRepositoryManager repoManager,
ProjectCache projectCache,
BatchUpdate.Factory batchUpdateFactory,
OnSubmitValidators.Factory onSubmitValidatorsFactory) {
this.repoManager = repoManager;
this.projectCache = projectCache;
this.batchUpdateFactory = batchUpdateFactory;
this.onSubmitValidatorsFactory = onSubmitValidatorsFactory;
openRepos = new HashMap<>();
@@ -199,12 +202,11 @@ public class MergeOpRepoManager implements AutoCloseable {
}
}
public List<BatchUpdate> batchUpdates(
BatchUpdate.Factory batchUpdateFactory, Collection<Project.NameKey> projects)
public List<BatchUpdate> batchUpdates(Collection<Project.NameKey> projects)
throws NoSuchProjectException, IOException {
List<BatchUpdate> updates = new ArrayList<>(projects.size());
for (Project.NameKey project : projects) {
updates.add(getRepo(project).getUpdate(batchUpdateFactory).setRefLogMessage("merged"));
updates.add(getRepo(project).getUpdate().setRefLogMessage("merged"));
}
return updates;
}

10
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -346,7 +346,7 @@ public class ReceiveCommits {
private Map<String, Ref> allRefs;
private final SubmoduleOp.Factory subOpFactory;
private final MergeOp.Factory mergeOpFactory;
private final Provider<MergeOp> mergeOpProvider;
private final Provider<MergeOpRepoManager> ormProvider;
private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
private final NotesMigration notesMigration;
@@ -400,7 +400,7 @@ public class ReceiveCommits {
@Assisted ProjectControl projectControl,
@Assisted Repository repo,
SubmoduleOp.Factory subOpFactory,
MergeOp.Factory mergeOpFactory,
Provider<MergeOp> mergeOpProvider,
Provider<MergeOpRepoManager> ormProvider,
DynamicMap<ProjectConfigEntry> pluginConfigEntries,
NotesMigration notesMigration,
@@ -448,7 +448,7 @@ public class ReceiveCommits {
this.receiveId = RequestId.forProject(project.getNameKey());
this.subOpFactory = subOpFactory;
this.mergeOpFactory = mergeOpFactory;
this.mergeOpProvider = mergeOpProvider;
this.ormProvider = ormProvider;
this.pluginConfigEntries = pluginConfigEntries;
this.notesMigration = notesMigration;
@@ -657,7 +657,7 @@ public class ReceiveCommits {
try (MergeOpRepoManager orm = ormProvider.get()) {
orm.setContext(db, TimeUtil.nowTs(), user, receiveId);
SubmoduleOp op = subOpFactory.create(branches, orm);
op.updateSuperProjects(batchUpdateFactory);
op.updateSuperProjects();
} catch (SubmoduleException e) {
logError("Can't update the superprojects", e);
}
@@ -2275,7 +2275,7 @@ public class ReceiveCommits {
tipChange, "tip of push does not correspond to a change; found these changes: %s", bySha);
logDebug(
"Processing submit with tip change {} ({})", tipChange.getId(), magicBranch.cmd.getNewId());
try (MergeOp op = mergeOpFactory.create(batchUpdateFactory)) {
try (MergeOp op = mergeOpProvider.get()) {
op.merge(db, tipChange, user, false, new SubmitInput(), false);
}
}

9
gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java
View File

@@ -355,7 +355,7 @@ public class SubmoduleOp {
return ret;
}
public void updateSuperProjects(BatchUpdate.Factory updateFactory) throws SubmoduleException {
public void updateSuperProjects() throws SubmoduleException {
ImmutableSet<Project.NameKey> projects = getProjectsInOrder();
if (projects == null) {
return;
@@ -370,15 +370,12 @@ public class SubmoduleOp {
// get a new BatchUpdate for the super project
OpenRepo or = orm.getRepo(project);
for (Branch.NameKey branch : branchesByProject.get(project)) {
addOp(or.getUpdate(updateFactory), branch);
addOp(or.getUpdate(), branch);
}
}
}
batchUpdateFactory.execute(
orm.batchUpdates(updateFactory, superProjects),
BatchUpdateListener.NONE,
orm.getSubmissionId(),
false);
orm.batchUpdates(superProjects), BatchUpdateListener.NONE, orm.getSubmissionId(), false);
} catch (RestApiException | UpdateException | IOException | NoSuchProjectException e) {
throw new SubmoduleException("Cannot update gitlinks", e);
}