Shared in-memory index cache for queriers with blocks storage #2189
Conversation
|
About this TODO: This issue is fixed in Thanos |
pracucci
force-pushed
the
single-querier-in-memory-cache-for-blocks-storage
branch
2 times, most recently
from
ecc6f61 to
7167a70
Compare
pracucci
force-pushed
the
single-querier-in-memory-cache-for-blocks-storage
branch
2 times, most recently
from
0f40094 to
9ffb9d9
Compare
There was a problem hiding this comment.
This is a nice PR, only one minor nit.
Though for me there is a bigger concern that we are not prefixing anything with the tenantID. I know its extremely rare for ULIDs to match across tenants, I'm afraid that if it did, we might be leaking some data.
Can we add a TODO to make sure that we propagate the tenantID into the cache somehow?
pkg/storage/tsdb/config.go
Outdated
| @@ -154,7 +154,7 @@ type BucketStoreConfig struct { | |||
| func (cfg *BucketStoreConfig) RegisterFlags(f *flag.FlagSet) { | |||
| f.StringVar(&cfg.SyncDir, "experimental.tsdb.bucket-store.sync-dir", "tsdb-sync", "Directory to store synchronized TSDB index headers.") | |||
| f.DurationVar(&cfg.SyncInterval, "experimental.tsdb.bucket-store.sync-interval", 5*time.Minute, "How frequently scan the bucket to look for changes (new blocks shipped by ingesters and blocks removed by retention or compaction). 0 disables it.") | |||
| f.Uint64Var(&cfg.IndexCacheSizeBytes, "experimental.tsdb.bucket-store.index-cache-size-bytes", uint64(250*units.Mebibyte), "Size - in bytes - of a per-tenant in-memory index cache used to speed up blocks index lookups.") | |||
| f.Uint64Var(&cfg.IndexCacheSizeBytes, "experimental.tsdb.bucket-store.index-cache-size-bytes", uint64(1*units.Gibibyte), "Size in bytes of in-memory index cache used to speed up blocks index lookups (shared across multiple tenants).") | |||
There was a problem hiding this comment.
shared across all tenants is better, but only slightly :)
There was a problem hiding this comment.
I've updated it to "shared between all tenants". I hope to have correctly merged both feedback 😉
There was a problem hiding this comment.
Merges can be difficult :)
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]> Co-Authored-By: Peter Štibraný <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
pracucci
force-pushed
the
single-querier-in-memory-cache-for-blocks-storage
branch
from
9ffb9d9 to
8f19883
Compare
Signed-off-by: Marco Pracucci <[email protected]>
This is a very good point. After some discussion in #2069 we decided to not propagate the tenantID but make the block ID entropy safer, which is what I did here: Feel free share further feedback. I'm willing to re-consider this decision if you think it's risky. |
pracucci
deleted the
single-querier-in-memory-cache-for-blocks-storage
branch
What this PR does:
In this PR I propose to shift to a shared in-memory index cache for queriers with blocks storage. The problem with per-tenant caches is that the total max cache size linearly increase with the number of tenants, while with a single cache it's easier to keep the max memory used under control.
Which issue(s) this PR fixes:
Fixes #2069
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]