Skip to content

Move NotificationService calls to Sidekiq

Sean McGivern requested to merge move-notification-service-calls-to-sidekiq into master

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?

What are the relevant issue numbers?

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/43106.

Edited by Sean McGivern

Merge request reports