Resolve "Reduce Redis usage from merge request diffs caching"
What does this MR do?
- First commit: change to only caching the highlight results of the latest MR diff, and explicitly delete older highlight results.
- Second commit: reduce the TTL to a week, instead of the default two weeks.
Between them, these reduce the amount of memory we'll use on these highlighted diff results in Redis.
As an example, I had these cached MR diffs:
redis /Users/seanmcgivern/Code/gitlab-development-kit/redis/redis.socket> keys cache:gitlab:merge_request_diffs/*
1) "cache:gitlab:merge_request_diffs/12-20180201121855831702000/highlighted-diff-files/expanded=false/ignore_whitespace_change=false/max_files=1000/max_lines=50000"
2) "cache:gitlab:merge_request_diffs/47-20180313140710462936000/highlighted-diff-files/expanded=false/ignore_whitespace_change=false/max_files=1000/max_lines=50000"
3) "cache:gitlab:merge_request_diffs/39-20180201121912645797000/highlighted-diff-files/expanded=false/ignore_whitespace_change=false/max_files=1000/max_lines=50000"
4) "cache:gitlab:merge_request_diffs/48-20180313140745621010000/highlighted-diff-files/expanded=false/ignore_whitespace_change=false/max_files=1000/max_lines=50000"
5) "cache:gitlab:merge_request_diffs/10-20180201121854548624000/highlighted-diff-files/expanded=false/ignore_whitespace_change=false/max_files=1000/max_lines=50000"
47 and 48 belonged to the same MR, and the others to a different MR each. When I pushed to the MR for 47 and 48, then re-ran the command, I got:
redis /Users/seanmcgivern/Code/gitlab-development-kit/redis/redis.socket> keys cache:gitlab:merge_request_diffs/*
1) "cache:gitlab:merge_request_diffs/12-20180201121855831702000/highlighted-diff-files/expanded=false/ignore_whitespace_change=false/max_files=1000/max_lines=50000"
2) "cache:gitlab:merge_request_diffs/39-20180201121912645797000/highlighted-diff-files/expanded=false/ignore_whitespace_change=false/max_files=1000/max_lines=50000"
3) "cache:gitlab:merge_request_diffs/49-20180314160247882384000/highlighted-diff-files/expanded=false/ignore_whitespace_change=false/max_files=1000/max_lines=50000"
4) "cache:gitlab:merge_request_diffs/10-20180201121854548624000/highlighted-diff-files/expanded=false/ignore_whitespace_change=false/max_files=1000/max_lines=50000"
Are there points in the code the reviewer needs to double check?
Don't think so.
Why was this MR needed?
https://gitlab.com/gitlab-com/infrastructure/issues/3840 was an outage. This may have contributed.
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Tests added for this feature/bug - Review
-
Has been reviewed by Backend
-
What are the relevant issue numbers?
Closes #44191 (closed).
Merge request reports
Activity
marked the checklist item Changelog entry added, if necessary as completed
@mdelaossa do you want to review this? The
MergeRequests::MergeRequestDiffCacheService
is called byMergeRequest#reload_diff
, which is called by theMergeRequests::RefreshService
- it's quite convolutedassigned to @mdelaossa
- Resolved by Stan Hu
mentioned in issue #44191 (closed)
Thanks @mdelaossa - please reassign when you're done reviewing
In this case, I would have assigned back to you after my changes, but you were still assigned!
@smcgivern sorry! Still not fully used to the flow
I'll reassign once I'm doneassigned to @smcgivern
Thanks! @stanhu would you like to do the final review?
assigned to @stanhu
- Resolved by Stan Hu
@smcgivern Looks great, thanks for the quick turnaround on this!
mentioned in commit 49d5c097
@northrup great point!
Please can you create a new issue for that? The same reasoning as https://gitlab.com/gitlab-com/infrastructure/issues/3840#note_62922186 applies here
I feel like the answer is 'sometimes' (for instance, 'what caused that bug?'), but not 'frequently enough that we want to keep it'. However, I have zero data to back that up.
Once this change is deployed, we could try sampling the cache keys at a point in time, and then query the DB to see how many of those are from merged MRs.
mentioned in epic gitlab-com/gl-infra&80
added devopsplan label