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) |
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.
| @@ -159,7 +156,7 @@ func (f *FailedQueryCache) UpdateFailedQueryCache(err error, query url.Values, q | |||
| queryExpressionRangeLength := getQueryRangeSeconds(query) | |||
| // 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
| @@ -34,19 +34,16 @@ func verifyMetricCount(t *testing.T, reg *prometheus.Registry, expectedCount int | |||
|
|
|||
| 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?
| @@ -48,6 +48,7 @@ type HandlerConfig struct { | |||
| 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.
| cachedHits prometheus.Counter | ||
| cachedQueries prometheus.Gauge | ||
| } | ||
|
|
||
| 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?
Changes
Verification
pending test