Submit: Enable auto-tracing only on REST endpoint level
Retrying for submit is done on 2 levels, on REST endpoint level and inside MergeOp. It is sufficient to enable auto-tracing only once on the outer level (REST endpoint level). Ideally we would like to have only one level of retrying (on the REST endpoint level). Removing the retrying in MergeOp should be considered, but must be done carefully (e.g. this code is also invoked from other places like ReceiveCommits that may still need this retrying). Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: I4ed36136e01045cc1d600d7a989b7453c6dae52d
This commit is contained in:
committed by
David Pursehouse
parent
cec334a06f
commit
3e634628bb
@@ -19,7 +19,6 @@ import static java.util.stream.Collectors.joining;
|
||||
|
||||
import com.google.common.base.MoreObjects;
|
||||
import com.google.common.base.Strings;
|
||||
import com.google.common.base.Throwables;
|
||||
import com.google.common.collect.ImmutableMap;
|
||||
import com.google.common.collect.ListMultimap;
|
||||
import com.google.common.collect.Sets;
|
||||
@@ -190,7 +189,8 @@ public class Submit
|
||||
@UsedAt(UsedAt.Project.GOOGLE)
|
||||
public Response<Output> mergeChange(
|
||||
RevisionResource rsrc, IdentifiedUser submitter, SubmitInput input)
|
||||
throws RestApiException, IOException {
|
||||
throws RestApiException, IOException, UpdateException, ConfigInvalidException,
|
||||
PermissionBackendException {
|
||||
Change change = rsrc.getChange();
|
||||
if (!change.isNew()) {
|
||||
throw new ResourceConflictException("change is " + ChangeUtil.status(change));
|
||||
@@ -207,13 +207,7 @@ public class Submit
|
||||
try (MergeOp op = mergeOpProvider.get()) {
|
||||
Change updatedChange;
|
||||
|
||||
try {
|
||||
updatedChange = op.merge(change, submitter, true, input, false);
|
||||
} catch (Exception e) {
|
||||
Throwables.throwIfInstanceOf(e, RestApiException.class);
|
||||
return Response.<Output>internalServerError(e).traceId(op.getTraceId().orElse(null));
|
||||
}
|
||||
|
||||
updatedChange = op.merge(change, submitter, true, input, false);
|
||||
if (updatedChange.isMerged()) {
|
||||
return Response.ok(new Output(change));
|
||||
}
|
||||
@@ -471,12 +465,6 @@ public class Submit
|
||||
}
|
||||
|
||||
Response<Output> response = submit.apply(new RevisionResource(rsrc, ps), input);
|
||||
if (response instanceof Response.InternalServerError) {
|
||||
Response.InternalServerError<?> ise = (Response.InternalServerError<?>) response;
|
||||
return Response.<ChangeInfo>internalServerError(ise.cause())
|
||||
.traceId(ise.traceId().orElse(null));
|
||||
}
|
||||
|
||||
return Response.ok(json.noOptions().format(response.value().change));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -91,7 +91,6 @@ import java.util.HashSet;
|
||||
import java.util.LinkedHashSet;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Optional;
|
||||
import java.util.Set;
|
||||
import org.eclipse.jgit.errors.ConfigInvalidException;
|
||||
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
|
||||
@@ -248,7 +247,6 @@ public class MergeOp implements AutoCloseable {
|
||||
private Set<Project.NameKey> allProjects;
|
||||
private boolean dryrun;
|
||||
private TopicMetrics topicMetrics;
|
||||
private String traceId;
|
||||
|
||||
@Inject
|
||||
MergeOp(
|
||||
@@ -518,9 +516,6 @@ public class MergeOp implements AutoCloseable {
|
||||
retryHelper
|
||||
.getDefaultTimeout(ActionType.CHANGE_UPDATE)
|
||||
.multipliedBy(cs.projects().size()))
|
||||
.caller(getClass())
|
||||
.retryWithTrace(t -> !(t instanceof RestApiException))
|
||||
.onAutoTrace(traceId -> this.traceId = traceId)
|
||||
.build());
|
||||
|
||||
if (projects > 1) {
|
||||
@@ -541,10 +536,6 @@ public class MergeOp implements AutoCloseable {
|
||||
}
|
||||
}
|
||||
|
||||
public Optional<String> getTraceId() {
|
||||
return Optional.ofNullable(traceId);
|
||||
}
|
||||
|
||||
private void openRepoManager() {
|
||||
if (orm != null) {
|
||||
orm.close();
|
||||
|
||||
@@ -115,10 +115,6 @@ public class RetryHelper {
|
||||
|
||||
public abstract Builder caller(String caller);
|
||||
|
||||
public Builder caller(Class<?> caller) {
|
||||
return caller(caller.getSimpleName());
|
||||
}
|
||||
|
||||
public abstract Builder retryWithTrace(Predicate<Throwable> exceptionPredicate);
|
||||
|
||||
public abstract Builder onAutoTrace(Consumer<String> traceIdConsumer);
|
||||
|
||||
Reference in New Issue
Block a user