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 a configuration for an index cache ttl #6234

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SungJin1212
Copy link
Contributor

Add a configuration for an index cache ttl. The index cache ttl can be configured by -blocks-storage.bucket-store.index-cache.redis.index-ttl and -blocks-storage.bucket-store.index-cache.memcached.index-ttl

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@SungJin1212 SungJin1212 force-pushed the Add-index-cache-ttl branch 2 times, most recently from b43ddd9 to 1a2b364 Compare September 25, 2024 07:44
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I think it probably needs more discussion on how we wanna do this. From my perspective, there are certain index cache items that worth a larger TTL than others.

For example, default TTL is 1 day but I might want to cache expanded postings for one week and other index cache types for only 1 day or shorter.

Cache entries from a 24h block might worth a longer TTL than 2h range uncompacted blocks because uncompacted blocks will be removed in a short period of time.

I am not against this PR. Just feel that the end state should be more flexible rather than a global TTL for everything.

@SungJin1212
Copy link
Contributor Author

SungJin1212 commented Sep 26, 2024

@yeya24
I agree with your thoughts. Different TTLs are necessary to consider block range. There is a necessary to set of different TTL based on block meta's MaxTime and MinTime. But, the cache interface is dependent on the thanos. I think if the TTL parameter is added to StorePostings, StoreExpandedPostings, and StoreSeries funcs, the implementation is possible.
c.f. https://github.com/thanos-io/thanos/blob/main/pkg/store/cache/cache.go#L43

@yeya24
Copy link
Contributor

yeya24 commented Sep 26, 2024

@SungJin1212 Agree. I think it would be useful to extend the interface to support TTL. The TTL could be a function that takes block metadata info as a parameter so that we know we use longer TTL for L4 blocks.

@SungJin1212
Copy link
Contributor Author

@yeya24
Yes. If then, more optimization can be done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants