Implement result cache for tenant query federation #3640
Conversation
simonswine
force-pushed
the
20201231_implement_result_cache_for_tenant_query_federation
branch
from
fe80a5a
to
5a2f935
Compare
|
@sandeepsukhani Would you mind taking a look at this PR please? |
There was a problem hiding this comment.
Changes look good to me, but I would prefer if @sandeepsukhani took a look as well, as he is an expert on this stuff.
|
|
||
| // set results number string if it's higher than the ones before | ||
| if numbers.results != "" { | ||
| results, err := strconv.Atoi(numbers.results) |
There was a problem hiding this comment.
I'm confused by this... is this value guaranteed to be an incrementing number?
update: comment above says cacheGenNumbers holds store and results cache gen numbers for a user.
There was a problem hiding this comment.
Yes, the gen numbers are actually timestamps, we simply set them to current timestamp whenever we need to change it.
Signed-off-by: Christian Simon <simon@swine.de>
simonswine
force-pushed
the
20201231_implement_result_cache_for_tenant_query_federation
branch
from
5a2f935
to
29ac099
Compare
| if numbers.results != "" { | ||
| results, err := strconv.Atoi(numbers.results) | ||
| if err != nil { | ||
| level.Error(util.Logger).Log("msg", "error parsing resultsCacheGenNumber", "tenant", tenantID, "err", err) |
There was a problem hiding this comment.
Across the code base, the tenant ID is logged with the field name user instead of tenant. Do you mind fixing this for consistency, please?
Same comment applies below.
There was a problem hiding this comment.
Good point see: #3680
What this PR does:
Tenant query federation PR #3250 did skip result caching for tenant query federation. This PR will implement it.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]