Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more robust cache invalidation to CachingBucket #9575

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Oct 9, 2024

What this PR does

This change makes use of add operations for special "lock" cache entries to ensure that when items in object storage are mutated, stale results are not immediately stored to cache again.

It does this by seting "lock" cache entries with a short TTL when an item in object storage is about to be mutated. This prevents reads of the item from caching the results afterwards. After the item is mutated in object storage, its cache entries (excluding the lock entries) are deleted. After the lock entries expire, reads of the item are allowed to store results in the cache again.

Which issue(s) this PR fixes or relates to

Part of #9386

Follow up to #9387

Notes for reviewers

At a high level the way this works is:

  • CachingBucket.Upload starts a mutation operation on some item in object storage and special "lock" keys get set in Memcached with a short (15s) TTL.
  • CachingBucket.Upload completes the mutation operation on the object storage item.
  • CachingBucket.Upload deletes any cache entries associated with the item (contents, exists, attributes), but not the lock
  • Any reads of the item in object storage via CachingBucket.Get that attempt to cache the item that has just been modified will short-circuit for the next 15s because they cannot successfully Memcached.Add() the lock key.
  • After the "lock" key expires, reads of the item via CachingBucket.Get will be able to cache the item again.

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@56quarters 56quarters force-pushed the 56quarters/rule-cache-locking branch from a918423 to ef22f68 Compare October 9, 2024 20:57
This change makes use of `add` operations for special "lock" cache
entries to ensure that when items in object storage are mutated, stale
results are not immediately stored to cache again.

It does this by `set`ing "lock" cache entries with a short TTL when an
item in object storage is about to be mutated. This prevents reads of
the item from caching the results afterwards. After the item is mutated
in object storage, its cache entries (excluding the lock entries) are
deleted. After the lock entries expire, reads of the item are allowed
to store results in the cache again.

Part of #9386

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters force-pushed the 56quarters/rule-cache-locking branch from ef22f68 to d2cfbb9 Compare October 9, 2024 22:53
@56quarters
Copy link
Contributor Author

This PR addresses a potential race condition with cache invalidation discussed in #9434

It's possible an object is not in cache, fetched from object storage by request A, updated in object storage by request B, invalidated in cache (no-op) by request B, and stored to the cache by request A (stale)

@56quarters 56quarters marked this pull request as ready for review October 9, 2024 23:30
@56quarters 56quarters requested review from stevesg, grafanabot and a team as code owners October 9, 2024 23:30
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters requested a review from narqo October 10, 2024 15:45
Copy link
Contributor

@narqo narqo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 Works for me

@56quarters 56quarters requested a review from a team October 10, 2024 19:17
@56quarters 56quarters merged commit 2c82657 into main Oct 11, 2024
29 checks passed
@56quarters 56quarters deleted the 56quarters/rule-cache-locking branch October 11, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants