Cache index queries for up to 15mins #947
Conversation
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
* Instrument the FifoCache * Move expiry checks into the cache itself Signed-off-by: Goutham Veeramachaneni <[email protected]>
Signed-off-by: Goutham Veeramachaneni <[email protected]>
Is that a recent change? How does that relate to https://github.com/weaveworks/cortex/blob/master/pkg/chunk/chunk_store.go#L208 ? |
|
We've got min-chunk-age set to 15mins in our cluster. The challenging with caching index entries is that that we need to ensure we don't miss a series, even when querying more that 15 mins in the past. So knowing that any entries in the cache are at max 15mins old, and knowing that the ingester is going to response with anything more recent than 15mins, allows us to make this optimisation. To be clear: this is a lower bound on the the cache freshness; we can probably cache most stuff for much longer (anything read from yesterdays rows can be cached for ever). A counter example might be useful. Imagine:
Hence, we much drop the cache entries after 15mins. Make sense? |
|
Does it become consistent if we add a line in the documentation stating that (and also one saying |
|
Separately, I would be very keen to cache index lookups from previous days' buckets. |
Yes we should add that line for sure.
Agreed! Should be relatively easy to do after this change. We've had it as a TODO in #209 for 18 months! |
Signed-off-by: Goutham Veeramachaneni <[email protected]>
gouthamve
force-pushed
the
cache-index
branch
2 times, most recently
from
94d55ba to
2eb86b1
Compare
pkg/chunk/storage/factory.go
Outdated
| @@ -28,26 +32,32 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | |||
| cfg.AWSStorageConfig.RegisterFlags(f) | |||
| cfg.GCPStorageConfig.RegisterFlags(f) | |||
| cfg.CassandraStorageConfig.RegisterFlags(f) | |||
|
|
|||
| f.IntVar(&cfg.IndexCacheSize, "store.index-cache-size", 0, "Size of in-memory index cache, 0 to disable.") | |||
| f.DurationVar(&cfg.IndexCacheValidity, "store.index-cache-validity", 15*time.Minute, "Period for which entries in the index cache are valid. Should be no higher than -ingester.max-chunk-idle.") | |||
There was a problem hiding this comment.
The default here needs to match the default for -ingester.max-chunk-idle, which is 5 minutes right now
Signed-off-by: Goutham Veeramachaneni <[email protected]>
|
This is running and has made a big difference for us; LGTM from me. |
pkg/chunk/storage/factory.go
Outdated
| default: | ||
| return nil, fmt.Errorf("Unrecognized storage client %v, choose one of: aws, gcp, cassandra, inmemory", cfg.StorageClient) | ||
| client, err = nil, fmt.Errorf("Unrecognized storage client %v, choose one of: aws, gcp, cassandra, inmemory", cfg.StorageClient) |
There was a problem hiding this comment.
Probably should still return here rather than make a caching client from a nil client
Signed-off-by: Goutham Veeramachaneni <[email protected]>
pkg/chunk/fifo_cache.go
Outdated
| @@ -1,20 +1,79 @@ | |||
| package chunk | |||
There was a problem hiding this comment.
Mostly a note/question, it doesn't look like FifoCache is related to the chunk. Could it be pulled out to a different package in the future?
| return &cachingStorageClient{ | ||
| StorageClient: client, | ||
| cache: chunk.NewFifoCache("index", size, validity), | ||
| validity: validity, |
There was a problem hiding this comment.
Is validity needed on the cachingStorageClient? Or only to create the cache?
There was a problem hiding this comment.
You're right. It was done outside the cache before, now I've pushed it into the cache. Forgot to remove this.
|
A couple final small questions/comments. Then this LGTM. |
Signed-off-by: Goutham Veeramachaneni <[email protected]>
|
@csmarchbanks fixed all comments, thanks! |
An in-process cache for index queries to accelerate queries.
Cache entries are valid for 15mins as thats how long the ingester will hold flushed chunks for.