Revert "Migrate all emails to send async via asyncPostUpdate"
This reverts commit ce4e7c6609a6aaa51745350e2a6b0661793861b3.
Reason for revert:
The code has threading issues. For instance, we see an NPE for submitter in SubmitStrategyOp#asyncPostUpdate on real servers. Looking at the code, there doesn't seem to be any precautions for the use of the existing objects on different threads. Objects like SubmitStrategyOp are not built to be thread-safe at the moment. We can't simply pass them to another thread and expect them to work correctly.
Change-Id: If2e8c7c4d40231fdc9390f52c883401a1c0710cb
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index d96ae46..2cad722 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -182,20 +182,6 @@
+
By default 1.
-[[asyncPostUpdate]]
-=== Section asyncPostUpdate
-
-[[asyncPostUpdate.threadPoolSize]]asyncPostUpdate.threadPoolSize::
-+
-Maximum size of thread pool in which async post updates are sent out.
-+
-When set to 0, a direct executor is used.
-+
-When unset, use link:#sendemail[sendemail.threadPoolSize] threadPoolSize. When both
-are unset, use the default.
-+
-By default, 1.
-
[[auth]]
=== Section auth
@@ -884,8 +870,6 @@
The check if they are is cheap and always happens on the thread that
inquires for a cached value.
+
-When set to 0, a direct executor is used.
-+
Defaults to 2.
==== [[cache_names]]Standard Caches
@@ -4489,7 +4473,11 @@
an error occurs.
[[sendemail.threadPoolSize]]sendemail.threadPoolSize::
-Deprecated. Replaced with link:#asyncPostUpdate.threadPoolSize[asyncPostUpdate.threadPoolSize]
++
+Maximum size of thread pool in which the review comments
+notifications are sent out asynchronously.
++
+By default, 1.
[[sendemail.from]]sendemail.from::
+
diff --git a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
index 32bc992..452df67 100644
--- a/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractNotificationTest.java
@@ -48,7 +48,6 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
-import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import org.eclipse.jgit.junit.TestRepository;
@@ -98,7 +97,7 @@
protected static class FakeEmailSenderSubject extends Subject {
private final FakeEmailSender fakeEmailSender;
- private Optional<Message> message;
+ private Message message;
private StagedUsers users;
private Map<RecipientType, List<String>> recipients = new HashMap<>();
private Set<String> accountedFor = new HashSet<>();
@@ -117,29 +116,35 @@
}
public FakeEmailSenderSubject sent(String messageType, StagedUsers users) {
- fakeEmailSender.readOneMessage();
- message =
- fakeEmailSender.getMessages().stream()
- .filter(
- m ->
- m.headers()
- .get("X-Gerrit-MessageType")
- .equals(new EmailHeader.String(messageType)))
- .findFirst();
- if (!message.isPresent()) {
- failWithoutActual(
- fact(String.format("expected message of type %s", messageType), "not sent"));
+ message = fakeEmailSender.nextMessage();
+ if (message == null) {
+ failWithoutActual(fact("expected message", "not sent"));
}
recipients = new HashMap<>();
- recipients.put(TO, parseAddresses(message.get(), "To"));
- recipients.put(CC, parseAddresses(message.get(), "Cc"));
+ recipients.put(TO, parseAddresses(message, "To"));
+ recipients.put(CC, parseAddresses(message, "Cc"));
recipients.put(
BCC,
- message.get().rcpt().stream()
+ message.rcpt().stream()
.map(Address::email)
.filter(e -> !recipients.get(TO).contains(e) && !recipients.get(CC).contains(e))
.collect(toList()));
this.users = users;
+ if (!message.headers().containsKey("X-Gerrit-MessageType")) {
+ failWithoutActual(
+ fact("expected to have message sent with", "X-Gerrit-MessageType header"));
+ }
+ EmailHeader header = message.headers().get("X-Gerrit-MessageType");
+ if (!header.equals(new EmailHeader.String(messageType))) {
+ failWithoutActual(
+ fact("expected message of type", messageType),
+ fact(
+ "actual",
+ header instanceof EmailHeader.String
+ ? ((EmailHeader.String) header).getString()
+ : header));
+ }
+
return this;
}
diff --git a/java/com/google/gerrit/server/PublishCommentsOp.java b/java/com/google/gerrit/server/PublishCommentsOp.java
index e03c244..358ce92 100644
--- a/java/com/google/gerrit/server/PublishCommentsOp.java
+++ b/java/com/google/gerrit/server/PublishCommentsOp.java
@@ -28,7 +28,6 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.CommentsRejectedException;
@@ -47,7 +46,7 @@
* <p>This class uses the {@link PublishCommentUtil} to publish draft comments and fires the
* necessary event for this.
*/
-public class PublishCommentsOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class PublishCommentsOp implements BatchUpdateOp {
private final PatchSetUtil psUtil;
private final ChangeNotes.Factory changeNotesFactory;
private final ChangeMessagesUtil cmUtil;
@@ -62,8 +61,6 @@
private List<HumanComment> comments = new ArrayList<>();
private ChangeMessage message;
private IdentifiedUser user;
- private ChangeNotes changeNotes;
- private PatchSet patchset;
public interface Factory {
PublishCommentsOp create(PatchSet.Id psId, Project.NameKey projectNameKey);
@@ -112,24 +109,11 @@
@Override
public void postUpdate(Context ctx) {
- changeNotes = changeNotesFactory.createChecked(projectNameKey, psId.changeId());
- patchset = psUtil.get(changeNotes, psId);
-
- commentAdded.fire(
- changeNotes.getChange(),
- patchset,
- ctx.getAccount(),
- message.getMessage(),
- ImmutableMap.of(),
- ImmutableMap.of(),
- ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
if (message == null || comments.isEmpty()) {
return;
}
+ ChangeNotes changeNotes = changeNotesFactory.createChecked(projectNameKey, psId.changeId());
+ PatchSet ps = psUtil.get(changeNotes, psId);
NotifyResolver.Result notify = ctx.getNotify(changeNotes.getChangeId());
if (notify.shouldNotify()) {
RepoView repoView;
@@ -140,10 +124,17 @@
String.format("Repository %s not found", ctx.getProject().get()), ex);
}
email
- .create(
- notify, changeNotes, patchset, user, message, comments, null, labelDelta, repoView)
- .send();
+ .create(notify, changeNotes, ps, user, message, comments, null, labelDelta, repoView)
+ .sendAsync();
}
+ commentAdded.fire(
+ changeNotes.getChange(),
+ ps,
+ ctx.getAccount(),
+ message.getMessage(),
+ ImmutableMap.of(),
+ ImmutableMap.of(),
+ ctx.getWhen());
}
private boolean insertMessage(ChangeContext ctx, ChangeUpdate changeUpdate) {
diff --git a/java/com/google/gerrit/server/change/AbandonOp.java b/java/com/google/gerrit/server/change/AbandonOp.java
index e0e489b..6c39ed0 100644
--- a/java/com/google/gerrit/server/change/AbandonOp.java
+++ b/java/com/google/gerrit/server/change/AbandonOp.java
@@ -30,14 +30,13 @@
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.mail.send.ReplyToChangeSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
-public class AbandonOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class AbandonOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final AbandonedSender.Factory abandonedSenderFactory;
@@ -52,7 +51,6 @@
private Change change;
private PatchSet patchSet;
private ChangeMessage message;
- private NotifyResolver.Result notify;
public interface Factory {
AbandonOp create(
@@ -98,7 +96,6 @@
update.setStatus(change.getStatus());
message = newMessage(ctx);
cmUtil.addChangeMessage(update, message);
- notify = ctx.getNotify(change.getId());
return true;
}
@@ -115,11 +112,7 @@
@Override
public void postUpdate(Context ctx) {
- changeAbandoned.fire(change, patchSet, accountState, msgTxt, ctx.getWhen(), notify.handling());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
+ NotifyResolver.Result notify = ctx.getNotify(change.getId());
try {
ReplyToChangeSender emailSender =
abandonedSenderFactory.create(ctx.getProject(), change.getId());
@@ -134,5 +127,6 @@
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
}
+ changeAbandoned.fire(change, patchSet, accountState, msgTxt, ctx.getWhen(), notify.handling());
}
}
diff --git a/java/com/google/gerrit/server/change/AddReviewersEmail.java b/java/com/google/gerrit/server/change/AddReviewersEmail.java
index 5e48353..4a3f638 100644
--- a/java/com/google/gerrit/server/change/AddReviewersEmail.java
+++ b/java/com/google/gerrit/server/change/AddReviewersEmail.java
@@ -23,27 +23,34 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.mail.send.AddReviewerSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.Collection;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
@Singleton
public class AddReviewersEmail {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final AddReviewerSender.Factory addReviewerSenderFactory;
+ private final ExecutorService sendEmailsExecutor;
private final MessageIdGenerator messageIdGenerator;
@Inject
AddReviewersEmail(
- AddReviewerSender.Factory addReviewerSenderFactory, MessageIdGenerator messageIdGenerator) {
+ AddReviewerSender.Factory addReviewerSenderFactory,
+ @SendEmailExecutor ExecutorService sendEmailsExecutor,
+ MessageIdGenerator messageIdGenerator) {
this.addReviewerSenderFactory = addReviewerSenderFactory;
+ this.sendEmailsExecutor = sendEmailsExecutor;
this.messageIdGenerator = messageIdGenerator;
}
- public void emailReviewers(
+ public void emailReviewersAsync(
IdentifiedUser user,
Change change,
Collection<Account.Id> added,
@@ -71,20 +78,27 @@
ImmutableList<Address> immutableAddedByEmail = ImmutableList.copyOf(addedByEmail);
ImmutableList<Address> immutableCopiedByEmail = ImmutableList.copyOf(copiedByEmail);
- try {
- AddReviewerSender emailSender = addReviewerSenderFactory.create(projectNameKey, cId);
- emailSender.setNotify(notify);
- emailSender.setFrom(userId);
- emailSender.addReviewers(immutableToMail);
- emailSender.addReviewersByEmail(immutableAddedByEmail);
- emailSender.addExtraCC(immutableToCopy);
- emailSender.addExtraCCByEmail(immutableCopiedByEmail);
- emailSender.setMessageId(
- messageIdGenerator.fromChangeUpdate(change.getProject(), change.currentPatchSetId()));
- emailSender.send();
- } catch (Exception err) {
- logger.atSevere().withCause(err).log(
- "Cannot send email to new reviewers of change %s", change.getId());
- }
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ sendEmailsExecutor.submit(
+ () -> {
+ try {
+ AddReviewerSender emailSender =
+ addReviewerSenderFactory.create(projectNameKey, cId);
+ emailSender.setNotify(notify);
+ emailSender.setFrom(userId);
+ emailSender.addReviewers(immutableToMail);
+ emailSender.addReviewersByEmail(immutableAddedByEmail);
+ emailSender.addExtraCC(immutableToCopy);
+ emailSender.addExtraCCByEmail(immutableCopiedByEmail);
+ emailSender.setMessageId(
+ messageIdGenerator.fromChangeUpdate(
+ change.getProject(), change.currentPatchSetId()));
+ emailSender.send();
+ } catch (Exception err) {
+ logger.atSevere().withCause(err).log(
+ "Cannot send email to new reviewers of change %s", change.getId());
+ }
+ });
}
}
diff --git a/java/com/google/gerrit/server/change/AddReviewersOp.java b/java/com/google/gerrit/server/change/AddReviewersOp.java
index fd57722..ff8e5c6 100644
--- a/java/com/google/gerrit/server/change/AddReviewersOp.java
+++ b/java/com/google/gerrit/server/change/AddReviewersOp.java
@@ -42,7 +42,6 @@
import com.google.gerrit.server.extensions.events.ReviewerAdded;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -53,7 +52,7 @@
import java.util.List;
import java.util.Set;
-public class AddReviewersOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class AddReviewersOp implements BatchUpdateOp {
public interface Factory {
/**
@@ -239,7 +238,7 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
opResult =
Result.builder()
.setAddedReviewers(addedReviewers)
@@ -247,20 +246,8 @@
.setAddedCCs(addedCCs)
.setAddedCCsByEmail(addedCCsByEmail)
.build();
- if (!addedReviewers.isEmpty()) {
- List<AccountState> reviewers =
- addedReviewers.stream()
- .map(r -> accountCache.get(r.accountId()))
- .flatMap(Streams::stream)
- .collect(toList());
- reviewerAdded.fire(change, patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
- }
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
if (sendEmail) {
- addReviewersEmail.emailReviewers(
+ addReviewersEmail.emailReviewersAsync(
ctx.getUser().asIdentifiedUser(),
change,
Lists.transform(addedReviewers, PatchSetApproval::accountId),
@@ -269,6 +256,14 @@
addedCCsByEmail,
ctx.getNotify(change.getId()));
}
+ if (!addedReviewers.isEmpty()) {
+ List<AccountState> reviewers =
+ addedReviewers.stream()
+ .map(r -> accountCache.get(r.accountId()))
+ .flatMap(Streams::stream)
+ .collect(toList());
+ reviewerAdded.fire(change, patchSet, reviewers, ctx.getAccount(), ctx.getWhen());
+ }
}
public Result getResult() {
diff --git a/java/com/google/gerrit/server/change/AddToAttentionSetOp.java b/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
index 31147fc..8053b30 100644
--- a/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
+++ b/java/com/google/gerrit/server/change/AddToAttentionSetOp.java
@@ -25,7 +25,6 @@
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -35,7 +34,7 @@
import java.io.IOException;
/** Add a specified user to the attention set. */
-public class AddToAttentionSetOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class AddToAttentionSetOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -102,7 +101,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
if (!notify) {
return;
}
@@ -115,7 +114,7 @@
reason,
messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), change.currentPatchSetId()),
attentionUserId)
- .send();
+ .sendAsync();
} catch (IOException e) {
logger.atSevere().withCause(e).log(e.getMessage(), change.getId());
}
diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java
index 2f95a5c..a086cb1 100644
--- a/java/com/google/gerrit/server/change/ChangeInserter.java
+++ b/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -52,6 +52,7 @@
import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAdditionList;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.extensions.events.CommentAdded;
@@ -68,12 +69,12 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.ssh.NoSshInfo;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.InsertChangeOp;
import com.google.gerrit.server.update.RepoContext;
import com.google.gerrit.server.util.CommitMessageUtil;
+import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -83,13 +84,15 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
-public class ChangeInserter implements InsertChangeOp, AsyncPostUpdateOp {
+public class ChangeInserter implements InsertChangeOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -103,6 +106,7 @@
private final ApprovalsUtil approvalsUtil;
private final ChangeMessagesUtil cmUtil;
private final CreateChangeSender.Factory createChangeSenderFactory;
+ private final ExecutorService sendEmailExecutor;
private final CommitValidators.Factory commitValidatorsFactory;
private final RevisionCreated revisionCreated;
private final CommentAdded commentAdded;
@@ -126,6 +130,7 @@
private List<String> groups = Collections.emptyList();
private boolean validate = true;
private Map<String, Short> approvals;
+ private RequestScopePropagator requestScopePropagator;
private boolean fireRevisionCreated;
private boolean sendMail;
private boolean updateRef;
@@ -141,7 +146,6 @@
private String pushCert;
private ProjectState projectState;
private ReviewerAdditionList reviewerAdditions;
- private NotifyResolver.Result notify;
@Inject
ChangeInserter(
@@ -152,6 +156,7 @@
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil cmUtil,
CreateChangeSender.Factory createChangeSenderFactory,
+ @SendEmailExecutor ExecutorService sendEmailExecutor,
CommitValidators.Factory commitValidatorsFactory,
CommentAdded commentAdded,
RevisionCreated revisionCreated,
@@ -168,6 +173,7 @@
this.approvalsUtil = approvalsUtil;
this.cmUtil = cmUtil;
this.createChangeSenderFactory = createChangeSenderFactory;
+ this.sendEmailExecutor = sendEmailExecutor;
this.commitValidatorsFactory = commitValidatorsFactory;
this.revisionCreated = revisionCreated;
this.commentAdded = commentAdded;
@@ -309,6 +315,11 @@
return this;
}
+ public ChangeInserter setRequestScopePropagator(RequestScopePropagator r) {
+ this.requestScopePropagator = r;
+ return this;
+ }
+
public ChangeInserter setRevertOf(Change.Id revertOf) {
this.revertOf = revertOf;
return this;
@@ -446,13 +457,57 @@
ChangeMessagesUtil.uploadedPatchSetTag(workInProgress));
cmUtil.addChangeMessage(update, changeMessage);
}
- notify = ctx.getNotify(change.getId());
return true;
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
reviewerAdditions.postUpdate(ctx);
+ NotifyResolver.Result notify = ctx.getNotify(change.getId());
+ if (sendMail && notify.shouldNotify()) {
+ Runnable sender =
+ new Runnable() {
+ @Override
+ public void run() {
+ try {
+ CreateChangeSender emailSender =
+ createChangeSenderFactory.create(change.getProject(), change.getId());
+ emailSender.setFrom(change.getOwner());
+ emailSender.setPatchSet(patchSet, patchSetInfo);
+ emailSender.setNotify(notify);
+ emailSender.addReviewers(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedReviewers).stream()
+ .map(PatchSetApproval::accountId)
+ .collect(toImmutableSet()));
+ emailSender.addReviewersByEmail(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedReviewersByEmail));
+ emailSender.addExtraCC(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCs));
+ emailSender.addExtraCCByEmail(
+ reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCsByEmail));
+ emailSender.setMessageId(
+ messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), patchSet.id()));
+ emailSender.send();
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log(
+ "Cannot send email for new change %s", change.getId());
+ }
+ }
+
+ @Override
+ public String toString() {
+ return "send-email newchange";
+ }
+ };
+ if (requestScopePropagator != null) {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError =
+ sendEmailExecutor.submit(requestScopePropagator.wrap(sender));
+ } else {
+ sender.run();
+ }
+ }
+
/* For labels that are not set in this operation, show the "current" value
* of 0, and no oldValue as the value was not modified by this operation.
* For labels that are set in this operation, the value was modified, so
@@ -480,34 +535,6 @@
}
}
- @Override
- public void asyncPostUpdate(Context ctx) {
- reviewerAdditions.asyncPostUpdate(ctx);
- if (sendMail && notify.shouldNotify()) {
- try {
- CreateChangeSender emailSender =
- createChangeSenderFactory.create(change.getProject(), change.getId());
- emailSender.setFrom(change.getOwner());
- emailSender.setPatchSet(patchSet, patchSetInfo);
- emailSender.setNotify(notify);
- emailSender.addReviewers(
- reviewerAdditions.flattenResults(AddReviewersOp.Result::addedReviewers).stream()
- .map(PatchSetApproval::accountId)
- .collect(toImmutableSet()));
- emailSender.addReviewersByEmail(
- reviewerAdditions.flattenResults(AddReviewersOp.Result::addedReviewersByEmail));
- emailSender.addExtraCC(reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCs));
- emailSender.addExtraCCByEmail(
- reviewerAdditions.flattenResults(AddReviewersOp.Result::addedCCsByEmail));
- emailSender.setMessageId(
- messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), patchSet.id()));
- emailSender.send();
- } catch (Exception e) {
- logger.atSevere().withCause(e).log("Cannot send email for new change %s", change.getId());
- }
- }
- }
-
private void validate(RepoContext ctx) throws IOException, ResourceConflictException {
if (!validate) {
return;
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java b/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
index d9ede58..255e13a 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerByEmailOp.java
@@ -22,7 +22,6 @@
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.mail.send.DeleteReviewerSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -30,7 +29,7 @@
import com.google.inject.assistedinject.Assisted;
import java.util.Collections;
-public class DeleteReviewerByEmailOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class DeleteReviewerByEmailOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -74,7 +73,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
try {
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (!notify.shouldNotify()) {
diff --git a/java/com/google/gerrit/server/change/DeleteReviewerOp.java b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
index de11e07..07cb04f 100644
--- a/java/com/google/gerrit/server/change/DeleteReviewerOp.java
+++ b/java/com/google/gerrit/server/change/DeleteReviewerOp.java
@@ -43,7 +43,6 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.RemoveReviewerControl;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -56,7 +55,7 @@
import java.util.HashMap;
import java.util.Map;
-public class DeleteReviewerOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class DeleteReviewerOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -77,12 +76,11 @@
private final AccountState reviewer;
private final DeleteReviewerInput input;
- private ChangeMessage changeMessage;
- private Change currChange;
- private PatchSet currPs;
- private Map<String, Short> newApprovals = new HashMap<>();
- private Map<String, Short> oldApprovals = new HashMap<>();
- private NotifyResolver.Result notify;
+ ChangeMessage changeMessage;
+ Change currChange;
+ PatchSet currPs;
+ Map<String, Short> newApprovals = new HashMap<>();
+ Map<String, Short> oldApprovals = new HashMap<>();
@Inject
DeleteReviewerOp(
@@ -167,27 +165,13 @@
changeMessage =
ChangeMessagesUtil.newMessage(ctx, msg.toString(), ChangeMessagesUtil.TAG_DELETE_REVIEWER);
cmUtil.addChangeMessage(update, changeMessage);
- notify = ctx.getNotify(currChange.getId());
return true;
}
@Override
public void postUpdate(Context ctx) {
- reviewerDeleted.fire(
- currChange,
- currPs,
- reviewer,
- ctx.getAccount(),
- changeMessage.getMessage(),
- newApprovals,
- oldApprovals,
- notify.handling(),
- ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
+ NotifyResolver.Result notify = ctx.getNotify(currChange.getId());
if (input.notify == null
&& currChange.isWorkInProgress()
&& !oldApprovals.isEmpty()
@@ -203,6 +187,16 @@
} catch (Exception err) {
logger.atSevere().withCause(err).log("Cannot email update for change %s", currChange.getId());
}
+ reviewerDeleted.fire(
+ currChange,
+ currPs,
+ reviewer,
+ ctx.getAccount(),
+ changeMessage.getMessage(),
+ newApprovals,
+ oldApprovals,
+ notify.handling(),
+ ctx.getWhen());
}
private Iterable<PatchSetApproval> approvals(ChangeContext ctx, Account.Id accountId) {
diff --git a/java/com/google/gerrit/server/change/EmailReviewComments.java b/java/com/google/gerrit/server/change/EmailReviewComments.java
index 309b041..cacfbe7 100644
--- a/java/com/google/gerrit/server/change/EmailReviewComments.java
+++ b/java/com/google/gerrit/server/change/EmailReviewComments.java
@@ -21,18 +21,24 @@
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.mail.send.CommentSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.update.RepoView;
import com.google.gerrit.server.util.LabelVote;
+import com.google.gerrit.server.util.RequestContext;
+import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
-public class EmailReviewComments {
+public class EmailReviewComments implements Runnable, RequestContext {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -65,8 +71,10 @@
RepoView repoView);
}
+ private final ExecutorService sendEmailsExecutor;
private final PatchSetInfoFactory patchSetInfoFactory;
private final CommentSender.Factory commentSenderFactory;
+ private final ThreadLocalRequestContext requestContext;
private final MessageIdGenerator messageIdGenerator;
private final NotifyResolver.Result notify;
@@ -81,8 +89,10 @@
@Inject
EmailReviewComments(
+ @SendEmailExecutor ExecutorService executor,
PatchSetInfoFactory patchSetInfoFactory,
CommentSender.Factory commentSenderFactory,
+ ThreadLocalRequestContext requestContext,
MessageIdGenerator messageIdGenerator,
@Assisted NotifyResolver.Result notify,
@Assisted ChangeNotes notes,
@@ -93,8 +103,10 @@
@Nullable @Assisted String patchSetComment,
@Assisted List<LabelVote> labels,
@Assisted RepoView repoView) {
+ this.sendEmailsExecutor = executor;
this.patchSetInfoFactory = patchSetInfoFactory;
this.commentSenderFactory = commentSenderFactory;
+ this.requestContext = requestContext;
this.messageIdGenerator = messageIdGenerator;
this.notify = notify;
this.notes = notes;
@@ -107,7 +119,14 @@
this.repoView = repoView;
}
- public void send() {
+ public void sendAsync() {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(this);
+ }
+
+ @Override
+ public void run() {
+ RequestContext old = requestContext.setContext(this);
try {
CommentSender emailSender =
commentSenderFactory.create(notes.getProjectName(), notes.getChangeId());
@@ -124,6 +143,18 @@
emailSender.send();
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email comments for %s", patchSet.id());
+ } finally {
+ requestContext.setContext(old);
}
}
+
+ @Override
+ public String toString() {
+ return "send-email comments";
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ return user.getRealUser();
+ }
}
diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java
index 4502408..882352d 100644
--- a/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -49,7 +49,6 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.ssh.NoSshInfo;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -63,7 +62,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.transport.ReceiveCommand;
-public class PatchSetInserter implements BatchUpdateOp, AsyncPostUpdateOp {
+public class PatchSetInserter implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -110,7 +109,6 @@
private ChangeMessage changeMessage;
private ReviewerSet oldReviewers;
private boolean oldWorkInProgressState;
- private NotifyResolver.Result notify;
@Inject
public PatchSetInserter(
@@ -281,12 +279,12 @@
throw new BadRequestException(ex.getMessage());
}
}
- notify = ctx.getNotify(change.getId());
return true;
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
+ NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (notify.shouldNotify() && sendEmail) {
requireNonNull(changeMessage);
try {
@@ -306,10 +304,7 @@
"Cannot send email for new patch set on change %s", change.getId());
}
}
- }
- @Override
- public void postUpdate(Context ctx) {
if (fireRevisionCreated) {
revisionCreated.fire(change, patchSet, ctx.getAccount(), ctx.getWhen(), notify);
}
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index a8cc75a..231359b 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -34,7 +34,6 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -49,7 +48,7 @@
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
-public class RebaseChangeOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class RebaseChangeOp implements BatchUpdateOp {
public interface Factory {
RebaseChangeOp create(ChangeNotes notes, PatchSet originalPatchSet, ObjectId baseCommitId);
}
@@ -222,11 +221,6 @@
patchSetInserter.postUpdate(ctx);
}
- @Override
- public void asyncPostUpdate(Context ctx) {
- patchSetInserter.asyncPostUpdate(ctx);
- }
-
public RevCommit getRebasedCommit() {
checkState(rebasedCommit != null, "getRebasedCommit() only valid after updateRepo");
return rebasedCommit;
diff --git a/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java b/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
index 7f4e0e3..e532409 100644
--- a/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
+++ b/java/com/google/gerrit/server/change/RemoveFromAttentionSetOp.java
@@ -26,7 +26,6 @@
import com.google.gerrit.server.mail.send.RemoveFromAttentionSetSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -37,7 +36,7 @@
import java.util.Optional;
/** Remove a specified user from the attention set. */
-public class RemoveFromAttentionSetOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class RemoveFromAttentionSetOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -102,7 +101,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
if (!notify) {
return;
}
@@ -115,7 +114,7 @@
reason,
messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), change.currentPatchSetId()),
attentionUserId)
- .send();
+ .sendAsync();
} catch (IOException e) {
logger.atSevere().withCause(e).log(e.getMessage(), change.getId());
}
diff --git a/java/com/google/gerrit/server/change/ReviewerAdder.java b/java/com/google/gerrit/server/change/ReviewerAdder.java
index 3638bf2..3d986d2 100644
--- a/java/com/google/gerrit/server/change/ReviewerAdder.java
+++ b/java/com/google/gerrit/server/change/ReviewerAdder.java
@@ -587,7 +587,7 @@
}
}
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
for (ReviewerAddition addition : additions()) {
if (addition.op != null) {
addition.op.postUpdate(ctx);
@@ -595,14 +595,6 @@
}
}
- public void asyncPostUpdate(Context ctx) {
- for (ReviewerAddition addition : additions()) {
- if (addition.op != null) {
- addition.op.asyncPostUpdate(ctx);
- }
- }
- }
-
public <T> ImmutableSet<T> flattenResults(
Function<AddReviewersOp.Result, ? extends Collection<T>> func) {
additions()
diff --git a/java/com/google/gerrit/server/change/SetAssigneeOp.java b/java/com/google/gerrit/server/change/SetAssigneeOp.java
index c2e36f5..411c9b6 100644
--- a/java/com/google/gerrit/server/change/SetAssigneeOp.java
+++ b/java/com/google/gerrit/server/change/SetAssigneeOp.java
@@ -28,7 +28,6 @@
import com.google.gerrit.server.mail.send.SetAssigneeSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.plugincontext.PluginSetContext;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -38,7 +37,7 @@
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
-public class SetAssigneeOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class SetAssigneeOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -121,7 +120,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
try {
SetAssigneeSender emailSender =
setAssigneeSenderFactory.create(
@@ -134,10 +133,6 @@
logger.atSevere().withCause(err).log(
"Cannot send email to new assignee of change %s", change.getId());
}
- }
-
- @Override
- public void postUpdate(Context ctx) {
assigneeChanged.fire(
change, ctx.getAccount(), oldAssignee != null ? oldAssignee.state() : null, ctx.getWhen());
}
diff --git a/java/com/google/gerrit/server/change/WorkInProgressOp.java b/java/com/google/gerrit/server/change/WorkInProgressOp.java
index 99f7adb..f0ebb80 100644
--- a/java/com/google/gerrit/server/change/WorkInProgressOp.java
+++ b/java/com/google/gerrit/server/change/WorkInProgressOp.java
@@ -28,7 +28,6 @@
import com.google.gerrit.server.extensions.events.WorkInProgressStateChanged;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -38,7 +37,7 @@
import java.io.IOException;
/* Set work in progress or ready for review state on a change */
-public class WorkInProgressOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class WorkInProgressOp implements BatchUpdateOp {
public static class Input extends InputWithMessage {
@Nullable public NotifyHandling notify;
@@ -127,7 +126,8 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
+ stateChanged.fire(change, ps, ctx.getAccount(), ctx.getWhen());
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (workInProgress
|| notify.handling().compareTo(NotifyHandling.OWNER_REVIEWERS) < 0
@@ -152,11 +152,6 @@
cmsg.getMessage(),
ImmutableList.of(),
repoView)
- .send();
- }
-
- @Override
- public void postUpdate(Context ctx) {
- stateChanged.fire(change, ps, ctx.getAccount(), ctx.getWhen());
+ .sendAsync();
}
}
diff --git a/java/com/google/gerrit/server/config/AsyncPostUpdateExecutor.java b/java/com/google/gerrit/server/config/SendEmailExecutor.java
similarity index 90%
rename from java/com/google/gerrit/server/config/AsyncPostUpdateExecutor.java
rename to java/com/google/gerrit/server/config/SendEmailExecutor.java
index 7951b8c..cf90cbf 100644
--- a/java/com/google/gerrit/server/config/AsyncPostUpdateExecutor.java
+++ b/java/com/google/gerrit/server/config/SendEmailExecutor.java
@@ -1,4 +1,4 @@
-// Copyright (C) 2020 The Android Open Source Project
+// Copyright (C) 2014 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.
@@ -23,4 +23,4 @@
/** Marker on the global {@link ScheduledThreadPoolExecutor} used to send email. */
@Retention(RUNTIME)
@BindingAnnotation
-public @interface AsyncPostUpdateExecutor {}
+public @interface SendEmailExecutor {}
diff --git a/java/com/google/gerrit/server/config/SysExecutorModule.java b/java/com/google/gerrit/server/config/SysExecutorModule.java
index 5caf6fe..ea45b12 100644
--- a/java/com/google/gerrit/server/config/SysExecutorModule.java
+++ b/java/com/google/gerrit/server/config/SysExecutorModule.java
@@ -51,20 +51,14 @@
@Provides
@Singleton
- @AsyncPostUpdateExecutor
+ @SendEmailExecutor
public ExecutorService provideSendEmailExecutor(
@GerritServerConfig Config config, WorkQueue queues) {
- // sendemail.threadPoolSize is deprecated and overridden by asyncPostUpdate.threadPoolSize.
- int poolSize =
- config.getInt(
- "asyncPostUpdate",
- null,
- "threadPoolSize",
- config.getInt("sendemail", null, "threadPoolSize", 1));
+ int poolSize = config.getInt("sendemail", null, "threadPoolSize", 1);
if (poolSize == 0) {
return newDirectExecutorService();
}
- return queues.createQueue(poolSize, "AsyncPostUpdate", true);
+ return queues.createQueue(poolSize, "SendEmail", true);
}
@Provides
diff --git a/java/com/google/gerrit/server/git/CommitUtil.java b/java/com/google/gerrit/server/git/CommitUtil.java
index 7ddf16fd..0f46199 100644
--- a/java/com/google/gerrit/server/git/CommitUtil.java
+++ b/java/com/google/gerrit/server/git/CommitUtil.java
@@ -45,7 +45,6 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.notedb.Sequences;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -297,7 +296,7 @@
return changeId;
}
- private class NotifyOp implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class NotifyOp implements BatchUpdateOp {
private final Change change;
private final ChangeInserter ins;
@@ -307,12 +306,8 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
changeReverted.fire(change, ins.getChange(), ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
try {
RevertedSender emailSender = revertedSenderFactory.create(ctx.getProject(), change.getId());
emailSender.setFrom(ctx.getAccountId());
diff --git a/java/com/google/gerrit/server/git/MergedByPushOp.java b/java/com/google/gerrit/server/git/MergedByPushOp.java
index 13625bc..40e2730 100644
--- a/java/com/google/gerrit/server/git/MergedByPushOp.java
+++ b/java/com/google/gerrit/server/git/MergedByPushOp.java
@@ -25,19 +25,22 @@
import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.extensions.events.ChangeMerged;
import com.google.gerrit.server.mail.send.MergedSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
+import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -49,21 +52,24 @@
* <p>When we find a change corresponding to a commit that is pushed to a branch directly, we close
* the change. This class marks the change as merged, and sends out the email notification.
*/
-public class MergedByPushOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class MergedByPushOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
MergedByPushOp create(
+ RequestScopePropagator requestScopePropagator,
PatchSet.Id psId,
@Assisted SubmissionId submissionId,
@Assisted("refName") String refName,
@Assisted("mergeResultRevId") String mergeResultRevId);
}
+ private final RequestScopePropagator requestScopePropagator;
private final PatchSetInfoFactory patchSetInfoFactory;
private final ChangeMessagesUtil cmUtil;
private final MergedSender.Factory mergedSenderFactory;
private final PatchSetUtil psUtil;
+ private final ExecutorService sendEmailExecutor;
private final ChangeMerged changeMerged;
private final MessageIdGenerator messageIdGenerator;
@@ -84,8 +90,10 @@
ChangeMessagesUtil cmUtil,
MergedSender.Factory mergedSenderFactory,
PatchSetUtil psUtil,
+ @SendEmailExecutor ExecutorService sendEmailExecutor,
ChangeMerged changeMerged,
MessageIdGenerator messageIdGenerator,
+ @Assisted RequestScopePropagator requestScopePropagator,
@Assisted PatchSet.Id psId,
@Assisted SubmissionId submissionId,
@Assisted("refName") String refName,
@@ -94,8 +102,10 @@
this.cmUtil = cmUtil;
this.mergedSenderFactory = mergedSenderFactory;
this.psUtil = psUtil;
+ this.sendEmailExecutor = sendEmailExecutor;
this.changeMerged = changeMerged;
this.messageIdGenerator = messageIdGenerator;
+ this.requestScopePropagator = requestScopePropagator;
this.submissionId = submissionId;
this.psId = psId;
this.refName = refName;
@@ -171,27 +181,36 @@
if (!correctBranch) {
return;
}
+ @SuppressWarnings("unused") // Runnable already handles errors
+ Future<?> possiblyIgnoredError =
+ sendEmailExecutor.submit(
+ requestScopePropagator.wrap(
+ new Runnable() {
+ @Override
+ public void run() {
+ try {
+ MergedSender emailSender =
+ mergedSenderFactory.create(ctx.getProject(), psId.changeId());
+ emailSender.setFrom(ctx.getAccountId());
+ emailSender.setPatchSet(patchSet, info);
+ emailSender.setMessageId(
+ messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), patchSet.id()));
+ emailSender.send();
+ } catch (Exception e) {
+ logger.atSevere().withCause(e).log(
+ "Cannot send email for submitted patch set %s", psId);
+ }
+ }
+
+ @Override
+ public String toString() {
+ return "send-email merged";
+ }
+ }));
+
changeMerged.fire(change, patchSet, ctx.getAccount(), mergeResultRevId, ctx.getWhen());
}
- @Override
- public void asyncPostUpdate(Context ctx) {
- if (!correctBranch) {
- return;
- }
-
- try {
- MergedSender emailSender = mergedSenderFactory.create(ctx.getProject(), psId.changeId());
- emailSender.setFrom(ctx.getAccountId());
- emailSender.setPatchSet(patchSet, info);
- emailSender.setMessageId(
- messageIdGenerator.fromChangeUpdate(ctx.getRepoView(), patchSet.id()));
- emailSender.send();
- } catch (Exception e) {
- logger.atSevere().withCause(e).log("Cannot send email for submitted patch set %s", psId);
- }
- }
-
private PatchSetInfo getPatchSetInfo(ChangeContext ctx) throws IOException {
RevWalk rw = ctx.getRevWalk();
RevCommit commit = rw.parseCommit(requireNonNull(patchSet).commitId());
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index e44f422..015ed0b 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -175,6 +175,7 @@
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.LabelVote;
import com.google.gerrit.server.util.MagicBranch;
+import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.gerrit.server.validators.ValidationException;
import com.google.gerrit.util.cli.CmdLineParser;
@@ -338,6 +339,7 @@
private final PluginSetContext<RequestListener> requestListeners;
private final PublishCommentsOp.Factory publishCommentsOp;
private final RetryHelper retryHelper;
+ private final RequestScopePropagator requestScopePropagator;
private final Sequences seq;
private final SetHashtagsOp.Factory hashtagsFactory;
private final SubmoduleOp.Factory subOpFactory;
@@ -419,6 +421,7 @@
ReplaceOp.Factory replaceOpFactory,
PluginSetContext<RequestListener> requestListeners,
RetryHelper retryHelper,
+ RequestScopePropagator requestScopePropagator,
Sequences seq,
SetHashtagsOp.Factory hashtagsFactory,
SubmoduleOp.Factory subOpFactory,
@@ -467,6 +470,7 @@
this.replaceOpFactory = replaceOpFactory;
this.requestListeners = requestListeners;
this.retryHelper = retryHelper;
+ this.requestScopePropagator = requestScopePropagator;
this.seq = seq;
this.subOpFactory = subOpFactory;
this.tagCache = tagCache;
@@ -2582,6 +2586,7 @@
magicBranch.getCombinedCcs(fromFooters))
.setApprovals(approvals)
.setMessage(msg.toString())
+ .setRequestScopePropagator(requestScopePropagator)
.setSendMail(true)
.setPatchSetDescription(magicBranch.message));
if (!magicBranch.hashtags.isEmpty()) {
@@ -3008,20 +3013,22 @@
RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
replaceOp =
- replaceOpFactory.create(
- projectState,
- notes.getChange().getDest(),
- checkMergedInto,
- checkMergedInto ? inputCommand.getNewId().name() : null,
- priorPatchSet,
- priorCommit,
- psId,
- newCommit,
- info,
- groups,
- magicBranch,
- receivePack.getPushCertificate(),
- notes.getChange());
+ replaceOpFactory
+ .create(
+ projectState,
+ notes.getChange().getDest(),
+ checkMergedInto,
+ checkMergedInto ? inputCommand.getNewId().name() : null,
+ priorPatchSet,
+ priorCommit,
+ psId,
+ newCommit,
+ info,
+ groups,
+ magicBranch,
+ receivePack.getPushCertificate(),
+ notes.getChange())
+ .setRequestScopePropagator(requestScopePropagator);
bu.addOp(notes.getChangeId(), replaceOp);
if (progress != null) {
bu.addOp(notes.getChangeId(), new ChangeProgressOp(progress));
@@ -3302,7 +3309,11 @@
bu.addOp(
psId.changeId(),
mergedByPushOpFactory.create(
- psId, submissionId, refName, newTip.getId().getName()));
+ requestScopePropagator,
+ psId,
+ submissionId,
+ refName,
+ newTip.getId().getName()));
continue COMMIT;
}
}
@@ -3346,7 +3357,12 @@
bu.addOp(
id,
mergedByPushOpFactory
- .create(req.psId, submissionId, refName, newTip.getId().getName())
+ .create(
+ 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 87266f3..ce62d7a 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -56,6 +56,7 @@
import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAdditionList;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.extensions.events.CommentAdded;
import com.google.gerrit.server.extensions.events.RevisionCreated;
@@ -70,11 +71,11 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
import com.google.gerrit.server.update.RepoContext;
+import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -85,6 +86,8 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
@@ -93,7 +96,7 @@
import org.eclipse.jgit.transport.PushCertificate;
import org.eclipse.jgit.transport.ReceiveCommand;
-public class ReplaceOp implements BatchUpdateOp, AsyncPostUpdateOp {
+public class ReplaceOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -120,6 +123,7 @@
private final ChangeData.Factory changeDataFactory;
private final ChangeKindCache changeKindCache;
private final ChangeMessagesUtil cmUtil;
+ private final ExecutorService sendEmailExecutor;
private final RevisionCreated revisionCreated;
private final CommentAdded commentAdded;
private final MergedByPushOp.Factory mergedByPushOpFactory;
@@ -153,6 +157,7 @@
private ChangeMessage msg;
private String rejectMessage;
private MergedByPushOp mergedByPushOp;
+ private RequestScopePropagator requestScopePropagator;
private ReviewerAdditionList reviewerAdditions;
private MailRecipients oldRecipients;
@@ -169,6 +174,7 @@
PatchSetUtil psUtil,
ReplacePatchSetSender.Factory replacePatchSetFactory,
ProjectCache projectCache,
+ @SendEmailExecutor ExecutorService sendEmailExecutor,
ReviewerAdder reviewerAdder,
Change change,
MessageIdGenerator messageIdGenerator,
@@ -196,6 +202,7 @@
this.psUtil = psUtil;
this.replacePatchSetFactory = replacePatchSetFactory;
this.projectCache = projectCache;
+ this.sendEmailExecutor = sendEmailExecutor;
this.reviewerAdder = reviewerAdder;
this.change = change;
this.messageIdGenerator = messageIdGenerator;
@@ -232,7 +239,11 @@
if (mergedInto != null) {
mergedByPushOp =
mergedByPushOpFactory.create(
- patchSetId, new SubmissionId(change), mergedInto, mergeResultRevId);
+ requestScopePropagator,
+ patchSetId,
+ new SubmissionId(change),
+ mergedInto,
+ mergeResultRevId);
}
}
@@ -482,8 +493,18 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
reviewerAdditions.postUpdate(ctx);
+ if (changeKind != ChangeKind.TRIVIAL_REBASE) {
+ // TODO(dborowitz): Merge email templates so we only have to send one.
+ Runnable e = new ReplaceEmailTask(ctx);
+ if (requestScopePropagator != null) {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError = sendEmailExecutor.submit(requestScopePropagator.wrap(e));
+ } else {
+ e.run();
+ }
+ }
NotifyResolver.Result notify = ctx.getNotify(notes.getChangeId());
revisionCreated.fire(notes.getChange(), newPatchSet, ctx.getAccount(), ctx.getWhen(), notify);
try {
@@ -496,11 +517,15 @@
}
}
- @Override
- public void asyncPostUpdate(Context ctx) {
- reviewerAdditions.asyncPostUpdate(ctx);
- if (changeKind != ChangeKind.TRIVIAL_REBASE) {
- // TODO(dborowitz): Merge email templates so we only have to send one.
+ private class ReplaceEmailTask implements Runnable {
+ private final Context ctx;
+
+ private ReplaceEmailTask(Context ctx) {
+ this.ctx = ctx;
+ }
+
+ @Override
+ public void run() {
try {
ReplacePatchSetSender emailSender =
replacePatchSetFactory.create(projectState.getNameKey(), notes.getChangeId());
@@ -528,8 +553,10 @@
"Cannot send email for new patch set %s", newPatchSet.id());
}
}
- if (mergedByPushOp != null) {
- mergedByPushOp.asyncPostUpdate(ctx);
+
+ @Override
+ public String toString() {
+ return "send-email newpatchset";
}
}
@@ -586,6 +613,11 @@
return cmd;
}
+ public ReplaceOp setRequestScopePropagator(RequestScopePropagator requestScopePropagator) {
+ this.requestScopePropagator = requestScopePropagator;
+ return this;
+ }
+
private static String findMergedInto(Context ctx, String first, RevCommit commit) {
try {
RevWalk rw = ctx.getRevWalk();
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
index b8c6bf0..aea5308 100644
--- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java
+++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -64,7 +64,6 @@
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -315,7 +314,7 @@
}
}
- private class Op implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class Op implements BatchUpdateOp {
private final PatchSet.Id psId;
private final List<MailComment> parsedComments;
private final String tag;
@@ -360,26 +359,6 @@
@Override
public void postUpdate(Context ctx) throws Exception {
- // Get previous approvals from this user
- Map<String, Short> approvals = new HashMap<>();
- approvalsUtil
- .byPatchSetUser(
- notes, psId, ctx.getAccountId(), ctx.getRevWalk(), ctx.getRepoView().getConfig())
- .forEach(a -> approvals.put(a.label(), a.value()));
- // Fire Gerrit event. Note that approvals can't be granted via email, so old and new approvals
- // are always the same here.
- commentAdded.fire(
- notes.getChange(),
- patchSet,
- ctx.getAccount(),
- changeMessage.getMessage(),
- approvals,
- approvals,
- ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) throws Exception {
String patchSetComment = null;
if (parsedComments.get(0).getType() == MailComment.CommentType.CHANGE_MESSAGE) {
patchSetComment = parsedComments.get(0).getMessage();
@@ -396,7 +375,23 @@
patchSetComment,
ImmutableList.of(),
ctx.getRepoView())
- .send();
+ .sendAsync();
+ // Get previous approvals from this user
+ Map<String, Short> approvals = new HashMap<>();
+ approvalsUtil
+ .byPatchSetUser(
+ notes, psId, ctx.getAccountId(), ctx.getRevWalk(), ctx.getRepoView().getConfig())
+ .forEach(a -> approvals.put(a.label(), a.value()));
+ // Fire Gerrit event. Note that approvals can't be granted via email, so old and new approvals
+ // are always the same here.
+ commentAdded.fire(
+ notes.getChange(),
+ patchSet,
+ ctx.getAccount(),
+ changeMessage.getMessage(),
+ approvals,
+ approvals,
+ ctx.getWhen());
}
private ChangeMessage generateChangeMessage(ChangeContext ctx) {
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteVote.java b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
index 22c3567..4b813df 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteVote.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteVote.java
@@ -50,7 +50,6 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RemoveReviewerControl;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -147,7 +146,7 @@
return Response.none();
}
- private class Op implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class Op implements BatchUpdateOp {
private final ProjectState projectState;
private final AccountState accountState;
private final String label;
@@ -222,30 +221,17 @@
@Override
public void postUpdate(Context ctx) {
- voteDeleted.fire(
- change,
- ps,
- accountState,
- newApprovals,
- oldApprovals,
- input.notify,
- changeMessage.getMessage(),
- ctx.getIdentifiedUser().state(),
- ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
if (changeMessage == null) {
return;
}
+ IdentifiedUser user = ctx.getIdentifiedUser();
try {
NotifyResolver.Result notify = ctx.getNotify(change.getId());
if (notify.shouldNotify()) {
ReplyToChangeSender emailSender =
deleteVoteSenderFactory.create(ctx.getProject(), change.getId());
- emailSender.setFrom(ctx.getIdentifiedUser().getAccountId());
+ emailSender.setFrom(user.getAccountId());
emailSender.setChangeMessage(changeMessage.getMessage(), ctx.getWhen());
emailSender.setNotify(notify);
emailSender.setMessageId(
@@ -255,6 +241,17 @@
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
}
+
+ voteDeleted.fire(
+ change,
+ ps,
+ accountState,
+ newApprovals,
+ oldApprovals,
+ input.notify,
+ changeMessage.getMessage(),
+ user.state(),
+ ctx.getWhen());
}
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 9064dc8..4a6dfda 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -120,7 +120,6 @@
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -449,7 +448,7 @@
ccByEmail.addAll(addition.reviewersByEmail);
}
}
- addReviewersEmail.emailReviewers(
+ addReviewersEmail.emailReviewersAsync(
user.asIdentifiedUser(), change, to, cc, toByEmail, ccByEmail, notify);
}
}
@@ -860,7 +859,7 @@
abstract Comment.Range range();
}
- private class Op implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class Op implements BatchUpdateOp {
private final ProjectState projectState;
private final PatchSet.Id psId;
private final ReviewInput in;
@@ -906,7 +905,7 @@
}
@Override
- public void asyncPostUpdate(Context ctx) {
+ public void postUpdate(Context ctx) {
if (message == null) {
return;
}
@@ -924,20 +923,12 @@
in.message,
labelDelta,
ctx.getRepoView())
- .send();
+ .sendAsync();
} catch (IOException ex) {
throw new StorageException(
String.format("Repository %s not found", ctx.getProject().get()), ex);
}
}
- }
-
- @Override
- public void postUpdate(Context ctx) {
- if (message == null) {
- return;
- }
-
commentAdded.fire(
notes.getChange(),
ps,
diff --git a/java/com/google/gerrit/server/restapi/change/Restore.java b/java/com/google/gerrit/server/restapi/change/Restore.java
index 26b9d4c..7faf8e0 100644
--- a/java/com/google/gerrit/server/restapi/change/Restore.java
+++ b/java/com/google/gerrit/server/restapi/change/Restore.java
@@ -44,7 +44,6 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -109,7 +108,7 @@
return Response.ok(json.noOptions().format(op.change));
}
- private class Op implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class Op implements BatchUpdateOp {
private final RestoreInput input;
private Change change;
@@ -150,12 +149,6 @@
@Override
public void postUpdate(Context ctx) {
- changeRestored.fire(
- change, patchSet, ctx.getAccount(), Strings.emptyToNull(input.message), ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
try {
ReplyToChangeSender emailSender =
restoredSenderFactory.create(ctx.getProject(), change.getId());
@@ -167,6 +160,8 @@
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
}
+ changeRestored.fire(
+ change, patchSet, ctx.getAccount(), Strings.emptyToNull(input.message), ctx.getWhen());
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index 1f89b94..cb3a375 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -72,7 +72,6 @@
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.restapi.change.CherryPickChange.Result;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -589,7 +588,7 @@
}
}
- private class NotifyOp implements BatchUpdateOp, AsyncPostUpdateOp {
+ private class NotifyOp implements BatchUpdateOp {
private final Change change;
private final Change.Id revertChangeId;
@@ -599,13 +598,9 @@
}
@Override
- public void postUpdate(Context ctx) {
+ public void postUpdate(Context ctx) throws Exception {
changeReverted.fire(
change, changeNotesFactory.createChecked(revertChangeId).getChange(), ctx.getWhen());
- }
-
- @Override
- public void asyncPostUpdate(Context ctx) {
try {
RevertedSender emailSender = revertedSenderFactory.create(ctx.getProject(), change.getId());
emailSender.setFrom(ctx.getAccountId());
diff --git a/java/com/google/gerrit/server/submit/EmailMerge.java b/java/com/google/gerrit/server/submit/EmailMerge.java
index c7671b0..4efa4c8 100644
--- a/java/com/google/gerrit/server/submit/EmailMerge.java
+++ b/java/com/google/gerrit/server/submit/EmailMerge.java
@@ -19,14 +19,22 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.NotifyResolver;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.mail.send.MergedSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
import com.google.gerrit.server.update.RepoView;
+import com.google.gerrit.server.util.RequestContext;
+import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.Inject;
+import com.google.inject.OutOfScopeException;
import com.google.inject.assistedinject.Assisted;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
-class EmailMerge {
+class EmailMerge implements Runnable, RequestContext {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
interface Factory {
@@ -38,7 +46,10 @@
RepoView repoView);
}
+ private final ExecutorService sendEmailsExecutor;
private final MergedSender.Factory mergedSenderFactory;
+ private final ThreadLocalRequestContext requestContext;
+ private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final MessageIdGenerator messageIdGenerator;
private final Project.NameKey project;
@@ -49,14 +60,20 @@
@Inject
EmailMerge(
+ @SendEmailExecutor ExecutorService executor,
MergedSender.Factory mergedSenderFactory,
+ ThreadLocalRequestContext requestContext,
+ IdentifiedUser.GenericFactory identifiedUserFactory,
MessageIdGenerator messageIdGenerator,
@Assisted Project.NameKey project,
@Assisted Change change,
@Assisted @Nullable Account.Id submitter,
@Assisted NotifyResolver.Result notify,
@Assisted RepoView repoView) {
+ this.sendEmailsExecutor = executor;
this.mergedSenderFactory = mergedSenderFactory;
+ this.requestContext = requestContext;
+ this.identifiedUserFactory = identifiedUserFactory;
this.messageIdGenerator = messageIdGenerator;
this.project = project;
this.change = change;
@@ -65,7 +82,14 @@
this.repoView = repoView;
}
- public void send() {
+ void sendAsync() {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(this);
+ }
+
+ @Override
+ public void run() {
+ RequestContext old = requestContext.setContext(this);
try {
MergedSender emailSender = mergedSenderFactory.create(project, change.getId());
if (submitter != null) {
@@ -77,6 +101,21 @@
emailSender.send();
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email merged notification for %s", change.getId());
+ } finally {
+ requestContext.setContext(old);
}
}
+
+ @Override
+ public String toString() {
+ return "send-email merged";
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ if (submitter != null) {
+ return identifiedUserFactory.create(submitter).getRealUser();
+ }
+ throw new OutOfScopeException("No user on email thread");
+ }
}
diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
index 3629640..edc3725 100644
--- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
@@ -275,13 +275,6 @@
rebaseOp.postUpdate(ctx);
}
}
-
- @Override
- public void asyncPostUpdateImpl(Context ctx) {
- if (rebaseOp != null) {
- rebaseOp.asyncPostUpdate(ctx);
- }
- }
}
private class MergeIfNecessaryOp extends SubmitStrategyOp {
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index 201eba8..3cc566b 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -43,7 +43,6 @@
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.update.AsyncPostUpdateOp;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -61,7 +60,7 @@
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.ReceiveCommand;
-abstract class SubmitStrategyOp implements BatchUpdateOp, AsyncPostUpdateOp {
+abstract class SubmitStrategyOp implements BatchUpdateOp {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
protected final SubmitStrategy.Arguments args;
@@ -484,22 +483,8 @@
}
}
- if (mergeResultRev != null && !args.dryrun) {
- args.changeMerged.fire(
- updatedChange,
- mergedPatchSet,
- args.accountCache.get(submitter.accountId()).orElse(null),
- args.mergeTip.getCurrentTip().name(),
- ctx.getWhen());
- }
- }
-
- /**
- * Assume the change must have been merged at this point, otherwise we would have failed in one of
- * the other steps in postUpdate (which is done prior to this method).
- */
- @Override
- public final void asyncPostUpdate(Context ctx) {
+ // Assume the change must have been merged at this point, otherwise we would
+ // have failed fast in one of the other steps.
try {
args.mergedSenderFactory
.create(
@@ -508,11 +493,18 @@
submitter.accountId(),
ctx.getNotify(getId()),
ctx.getRepoView())
- .send();
+ .sendAsync();
} catch (Exception e) {
logger.atSevere().withCause(e).log("Cannot email merged notification for %s", getId());
}
- asyncPostUpdateImpl(ctx);
+ if (mergeResultRev != null && !args.dryrun) {
+ args.changeMerged.fire(
+ updatedChange,
+ mergedPatchSet,
+ args.accountCache.get(submitter.accountId()).orElse(null),
+ args.mergeTip.getCurrentTip().name(),
+ ctx.getWhen());
+ }
}
/**
@@ -537,12 +529,6 @@
protected void postUpdateImpl(Context ctx) throws Exception {}
/**
- * @see #asyncPostUpdate(Context)
- * @param ctx
- */
- protected void asyncPostUpdateImpl(Context ctx) {}
-
- /**
* Amend the commit with gitlink update
*
* @param commit
diff --git a/java/com/google/gerrit/server/update/AsyncPostUpdateOp.java b/java/com/google/gerrit/server/update/AsyncPostUpdateOp.java
deleted file mode 100644
index 9a989b7..0000000
--- a/java/com/google/gerrit/server/update/AsyncPostUpdateOp.java
+++ /dev/null
@@ -1,38 +0,0 @@
-// Copyright (C) 2020 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.google.gerrit.server.config.SysExecutorModule;
-
-/** Base interface for operations performed asynchronously as part of a {@link BatchUpdate}. */
-public interface AsyncPostUpdateOp {
-
- /**
- * Override this method to do something after the update e.g. send emails. This method will be
- * invoked asynchronously, and when invoked, the invoking method will not wait for the async
- * updates to finish. This method will be called after {@link BatchUpdateOp} operations and {@link
- * RepoOnlyOp} are finished.
- *
- * <p>The maximum amount of threads in the thread pool is decided by
- * asyncPostUpdate.threadPoolSize. When asyncPostUpdate.threadPoolSize is not specified, the
- * deprecated sendemail.threadPoolSize is used (see {@link
- * SysExecutorModule#provideSendEmailExecutor}). This is the case for legacy reasons, since in the
- * past only some emails were sent async (and sendemail.threadPoolSize) was used, and now all
- * emails (and possibly others) are done async, so asyncPostUpdate.threadPoolSize is used.
- *
- * @param ctx context
- */
- default void asyncPostUpdate(Context ctx) throws Exception {}
-}
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index 2fdc124..166e88d 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -47,7 +47,6 @@
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.change.NotifyResolver;
-import com.google.gerrit.server.config.AsyncPostUpdateExecutor;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
@@ -63,7 +62,6 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.NoSuchRefException;
-import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.Inject;
import com.google.inject.Module;
import com.google.inject.assistedinject.Assisted;
@@ -77,7 +75,6 @@
import java.util.Optional;
import java.util.TimeZone;
import java.util.TreeMap;
-import java.util.concurrent.ExecutorService;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
@@ -335,8 +332,6 @@
private final NoteDbUpdateManager.Factory updateManagerFactory;
private final ChangeIndexer indexer;
private final GitReferenceUpdated gitRefUpdated;
- private final ThreadLocalRequestContext requestContext;
- private final ExecutorService executorService;
private final Project.NameKey project;
private final CurrentUser user;
@@ -365,8 +360,6 @@
NoteDbUpdateManager.Factory updateManagerFactory,
ChangeIndexer indexer,
GitReferenceUpdated gitRefUpdated,
- ThreadLocalRequestContext requestContext,
- @AsyncPostUpdateExecutor ExecutorService executorService,
@Assisted Project.NameKey project,
@Assisted CurrentUser user,
@Assisted Timestamp when) {
@@ -376,8 +369,6 @@
this.updateManagerFactory = updateManagerFactory;
this.indexer = indexer;
this.gitRefUpdated = gitRefUpdated;
- this.requestContext = requestContext;
- this.executorService = executorService;
this.project = project;
this.user = user;
this.when = when;
@@ -646,40 +637,23 @@
return new ChangeContextImpl(notes);
}
- private void executePostOps() {
+ private void executePostOps() throws Exception {
ContextImpl ctx = new ContextImpl();
for (BatchUpdateOp op : ops.values()) {
- postUpdate(ctx, op);
- if (op instanceof AsyncPostUpdateOp) {
- asyncPostUpdate(ctx, ((AsyncPostUpdateOp) op));
+ try (TraceContext.TraceTimer ignored =
+ TraceContext.newTimer(op.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
+ op.postUpdate(ctx);
}
}
for (RepoOnlyOp op : repoOnlyOps) {
- postUpdate(ctx, op);
- if (op instanceof AsyncPostUpdateOp) {
- asyncPostUpdate(ctx, ((AsyncPostUpdateOp) op));
+ try (TraceContext.TraceTimer ignored =
+ TraceContext.newTimer(op.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
+ op.postUpdate(ctx);
}
}
}
- /** Invoke the postUpdate methods synchronously. */
- private void postUpdate(ContextImpl ctx, RepoOnlyOp op) {
- try (TraceContext.TraceTimer ignored =
- TraceContext.newTimer(op.getClass().getSimpleName() + "#postUpdate", Metadata.empty())) {
- op.postUpdate(ctx);
- } catch (Exception ex) {
- logDebug(
- String.format(
- "postUpdate for project %s failed for user %s", ctx.getProject(), ctx.getUser()));
- }
- }
-
- /** Invoke the asyncPostUpdate methods asynchronously. */
- private void asyncPostUpdate(ContextImpl ctx, AsyncPostUpdateOp op) {
- executorService.execute(new ExecuteAsyncPostUpdate(op, ctx, user, requestContext));
- }
-
private static void logDebug(String msg) {
// Only log if there is a requestId assigned, since those are the
// expensive/complicated requests like MergeOp. Doing it every time would be
diff --git a/java/com/google/gerrit/server/update/ExecuteAsyncPostUpdate.java b/java/com/google/gerrit/server/update/ExecuteAsyncPostUpdate.java
deleted file mode 100644
index e640ab1..0000000
--- a/java/com/google/gerrit/server/update/ExecuteAsyncPostUpdate.java
+++ /dev/null
@@ -1,65 +0,0 @@
-// Copyright (C) 2020 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.google.common.flogger.FluentLogger;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.util.RequestContext;
-import com.google.gerrit.server.util.ThreadLocalRequestContext;
-
-/** Executes {@link AsyncPostUpdateOp#asyncPostUpdate(Context)} on a specific op, asynchronously. */
-public class ExecuteAsyncPostUpdate implements Runnable, RequestContext {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- private final AsyncPostUpdateOp op;
- private final Context ctx;
- private final CurrentUser user;
- private final ThreadLocalRequestContext threadLocalRequestContext;
-
- ExecuteAsyncPostUpdate(
- AsyncPostUpdateOp op,
- Context ctx,
- CurrentUser user,
- ThreadLocalRequestContext threadLocalRequestContext) {
- this.op = op;
- this.ctx = ctx;
- this.user = user;
- this.threadLocalRequestContext = threadLocalRequestContext;
- }
-
- @Override
- public void run() {
- RequestContext old = threadLocalRequestContext.setContext(this);
- try {
- op.asyncPostUpdate(ctx);
- } catch (Exception e) {
- logger.atSevere().withCause(e).log(
- "Cannot perform async post update for repo %s and user %s",
- ctx.getProject(), ctx.getAccount().account().getName());
- } finally {
- threadLocalRequestContext.setContext(old);
- }
- }
-
- @Override
- public String toString() {
- return "async-post-update";
- }
-
- @Override
- public CurrentUser getUser() {
- return user;
- }
-}
diff --git a/java/com/google/gerrit/server/update/RepoOnlyOp.java b/java/com/google/gerrit/server/update/RepoOnlyOp.java
index f9b41c4..7e9c47e 100644
--- a/java/com/google/gerrit/server/update/RepoOnlyOp.java
+++ b/java/com/google/gerrit/server/update/RepoOnlyOp.java
@@ -30,11 +30,10 @@
default void updateRepo(RepoContext ctx) throws Exception {}
/**
- * Override this method to do something after the update e.g. run hooks. This method will
- * <strong>NOT</strong> be invoked asynchronously. This method will be finished before {@link
- * AsyncPostUpdateOp#asyncPostUpdate} is called.
+ * Override this method to do something after the update e.g. send email or run hooks
*
* @param ctx context
*/
+ // TODO(dborowitz): Support async operations?
default void postUpdate(Context ctx) throws Exception {}
}
diff --git a/java/com/google/gerrit/server/util/AttentionSetEmail.java b/java/com/google/gerrit/server/util/AttentionSetEmail.java
index 2b09c49..56b1dda 100644
--- a/java/com/google/gerrit/server/util/AttentionSetEmail.java
+++ b/java/com/google/gerrit/server/util/AttentionSetEmail.java
@@ -17,7 +17,9 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.mail.send.AddToAttentionSetSender;
import com.google.gerrit.server.mail.send.AttentionSetSender;
import com.google.gerrit.server.mail.send.MessageIdGenerator;
@@ -25,8 +27,10 @@
import com.google.gerrit.server.update.Context;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
-public class AttentionSetEmail {
+public class AttentionSetEmail implements Runnable, RequestContext {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public interface Factory {
@@ -51,6 +55,7 @@
Account.Id attentionUserId);
}
+ private ExecutorService sendEmailsExecutor;
private AttentionSetSender sender;
private Context ctx;
private Change change;
@@ -61,12 +66,14 @@
@Inject
AttentionSetEmail(
+ @SendEmailExecutor ExecutorService executor,
@Assisted AttentionSetSender sender,
@Assisted Context ctx,
@Assisted Change change,
@Assisted String reason,
@Assisted MessageIdGenerator.MessageId messageId,
@Assisted Account.Id attentionUserId) {
+ this.sendEmailsExecutor = executor;
this.sender = sender;
this.ctx = ctx;
this.change = change;
@@ -75,7 +82,13 @@
this.attentionUserId = attentionUserId;
}
- public void send() {
+ public void sendAsync() {
+ @SuppressWarnings("unused")
+ Future<?> possiblyIgnoredError = sendEmailsExecutor.submit(this);
+ }
+
+ @Override
+ public void run() {
try {
AccountState accountState =
ctx.getUser().isIdentifiedUser() ? ctx.getUser().asIdentifiedUser().state() : null;
@@ -91,4 +104,14 @@
logger.atSevere().withCause(e).log("Cannot email update for change %s", change.getId());
}
}
+
+ @Override
+ public String toString() {
+ return "send-email comments";
+ }
+
+ @Override
+ public CurrentUser getUser() {
+ return ctx.getUser();
+ }
}
diff --git a/java/com/google/gerrit/testing/FakeEmailSender.java b/java/com/google/gerrit/testing/FakeEmailSender.java
index f73020a..fec9b27 100644
--- a/java/com/google/gerrit/testing/FakeEmailSender.java
+++ b/java/com/google/gerrit/testing/FakeEmailSender.java
@@ -128,7 +128,6 @@
}
public synchronized @Nullable Message peekMessage() {
- waitForEmails();
if (messagesRead >= messages.size()) {
return null;
}
@@ -136,14 +135,9 @@
}
public synchronized @Nullable Message nextMessage() {
- waitForEmails();
Message msg = peekMessage();
- readOneMessage();
- return msg;
- }
-
- public synchronized void readOneMessage() {
messagesRead++;
+ return msg;
}
public ImmutableList<Message> getMessages() {
@@ -166,7 +160,7 @@
// a single thread in tests (tricky because most callers just use the
// default executor).
for (WorkQueue.Task<?> task : workQueue.getTasks()) {
- if (task.toString().contains("async-post-update")) {
+ if (task.toString().contains("send-email")) {
try {
task.get();
} catch (ExecutionException | InterruptedException e) {
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 693fd90..6c9fbed 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -47,7 +47,6 @@
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
-import com.google.gerrit.server.config.AsyncPostUpdateExecutor;
import com.google.gerrit.server.config.CanonicalWebUrlModule;
import com.google.gerrit.server.config.CanonicalWebUrlProvider;
import com.google.gerrit.server.config.DefaultUrlFormatter;
@@ -59,6 +58,7 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.config.GerritServerIdProvider;
+import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.config.SitePath;
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.config.TrackingFootersProvider;
@@ -279,7 +279,7 @@
@Provides
@Singleton
- @AsyncPostUpdateExecutor
+ @SendEmailExecutor
public ExecutorService createSendEmailExecutor() {
return newDirectExecutorService();
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 0bfd525..d4affb7 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -284,11 +284,6 @@
String fileName = "a_new_file.txt";
String fileContent = "First line\nSecond line\n";
PushOneCommit.Result result = createChange("Add a file", fileName, fileContent);
-
- // Emails are sent here async which triggers cache hits, so we must wait until those email are
- // actually sent.
- sender.clear();
-
String triplet = project.get() + "~master~" + result.getChangeId();
CacheStats startPatch = cloneStats(fileCache.stats());
CacheStats startIntra = cloneStats(intraCache.stats());
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java b/javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java
index aafe9b9..23bcdec 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractSubmitOnPush.java
@@ -412,17 +412,17 @@
sender.clear();
result = pushTo(pushSpec + ",submit,notify=" + NotifyHandling.OWNER_REVIEWERS);
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(result.getChangeId(), user, null);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user, null);
sender.clear();
result = pushTo(pushSpec + ",submit,notify=" + NotifyHandling.ALL);
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(result.getChangeId(), user, null);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user, null);
sender.clear();
result = pushTo(pushSpec + ",submit"); // default is notify = ALL
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(result.getChangeId(), user, null);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user, null);
}
@Test
@@ -442,22 +442,19 @@
PushOneCommit.Result result =
pushTo(pushSpec + ",submit,notify=" + NotifyHandling.NONE + ",notify-to=" + user2.email());
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(
- result.getChangeId(), user2, RecipientType.TO);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user2, RecipientType.TO);
sender.clear();
result =
pushTo(pushSpec + ",submit,notify=" + NotifyHandling.NONE + ",notify-cc=" + user2.email());
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(
- result.getChangeId(), user2, RecipientType.CC);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user2, RecipientType.CC);
sender.clear();
result =
pushTo(pushSpec + ",submit,notify=" + NotifyHandling.NONE + ",notify-bcc=" + user2.email());
result.assertOkStatus();
- assertThatEmailsForChangeCreationAndSubmitWereSent(
- result.getChangeId(), user2, RecipientType.BCC);
+ assertThatEmailsForChangeCreationAndSubmitWereSent(user2, RecipientType.BCC);
}
private PatchSetApproval getSubmitter(PatchSet.Id patchSetId) throws Exception {
@@ -518,15 +515,15 @@
* sent as "To" and sometimes can be sent as "Cc".
*/
private void assertThatEmailsForChangeCreationAndSubmitWereSent(
- String changeId, TestAccount expected, @Nullable RecipientType expectedRecipientType) {
+ TestAccount expected, @Nullable RecipientType expectedRecipientType) {
String expectedEmail = expected.email();
String expectedFullName = expected.fullName();
Address expectedAddress = Address.create(expectedFullName, expectedEmail);
assertThat(sender.getMessages()).hasSize(2);
- Message message = Iterables.getOnlyElement(sender.getMessages(changeId, "newchange"));
+ Message message = sender.getMessages().get(0);
assertThat(message.body().contains("review")).isTrue();
assertAddress(message, expectedAddress, expectedRecipientType);
- message = Iterables.getOnlyElement(sender.getMessages(changeId, "merged"));
+ message = sender.getMessages().get(1);
assertThat(message.rcpt()).containsExactly(expectedAddress);
assertAddress(message, expectedAddress, expectedRecipientType);
assertThat(message.body().contains("submitted")).isTrue();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
index e040860..a6bd5eb 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
@@ -28,7 +28,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
@@ -399,13 +398,13 @@
List<Message> messages = sender.getMessages();
assertThat(messages).hasSize(2);
- Message m = Iterables.getOnlyElement(sender.getMessages(r.getChangeId(), "comment"));
+ Message m = messages.get(0);
assertThat(m.rcpt()).containsExactly(user.getNameEmail(), observer.getNameEmail());
assertThat(m.body()).contains(admin.fullName() + " has posted comments on this change.");
assertThat(m.body()).contains("Change subject: " + PushOneCommit.SUBJECT + "\n");
assertThat(m.body()).contains("Patch Set 1: Code-Review+2");
- m = Iterables.getOnlyElement(sender.getMessages(r.getChangeId(), "newchange"));
+ m = messages.get(1);
assertThat(m.rcpt()).containsExactly(user.getNameEmail(), observer.getNameEmail());
assertThat(m.body()).contains("Hello " + user.fullName() + ",\n");
assertThat(m.body()).contains("I'd like you to do a code review.");
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
index 7570ce9..1a01184 100644
--- a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
@@ -124,14 +124,12 @@
List<FakeEmailSender.Message> messages = sender.getMessages();
assertThat(messages).hasSize(2);
- FakeEmailSender.Message newPatchsetMessage =
- Iterables.getOnlyElement(sender.getMessages(changeId, "newpatchset"));
+ FakeEmailSender.Message newPatchsetMessage = messages.get(0);
assertThat(newPatchsetMessage.body()).contains("new patch set");
assertThat(newPatchsetMessage.headers().get("Message-ID").toString())
.doesNotContain("EmailReviewComments");
- FakeEmailSender.Message newCommentsMessage =
- Iterables.getOnlyElement(sender.getMessages(changeId, "comment"));
+ FakeEmailSender.Message newCommentsMessage = messages.get(1);
assertThat(newCommentsMessage.body()).contains("has posted comments on this change");
assertThat(newCommentsMessage.headers().get("Message-ID").toString())
.contains("EmailReviewComments");
