Use GitLab version number (or similar) for merge request diffs caching

In https://gitlab.com/gitlab-org/gitlab-ce/issues/48801 and https://gitlab.com/gitlab-org/gitlab-ce/issues/50793 we saw bugs due to MR diff caching. Both of these bugs were because the data stored in the cache changed, but the key didn't.

This comes from:

  • Gitlab::Diff::FileCollection::MergeRequestDiff is a wrapper class that allows caching highlighted MR diffs in Redis, to avoid having to fetch blobs each time.
  • It uses Gitlab::Diff::Line::SERIALIZE_KEYS as part of its cache key, along with info about the MR diff itself: https://gitlab.com/gitlab-org/gitlab-ce/blob/v11.2.3/lib/gitlab/diff/file_collection/merge_request_diff.rb#L37
  • Gitlab::Diff::Line::SERIALIZE_KEYS may not necessarily change, even if the semantics of the cached values change (https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/2463).

In the CacheableAttributes concern, we use this as the cache key: "#{name}:#{Gitlab::VERSION}:#{Rails.version}"

I propose that we use the GitLab version in the cache key for MR diffs, instead of Gitlab::Diff::Line::SERIALIZE_KEYS.

In favour:

  1. Much simpler to reason about. Every time the GitLab version changes, the cache key changes.
  2. Similarly, it's much harder to get wrong. It's not possible to miss this in an update, and right now the situation is that it is far too easy to miss this.
  3. It's unreasonably hard to write a test that verifies the current behaviour across upgrades.

In opposition:

  1. This has only changed twice. Both were recently, and both were bad issues that were hard to solve, but it's still only two data points.
  2. We cache this for a week right now. We might bump GitLab versions on GitLab.com more often than that, keeping stale data around in the cache.
    • To mitigate, we could reduce the TTL further.
  3. As a result of 2, we'd probably see more frequent cache misses, leading to an increase in response times.

I have only thought of this since yesterday, so there might be other points I'm missing. But writing this now, I think the points in favour are much stronger than the points against.

Edited Aug 30, 2018 by Sean McGivern
Assignee Loading
Time tracking Loading