Skip to content
Snippets Groups Projects

Broadcast message stale cache on different Gitlab revisions

All threads resolved!

What does this MR do and why?

Previously, redis expiration wasn't working in some cases where canary and production were on different revisions - see #412199 (closed)

  • Fixes the cache expiration issue with Broadcast messages that can occur from canary vs production Gitlab.revision differences.
  • Extends the JsonCache class to support nested key structure to allow for easy key expiration logic.

The way the fix here works is to make one cache entry with many revisions. Before there was a cache entry per Gitlab.revision. So we take advantage of the json structure here and simply move the Gitlab.revision from being part of the key for the cache to an entry in the cache as seen below.

Previous structure used for broadcast messages

cache key: cache:gitlab:broadcast_message_current_json:d9cef69a710

  • namespace: cache:gitlab defined here
  • key: broadcast_message_current_json or variants, defined here
  • git revision: d9cef69a710 changes for code changes(causing current expire issue) defined here
[{"id"=>34,
  "message"=>"MyText",
  "starts_at"=>"2023-05-29T23:52:54.006Z",
  "ends_at"=>"2023-05-31T23:52:54.006Z",
  "created_at"=>"2023-05-30T23:52:54.006Z",
  "updated_at"=>"2023-05-30T23:52:54.006Z",
  "color"=>"#E75E40",
  "font"=>"#FFFFFF",
  "message_html"=>"<p>MyText</p>",
  "cached_markdown_version"=>2097152,
  "target_path"=>nil,
  "broadcast_type"=>"banner",
  "dismissable"=>nil,
  "target_access_levels"=>[],
  "theme"=>"indigo"}]

New structure used for broadcast messages

cache key: cache:gitlab:broadcast_message_current_json

  • namespace: cache:gitlab defined here
  • key: broadcast_message_current_json or variants, defined here
  • git revision: d9cef69a710 now moved into becoming part of the cached result...this makes the value of the keys larger, but shouldn't take more space up in redis overall as we only have one key with many revisions now instead of a key per revision.
{"d9cef69a710"=>
  [{"id"=>58,
    "message"=>"MyText",
    "starts_at"=>"2023-05-30T02:58:18.949Z",
    "ends_at"=>"2023-06-01T02:58:18.949Z",
    "created_at"=>"2023-05-31T02:58:18.950Z",
    "updated_at"=>"2023-05-31T02:58:18.950Z",
    "color"=>"#E75E40",
    "font"=>"#FFFFFF",
    "message_html"=>"<p>MyText</p>",
    "cached_markdown_version"=>2097152,
    "target_path"=>nil,
    "broadcast_type"=>"banner",
    "dismissable"=>nil,
    "target_access_levels"=>[],
    "theme"=>"indigo"}],
 "abc123"=>
  [{"id"=>58,
    "message"=>"MyText",
    "starts_at"=>"2023-05-30T02:58:18.949Z",
    "ends_at"=>"2023-06-01T02:58:18.949Z",
    "created_at"=>"2023-05-31T02:58:18.950Z",
    "updated_at"=>"2023-05-31T02:58:18.950Z",
    "color"=>"#E75E40",
    "font"=>"#FFFFFF",
    "message_html"=>"<p>MyText</p>",
    "cached_markdown_version"=>2097152,
    "target_path"=>nil,
    "broadcast_type"=>"banner",
    "dismissable"=>nil,
    "target_access_levels"=>[],
    "theme"=>"indigo"}]}

How to set up and validate locally

Follow 'Verify by UI' steps here

:notebook: to verify this(or master if verifying the before) branch you can use this branch(or master if verifying the before) as a base and then create a new branch locally with a minor diff somewhere and create a new local commit from that.

example of what those commands might look like
10088  git checkout -b test-broadcast
10089  git stash pop
10090  git add -A
10091  git commit -m 'Test broadcast cache'
10092  git rev-parse HEAD
10093  git checkout -
10094  git rev-parse HEAD

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #412199 (closed)

Edited by Doug Stull

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
  • Doug Stull
  • Doug Stull marked this merge request as ready

    marked this merge request as ready

  • Doug Stull requested review from @jmontal

    requested review from @jmontal

  • Doug Stull added workflowin review label and removed workflowin dev label

    added workflowin review label and removed workflowin dev label

  • Jay Montal changed the description

    changed the description

  • Jay Montal approved this merge request

    approved this merge request

  • Jay Montal requested review from @ayufan and removed review request for @jmontal

    requested review from @ayufan and removed review request for @jmontal

  • :wave: @jmontal, thanks for approving this merge request.

    This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

    For more info, please refer to the following links:

  • Roy Liu mentioned in issue #412200 (closed)

    mentioned in issue #412200 (closed)

  • Kamil Trzciński removed review request for @ayufan

    removed review request for @ayufan

  • Doug Stull added 1 commit

    added 1 commit

    • c661b791 - Refine as true base / inherited class concept

    Compare with previous version

  • Doug Stull added 1 commit

    added 1 commit

    • fd6f1e64 - Refine as true base / inherited class concept

    Compare with previous version

  • A deleted user added documentation label

    added documentation label

  • Doug Stull added 1 commit

    added 1 commit

    • f142594c - Refine as true base / inherited class concept

    Compare with previous version

  • Doug Stull added 1 commit

    added 1 commit

    • 83d32335 - Refine as true base / inherited class concept

    Compare with previous version

  • Doug Stull
  • Doug Stull requested review from @ayufan

    requested review from @ayufan

  • Doug Stull added 1170 commits

    added 1170 commits

    Compare with previous version

  • Doug Stull added 1 commit

    added 1 commit

    • 22345bbe - Refine as true base / inherited class concept

    Compare with previous version

  • Kamil Trzciński approved this merge request

    approved this merge request

  • Doug Stull requested review from @nicolasdular

    requested review from @nicolasdular

  • Doug Stull added 1 commit

    added 1 commit

    Compare with previous version

  • Kamil Trzciński approved this merge request

    approved this merge request

  • Kamil Trzciński removed review request for @ayufan

    removed review request for @ayufan

  • Nicolas Dular
  • Nicolas Dular approved this merge request

    approved this merge request

  • Nicolas Dular resolved all threads

    resolved all threads

  • merged

  • @nicolasdular, did you forget to run a pipeline before you merged this work? Based on our code review process, if the latest pipeline was created more than 6 hours ago OR finished more than 2 hours ago, you should:

    1. Ensure the merge request is not in Draft status.
    2. Start a pipeline (especially important for Community contribution merge requests).
    3. Set the merge request to merge when pipeline succeeds.

    This is a guideline, not a rule. Please consider replying to this comment for transparency.

    This message was generated automatically. You're welcome to improve it.

  • Nicolas Dular mentioned in commit 0bd8a2f6

    mentioned in commit 0bd8a2f6

  • Doug Stull marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

    marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

  • mentioned in issue #412199 (closed)

  • added workflowstaging label and removed workflowcanary label

  • Please register or sign in to reply
    Loading