Skip to content
Snippets Groups Projects

Resolve "Reduce Redis usage from merge request diffs caching"

All threads resolved!

What does this MR do?

  1. First commit: change to only caching the highlight results of the latest MR diff, and explicitly delete older highlight results.
  2. 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).

Edited by Sean McGivern

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Mario de la Ossa resolved all discussions

    resolved all discussions

  • This looks good to me :thumbsup:

  • Mario de la Ossa approved this merge request

    approved this merge request

  • Stan Hu mentioned in issue #44191 (closed)

    mentioned in issue #44191 (closed)

  • Sean McGivern added 2 commits

    added 2 commits

    • aa7400d5 - Only cache highlight results for latest MR diffs
    • e37582c3 - Only cache MR diffs for one week

    Compare with previous version

  • Author Contributor

    Thanks @mdelaossa - please reassign when you're done reviewing :slight_smile:

    In this case, I would have assigned back to you after my changes, but you were still assigned!

  • Sean McGivern added 2 commits

    added 2 commits

    • 6cd7f679 - Only cache highlight results for latest MR diffs
    • db908826 - Only cache MR diffs for one week

    Compare with previous version

  • @smcgivern sorry! Still not fully used to the flow :sweat_smile: I'll reassign once I'm done

  • Mario de la Ossa approved this merge request

    approved this merge request

  • Author Contributor

    Thanks! @stanhu would you like to do the final review?

  • assigned to @stanhu

  • Stan Hu
  • Stan Hu approved this merge request

    approved this merge request

  • @smcgivern Looks great, thanks for the quick turnaround on this!

  • Stan Hu resolved all discussions

    resolved all discussions

  • Stan Hu mentioned in commit 49d5c097

    mentioned in commit 49d5c097

  • merged

  • I have a question about further optimization, How frequently after a merge has been approved and completed to people view the diff history? Have we looked at ejecting the diff cache of the merge after the merge is complete?

  • Author Contributor

    @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 :slight_smile:

    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.

  • Please register or sign in to reply
    Loading