Cache older index entries #1130
Conversation
Signed-off-by: Goutham Veeramachaneni <[email protected]>
gouthamve
force-pushed
the
cache-old-entries
branch
from
79022a1 to
d513fc9
Compare
|
This is good for review now. I'll need to fix the flag naming ( I'll put this in dev and report back. |
Signed-off-by: Goutham Veeramachaneni <[email protected]>
gouthamve
force-pushed
the
cache-old-entries
branch
from
d513fc9 to
150ca6f
Compare
pkg/chunk/series_store.go
Outdated
| @@ -60,6 +61,8 @@ type seriesStore struct { | |||
| cardinalityCache *cache.FifoCache | |||
|
|
|||
| writeDedupeCache cache.Cache | |||
|
|
|||
| cacheLookupsOlderThan time.Duration | |||
There was a problem hiding this comment.
Yup, will remove it.
pkg/chunk/schema.go
Outdated
| @@ -58,6 +58,9 @@ type IndexQuery struct { | |||
|
|
|||
| // Filters for querying | |||
| ValueEqual []byte | |||
|
|
|||
| // If the result of this lookup can be cached or not. | |||
| Cacheable bool | |||
There was a problem hiding this comment.
This might be cleaner if its a duration and not just a bool? Ie the schema tells (by virtue of returning queries) how long their results are cachable for?
There was a problem hiding this comment.
Yes, but the only answer to that is 0 and infinite because how long we can cache the active entries is determined by how long we keep chunks which the schema has no info about.
There was a problem hiding this comment.
Right, false doesn't mean don't cache, it means cache based on something else. Therefore this is misleading. Maybe this should be called Mutable, indicating it will change, and therefore shouldn't be cached for too long?
| cfg.memcacheClient.RegisterFlagsWithPrefix("index.", "Deprecated: Use -store.index-cache-read.*;", f) | ||
|
|
||
| cfg.indexQueriesCacheConfig.RegisterFlagsWithPrefix("store.index-cache-read.", "Cache config for index entry reading. ", f) | ||
| f.DurationVar(&cfg.IndexCacheValidity, "store.index-cache-validity", 5*time.Minute, "Cache validity for active index entries. Should be no higher than -ingester.max-chunk-idle.") |
There was a problem hiding this comment.
Looks like this is no longer used?
There was a problem hiding this comment.
This is how long we want to cache the active entries.
There was a problem hiding this comment.
I can't see where it is used either
There was a problem hiding this comment.
It is being used here: https://github.com/cortexproject/cortex/pull/1130/files#diff-d479a87a51735dca31797a0bc4af42caL95 to set the valid duration for caching mutable entries.
|
I was wondering if, instead of adding a caching schema wrapper, we might extend the |
|
Yes, but if a bucket's entries can be cached not based on if it's the most recent but rather how stale our writes can be. Having said that, I think it still can result in a cleaner abstraction. Will check. |
Agreed, in other places (table manager) there is a grace period to deal with this. |
pkg/chunk/schema_caching.go
Outdated
| "github.com/prometheus/common/model" | ||
| ) | ||
|
|
||
| type cachingSchema struct { |
There was a problem hiding this comment.
type is cachingSchema but filename is schema_caching ?
| return mergeCacheableAndActiveQueries(cacheableQueries, activeQueries), nil | ||
| } | ||
|
|
||
| func splitTimesByCacheability(from, through model.Time, cacheBefore model.Time) (model.Time, model.Time, model.Time, model.Time) { |
There was a problem hiding this comment.
Is this really valuable in the presence of the caching front-end which will shard by day?
There was a problem hiding this comment.
Not really, if using the frontend, but the frontend is an optional component, I guess?
There was a problem hiding this comment.
Plus I'd say yes it is - the caching frontend only matches exact querier, this will match individual labels, which is useful across multiple different queries.
| cfg.memcacheClient.RegisterFlagsWithPrefix("index.", "Deprecated: Use -store.index-cache-read.*;", f) | ||
|
|
||
| cfg.indexQueriesCacheConfig.RegisterFlagsWithPrefix("store.index-cache-read.", "Cache config for index entry reading. ", f) | ||
| f.DurationVar(&cfg.IndexCacheValidity, "store.index-cache-validity", 5*time.Minute, "Cache validity for active index entries. Should be no higher than -ingester.max-chunk-idle.") |
There was a problem hiding this comment.
I can't see where it is used either
Signed-off-by: Goutham Veeramachaneni <[email protected]>
|
@gouthamve needs rebasing |
Signed-off-by: Goutham Veeramachaneni <[email protected]>
|
LGTM! |
Fixes #964
Tests pending