Moving code for cache functions to utils file #48
Conversation
|
Can you paste the command to run the new unit test? |
=== RUN TestNewFailedQueryCache |
|
New testing output === RUN TestNewFailedQueryCache |
|
|
||
| // QueryHitCache checks if the lru cache is hit and returns whether to increment counter for cache hits along with appropriate message. | ||
| func queryHitCache(queryExpressionNormalized string, queryExpressionRangeLength int, lruCache *lru.Cache) (bool, string) { | ||
| if value, ok := lruCache.Get(queryExpressionNormalized); ok && value.(int) <= queryExpressionRangeLength { |
There was a problem hiding this comment.
There is a bug here. The condition should be value.(int) >= queryExpressionRangeLength such that the cached range is longer than the current query range.
@pranavmishradatabricks Can you fix it and add a unit test case like the following one?
1. Cache failed query("xxx", range=1000)
2. query("xxx", range=999) shouldn't be banned.
There was a problem hiding this comment.
The condition should be value.(int) <= queryExpressionRangeLength since queryExpressionRangeLength is the range length of the new query and value.(int) is the range length of the old query. From my understanding, it should only hit the cache and block the query if the old query's range length <= the new query's range length (new query is longer range length).
I have a test case showing that it does not hit the cache here: Test File Line 153
|
|
||
| // QueryHitCache checks if the lru cache is hit and returns whether to increment counter for cache hits along with appropriate message. | ||
| func queryHitCache(queryExpressionNormalized string, queryExpressionRangeLength int, lruCache *lru.Cache) (bool, string) { | ||
| if value, ok := lruCache.Get(queryExpressionNormalized); ok && value.(int) <= queryExpressionRangeLength { |
Changes
Moved code pertaining to the LRU Cache in the handler to a separate file for code clarity and to prevent conflicts upstream. This is a fast follow on https://github.com/databricks/thanos/pull/40
Verification
I used the same testing process as in the original pull request.