Skip to content

Move MergeabilityCheckService processing to Sidekiq

The following discussion from gitlab-ee!14577 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, should MergeToRefService 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:

  1. Updating merge-refs of MRs impacted by a push/update by scheduling jobs at the MergeRequests::RefreshService
  2. 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:

  1. Direct callers such as CreatePipelineService would need instant feedback of the ref, so a sync call would probably be required for that scenario.
  2. 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

Edited by Oswaldo Ferreira