Move MergeabilityCheckService processing to Sidekiq
The following discussion from !14577 (merged) should be addressed:
-
@stanhu
started a discussion: (+4 comments)@oswaldo
This shouldn't block this merge request because I see the 500 errors need to be fixed, but I have a few questions: 2. Also, shouldMergeToRefService
be run synchronously? I'd imagine for large merges, this might tie up a Unicorn worker for a while.
It was discussed at this MR the pros and cons of having this service being called sync (Unicorn), which happens today whenever the MR is marked as unchecked - meaning the merge-ref is outdated along the merge_status
. So when accessing the MR page, the merge-ref is updated in that scenario.
We've measured its performance with big MRs:
Since it'll be executed whenever the source/target branches get updated and the user accesses the MR page, I've put this into a reasonably heavy test in a MR with 114 file changes, 2882 additions and 12802 deletions. The repo target branch has 20k commits and 142MB in file size.
It's taking around 1 second to recalculate the merge-ref for it, which shows it's not a cheap operation. Important to bring up that we won't need to make this operation again if the source/target branches doesn't get updated.
But considering the lower performance with NFS we might see worse scenarios at self-hosted instances. Which means we should consider updating the merge-ref async at some point.
We got a few options under the radar:
- Updating merge-refs of MRs impacted by a push/update by scheduling jobs at the
MergeRequests::RefreshService
-
Scheduling just the merge-ref update at
MergeabilityCheckService
upon MR page access
The 1st would mean a big burst of workload for Gitaly at every push, which doesn't sound ideal. The 2nd sounds more reasonable, though that would mean:
- Direct callers such as
CreatePipelineService
would need instant feedback of the ref, so a sync call would probably be required for that scenario. - In order to use the ref to present diffs (&854 (closed)), we'll need to provide a "ongoing" state if this job wasn't finished yet.
cc @DouweM