Move NotificationService calls to Sidekiq
What does this MR do?
There are quite a few places where we call methods on NotificationService
from Unicorn. These can be very expensive: https://gitlab.com/gitlab-org/gitlab-ce/issues/43106#note_64018144
This does not make them less expensive (that will come in another MR), but does make a related change: it moves the simplest calls to Sidekiq. We shouldn't be computing notification recipients in an HTTP request anyway, really.
Are there points in the code the reviewer needs to double check?
One thing I thought of was that we could try to be a little fancier, and automatically serialise a variable-length arguments list by serialising the class and ID of each argument. However, that's more complicated, so I figured that we could just create a new worker in the mail_scheduler
namespace for each method signature. I can do it the other way if we think it's better, though.
I also removed previous_record
from the NotificationService
as this won't work if called from Sidekiq (it isn't at present, but it would break in future).
Why was this MR needed?
It's a performance change by shuffling things about.
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Tests added for this feature/bug - Review
-
Has been reviewed by Backend
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Squashed related commits together
What are the relevant issue numbers?
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/43106.