Merge "Update replication plugin to get fixes for IT flakiness"
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 8235bee..0535397 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -175,6 +175,7 @@
import java.util.Set;
import java.util.TreeMap;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.regex.Pattern;
import java.util.stream.Stream;
import java.util.zip.GZIPOutputStream;
@@ -291,7 +292,6 @@
private final Globals globals;
private final Provider<RestCollection<RestResource, RestResource>> members;
- private Optional<String> traceId = Optional.empty();
public RestApiServlet(
Globals globals, RestCollection<? extends RestResource, ? extends RestResource> members) {
@@ -666,7 +666,7 @@
responseBytes = replyError(req, res, status = SC_SERVICE_UNAVAILABLE, "Lock failure", e);
} else {
status = SC_INTERNAL_SERVER_ERROR;
- responseBytes = handleException(e, req, res);
+ responseBytes = handleException(traceContext, e, req, res);
}
} catch (QuotaException e) {
cause = Optional.of(e);
@@ -681,7 +681,7 @@
} catch (Exception e) {
cause = Optional.of(e);
status = SC_INTERNAL_SERVER_ERROR;
- responseBytes = handleException(e, req, res);
+ responseBytes = handleException(traceContext, e, req, res);
} finally {
String metric = getViewName(viewData);
String formattedCause = cause.map(globals.retryHelper::formatCause).orElse("_none");
@@ -814,6 +814,7 @@
ActionType actionType,
Action<T> action)
throws Exception {
+ AtomicReference<Optional<String>> traceId = new AtomicReference<>(Optional.empty());
RetryHelper.Options.Builder retryOptionsBuilder = RetryHelper.options().caller(caller);
if (!traceContext.isTracing()) {
// enable automatic retry with tracing in case of non-recoverable failure
@@ -822,7 +823,7 @@
.retryWithTrace(t -> !(t instanceof RestApiException))
.onAutoTrace(
autoTraceId -> {
- traceId = Optional.of(autoTraceId);
+ traceId.set(Optional.of(autoTraceId));
// Include details of the request into the trace.
traceRequestData(req);
@@ -838,7 +839,9 @@
// If auto-tracing got triggered due to a non-recoverable failure, also trace the rest of
// this request. This means logging is forced for all further log statements and the logs are
// associated with the same trace ID.
- traceId.ifPresent(tid -> traceContext.addTag(RequestId.Type.TRACE_ID, tid).forceLogging());
+ traceId
+ .get()
+ .ifPresent(tid -> traceContext.addTag(RequestId.Type.TRACE_ID, tid).forceLogging());
}
}
@@ -1660,12 +1663,13 @@
}
}
- private long handleException(Throwable err, HttpServletRequest req, HttpServletResponse res)
+ private long handleException(
+ TraceContext traceContext, Throwable err, HttpServletRequest req, HttpServletResponse res)
throws IOException {
logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uriForLogging(req));
if (!res.isCommitted()) {
res.reset();
- traceId.ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
+ traceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
StringBuilder msg = new StringBuilder("Internal server error");
ImmutableList<String> userMessages =
globals.exceptionHooks.stream()
diff --git a/java/com/google/gerrit/server/git/MergedByPushOp.java b/java/com/google/gerrit/server/git/MergedByPushOp.java
index c8b5e3f..9aebebf 100644
--- a/java/com/google/gerrit/server/git/MergedByPushOp.java
+++ b/java/com/google/gerrit/server/git/MergedByPushOp.java
@@ -52,6 +52,7 @@
MergedByPushOp create(
RequestScopePropagator requestScopePropagator,
PatchSet.Id psId,
+ @Assisted RequestId submissionId,
@Assisted("refName") String refName,
@Assisted("mergeResultRevId") String mergeResultRevId);
}
@@ -65,6 +66,7 @@
private final ChangeMerged changeMerged;
private final PatchSet.Id psId;
+ private final RequestId submissionId;
private final String refName;
private final String mergeResultRevId;
@@ -84,6 +86,7 @@
ChangeMerged changeMerged,
@Assisted RequestScopePropagator requestScopePropagator,
@Assisted PatchSet.Id psId,
+ @Assisted RequestId submissionId,
@Assisted("refName") String refName,
@Assisted("mergeResultRevId") String mergeResultRevId) {
this.patchSetInfoFactory = patchSetInfoFactory;
@@ -93,6 +96,7 @@
this.sendEmailExecutor = sendEmailExecutor;
this.changeMerged = changeMerged;
this.requestScopePropagator = requestScopePropagator;
+ this.submissionId = submissionId;
this.psId = psId;
this.refName = refName;
this.mergeResultRevId = mergeResultRevId;
@@ -133,7 +137,6 @@
}
change.setCurrentPatchSet(info);
change.setStatus(Change.Status.MERGED);
- RequestId submissionId = new RequestId(change.getId().toString());
change.setSubmissionId(submissionId.toStringForStorage());
// we cannot reconstruct the submit records for when this change was
// submitted, this is why we must fix the status and other details.
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 7dd21e1..cec9e4e 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -3281,6 +3281,7 @@
int existingPatchSets = 0;
int newPatchSets = 0;
+ RequestId submissionId = null;
COMMIT:
for (RevCommit c; (c = rw.next()) != null; ) {
rw.parseBody(c);
@@ -3289,13 +3290,20 @@
receivePackRefCache.tipsFromObjectId(c.copy(), RefNames.REFS_CHANGES)) {
PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName());
Optional<ChangeNotes> notes = getChangeNotes(psId.changeId());
+ if (submissionId == null) {
+ submissionId = new RequestId(psId.changeId().toString());
+ }
if (notes.isPresent() && notes.get().getChange().getDest().equals(branch)) {
existingPatchSets++;
bu.addOp(notes.get().getChangeId(), setPrivateOpFactory.create(false, null));
bu.addOp(
psId.changeId(),
mergedByPushOpFactory.create(
- requestScopePropagator, psId, refName, newTip.getId().getName()));
+ requestScopePropagator,
+ psId,
+ submissionId,
+ refName,
+ newTip.getId().getName()));
continue COMMIT;
}
}
@@ -3324,13 +3332,20 @@
logger.atFine().log("Not closing %s because validation failed", id);
continue;
}
+ if (submissionId == null) {
+ submissionId = new RequestId(id.toString());
+ }
req.addOps(bu, null);
bu.addOp(id, setPrivateOpFactory.create(false, null));
bu.addOp(
id,
mergedByPushOpFactory
.create(
- requestScopePropagator, req.psId, refName, newTip.getId().getName())
+ requestScopePropagator,
+ req.psId,
+ submissionId,
+ refName,
+ newTip.getId().getName())
.setPatchSetProvider(req.replaceOp::getPatchSet));
bu.addOp(id, new ChangeProgressOp(progress));
ids.add(id);
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index e95cf3b..6c0d5d3 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -61,6 +61,7 @@
import com.google.gerrit.server.extensions.events.RevisionCreated;
import com.google.gerrit.server.git.MergedByPushOp;
import com.google.gerrit.server.git.receive.ReceiveCommits.MagicBranchInput;
+import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.mail.MailUtil.MailRecipients;
import com.google.gerrit.server.mail.send.ReplacePatchSetSender;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -236,7 +237,11 @@
if (mergedInto != null) {
mergedByPushOp =
mergedByPushOpFactory.create(
- requestScopePropagator, patchSetId, mergedInto, mergeResultRevId);
+ requestScopePropagator,
+ patchSetId,
+ new RequestId(patchSetId.changeId().toString()),
+ mergedInto,
+ mergeResultRevId);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java b/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java
index 8040847..af65483 100644
--- a/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java
+++ b/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java
@@ -16,6 +16,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toMap;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
@@ -76,7 +77,15 @@
// Map of all patch sets, keyed by commit SHA-1.
Map<ObjectId, PatchSetData> byId = collectById(in);
PatchSetData start = byId.get(startPs.commitId());
- checkArgument(start != null, "%s not found in %s", startPs, in);
+ requireNonNull(
+ start,
+ () ->
+ String.format(
+ "commit %s of patch set %s not found in %s",
+ startPs.commitId().name(),
+ startPs.id(),
+ byId.entrySet().stream()
+ .collect(toMap(e -> e.getKey().name(), e -> e.getValue().patchSet().id()))));
// Map of patch set -> immediate parent.
ListMultimap<PatchSetData, PatchSetData> parents =
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index 47c6a7e..94c379b 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -160,10 +160,21 @@
"Revert this change and all changes that have been submitted together with this change")
.setVisible(
and(
- change.isMerged() && change.getSubmissionId() != null && projectStatePermitsWrite,
+ change.isMerged()
+ && change.getSubmissionId() != null
+ && isChangePartOfSubmission(change.getSubmissionId())
+ && projectStatePermitsWrite,
permissionBackend
.user(rsrc.getUser())
.ref(change.getDest())
.testCond(CREATE_CHANGE)));
}
+
+ /**
+ * @param submissionId the submission id of the change.
+ * @return True if the submission has more than one change, false otherwise.
+ */
+ private Boolean isChangePartOfSubmission(String submissionId) {
+ return (queryProvider.get().setLimit(2).bySubmissionId(submissionId).size() > 1);
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 7ecb07e..477ec38 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -3573,6 +3573,28 @@
}
@Test
+ public void checkSubmissionIdForAutoClosedChange() throws Exception {
+ PushOneCommit.Result first = createChange();
+ PushOneCommit.Result second = createChange();
+
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo);
+
+ PushOneCommit.Result result = push.to("refs/heads/master");
+ result.assertOkStatus();
+
+ ChangeInfo firstChange = gApi.changes().id(first.getChangeId()).get();
+ assertThat(firstChange.status).isEqualTo(ChangeStatus.MERGED);
+ assertThat(firstChange.submissionId).isNotNull();
+
+ ChangeInfo secondChange = gApi.changes().id(second.getChangeId()).get();
+ assertThat(secondChange.status).isEqualTo(ChangeStatus.MERGED);
+ assertThat(secondChange.submissionId).isNotNull();
+
+ assertThat(secondChange.submissionId).isEqualTo(firstChange.submissionId);
+ assertThat(gApi.changes().id(second.getChangeId()).submittedTogether()).hasSize(2);
+ }
+
+ @Test
public void maxPermittedValueAllowed() throws Exception {
final int minPermittedValue = -2;
final int maxPermittedValue = +2;
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
index 8b51e7f..911a04d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -72,13 +72,28 @@
}
@Test
- public void changeActionOneMergedChangeHasReverts() throws Exception {
+ public void changeActionOneMergedChangeHasOnlyNormalRevert() throws Exception {
String changeId = createChangeWithTopic().getChangeId();
gApi.changes().id(changeId).current().review(ReviewInput.approve());
gApi.changes().id(changeId).current().submit();
Map<String, ActionInfo> actions = getChangeActions(changeId);
assertThat(actions).containsKey("revert");
- assertThat(actions).containsKey("revert_submission");
+ assertThat(actions).doesNotContainKey("revert_submission");
+ }
+
+ @Test
+ public void changeActionTwoMergedChangesHaveReverts() throws Exception {
+ String changeId1 = createChangeWithTopic().getChangeId();
+ String changeId2 = createChangeWithTopic().getChangeId();
+ gApi.changes().id(changeId1).current().review(ReviewInput.approve());
+ gApi.changes().id(changeId2).current().review(ReviewInput.approve());
+ gApi.changes().id(changeId2).current().submit();
+ Map<String, ActionInfo> actions1 = getChangeActions(changeId1);
+ assertThat(actions1).containsKey("revert");
+ assertThat(actions1).containsKey("revert_submission");
+ Map<String, ActionInfo> actions2 = getChangeActions(changeId2);
+ assertThat(actions2).containsKey("revert");
+ assertThat(actions2).containsKey("revert_submission");
}
@Test
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
index b7e615c..f1338fb 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
@@ -92,7 +92,6 @@
a {
color: inherit;
cursor: pointer;
- display: inline-block;
text-decoration: none;
}
a:hover {
