Skip to content

Introduce caching on Projects::MergeRequestsController#discussions

Problem to solve

During investigation in #330893 (closed), noticed that Projects::MergeRequestController#dicsussions can take a while for a large MR (example: https://staging.gitlab.com/gpt/large_projects/gitlabhq1/-/merge_requests/8785).

Locally, it can take ~9s to load the discussions. Profiling the endpoint shows that it takes most of the time serializing the response.

Proposal

While working on a PoC on how we can cache this endpoint (!63644 (closed)), noticed that it can go down up to ~1.5s. That's ~80% improvement.

In the PoC, we cache the serialized response via Rails.cache. It's utilizing the Api::Helpers::Caching module with some modification to work with Rails controllers instead of Grape API (see #render_cached). This will cache each discussion on a MR.

To implement this, we can:

  1. Implement #render_cached or name it something else. Not exactly how it's written in the PoC MR as it can be improved but it should behave the same way.
  2. Define appropriate cache key for a discussion. In the PoC, used the discussion ID and the latest updated at of notes under a discussion.
  3. Define a cache expiration. 1 day seems fine as it is possible to have a discussion that gets untouched in a day.
  4. Implement it behind a feature flag then roll it out in a separate roll out issue. This way we can enable it on production and monitor how it affects our Redis cache usage.

Be on the look out for possible bug due to cache response is still being returned while it shouldn't.

Edited by Patrick Bajao