-
Notifications
You must be signed in to change notification settings - Fork 462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Streaming store-gateway: make room for tenant ID in postings cache key #3839
Conversation
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
i also did a comparison of the size of entries in memcached before and after this change on on this PR: these are the common benchmarks between the two. It looks like postings aren't adding almost anything. Could be because of the way they are generated in tests. Not sure if I should investigate
|
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, LGTM! I left few minor comments. I agree with your reasoning in the PR description.
i ran a quick test with this in our dev cluster. The test scenario was running many requests selecting ~50K series each without chunks. zone-a was running r218 with streaming and without mmap, zone-b was running with streaming and without mmap and with the changes in this PR, zone-c was running the default implementations i didn't observe any negative impact on CPU or memory utilization, or latency |
@dimitarvdimitrov Thanks! Looks great. Can you confirm the store-gateway-load-test results comparison hasn't reported any issue, please? |
Let's gooooo! 😂 |
Background + Why?
The main purpose of this PR is to make enough room in the memcached key for the tenant ID. The key in question is for storing series for a set of postings. This is only used in the streaming store-gateway implementation
Mimir supports tenant ID of up to 150 characters (I assume ASCII).
Memcached keys are limited to 250 bytes. With the current implementation we cannot fit all items in the cache key. This isn't a problem in reality since the tenant ID doesn't go anywhere near 150 bytes.
Here is how the cache key length allocation looks like on
main
SP2
: identifier of the cache key version:
: colon separators1_of_3
: sharding selectorSince most of these are pretty much fixed in length, the extra 32 bytes will be taken from the tenant ID.
Changes
This PR does a few notable changes
{a="1", b="2"}
and{c="3"}
can both select the same series)SP2
: identifier of the cache key version:
: colon separators1_of_3
: sharding selectorblake2b.Sum
we need to doblake2b.New
. The latter does extra allocations for the digest, whereas the former allocated only a slice for the hash. So I decided not to do it; we can reconsider if collisions are commonBenchmarks
There are some marginal savings due to now not having to print and hash the matchers when storing the series. The 13% slowdown in
Bucket_Series_WithSkipChunks/1SeriesWith10000000Samples/with_default_options/10000000of10000000
is a fluke because it doesn't use any of the code changed in this PRThe baseline was 748c19e
click