convert cache to a ttl cache #77
Conversation
christopherzli
requested review from
a team,
jnyi,
hczhu-db and
yuchen-db
and removed request for
a team
| } | ||
| f.lruCache.Add(queryExpressionNormalized, queryExpressionRangeLength) | ||
| } | ||
| f.lruCache.Add(queryExpressionNormalized, queryExpressionRangeLength) |
There was a problem hiding this comment.
Can you confirm Add() is thread-safe? Multiple goroutines from the HTTP server may call this function here concurrently.
| // TODO(hc.zhu): add a flag for the threshold | ||
| // The current gateway timeout is 5 minutes, so we cache the failed query running longer than 5 minutes - 10 seconds. | ||
| if queryResponseTime > time.Second * (60 * 5 - 10) { | ||
| if queryResponseTime > time.Second*(60*5-10) { |
There was a problem hiding this comment.
Can you make the magic number a flag since you are touching this part? :)
There was a problem hiding this comment.
I didnt, it was due to go format
There was a problem hiding this comment.
will do it in a separate PR
| func TestNewFailedQueryCache(t *testing.T) { | ||
| reg := prometheus.NewRegistry() | ||
| cache, err := NewFailedQueryCache(2, reg) | ||
| cache := NewFailedQueryCache(2, 0, reg) |
There was a problem hiding this comment.
Can you add test cases where the TTL is not 0?
There was a problem hiding this comment.
What does 0 TTL mean? Never expire?
| QueryStatsEnabled bool `yaml:"query_stats_enabled"` | ||
| LogFailedQueries bool `yaml:"log_failed_queries"` | ||
| FailedQueryCacheCapacity int `yaml:"failed_query_cache_capacity"` | ||
| FailedQueryTTL time.Duration `yaml:"failed_query_ttl"` |
There was a problem hiding this comment.
You need to add a cmd line flag for this config.
| } | ||
|
|
||
| func NewFailedQueryCache(capacity int, reg prometheus.Registerer) (*FailedQueryCache, error) { | ||
| func NewFailedQueryCache(capacity int, ttlDuration time.Duration, reg prometheus.Registerer) *FailedQueryCache { |
There was a problem hiding this comment.
I think the eventual goal of a TTL is to cache expensive queries as well. If a query fetches too many time series, add it to the cache.
Can you rename the cache like ExpensiveQueryCache and make it cache both failed queries (usually expensive ones), long-running ones (> 4 minutes), and the ones fetching too many time series?
yuchen-db
force-pushed
the
db_main
branch
8 times, most recently
from
64fbb3a to
5276dd1
Compare
Changes
Verification
pending test