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

Set series cache TTL different based on block metadata #7407

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

56quarters
Copy link
Contributor

@56quarters 56quarters commented Feb 16, 2024

What this PR does

Instead of using a default TTL of 7d for all index-cache related entries, pick different TTLs based on metadata about the block. This allows entries related to blocks that will be deleted (OOO blocks, 2h, 12h) to expire and be eligible to be evicted sooner than they would otherwise.

This change only sets different TTLs for series entries in the cache since they account for most of the space in the cache.

Which issue(s) this PR fixes or relates to

N/A

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/diff-block-ttl branch 4 times, most recently from 750e359 to cc4c0e4 Compare February 16, 2024 19:01
Instead of using a default TTL of 7d for all index-cache related entries,
pick different TTLs based on metadata about the block. This allows entries
related to blocks that will be deleted (OOO blocks, 2h, 12h) to expire and
be eligible to be evicted sooner than they would otherwise.

This change only sets different TTLs for series entries in the cache since
they account for most of the space in the cache.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters marked this pull request as ready for review February 16, 2024 21:33
@56quarters 56quarters requested a review from a team as a code owner February 16, 2024 21:33
Copy link
Member

@jhalterman jhalterman left a comment

Choose a reason for hiding this comment

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

Left a nit comment, but LGTM

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Not approving just because I only reviewed BlockTTL(). I defer to @jhalterman proper review for the rest.

pkg/storegateway/indexcache/cache.go Outdated Show resolved Hide resolved
pkg/storegateway/indexcache/cache.go Show resolved Hide resolved
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM! (modulo a last comment)

pkg/storegateway/indexcache/cache.go Outdated Show resolved Hide resolved
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
@56quarters 56quarters merged commit 1dbb552 into main Feb 22, 2024
28 checks passed
@56quarters 56quarters deleted the 56quarters/diff-block-ttl branch February 22, 2024 18:34
56quarters added a commit that referenced this pull request Mar 4, 2024
Like #7407, use different TTL for postings based on block metadata.
Use shorter TTLs for cache entries associated with temporary blocks
that will be compacted and deleted shortly.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
56quarters added a commit that referenced this pull request Mar 5, 2024
Like #7407, use different TTL for postings based on block metadata.
Use shorter TTLs for cache entries associated with temporary blocks
that will be compacted and deleted shortly.

Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
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.

None yet

3 participants