store-gateway: add support for partial expanded postings #4667
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
dimitarvdimitrov
changed the title
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
|
I think it will make sense to also add a test case to make sure that the postings selection strategy is respected and the posting groups aren't mangled. |
colega
reviewed
Apr 5, 2023
colega
reviewed
Apr 5, 2023
colega
reviewed
Apr 5, 2023
colega
reviewed
Apr 5, 2023
colega
reviewed
Apr 5, 2023
colega
reviewed
Apr 5, 2023
colega
reviewed
Apr 5, 2023
colega
reviewed
Apr 5, 2023
colega
reviewed
Apr 5, 2023
|
I would consider caching the final filtered list, and trying to retrieve that one from the expanded postings promise/cache. Is there any reason why caching the partial list + matchers would be more useful/efficient? Otherwise, if this approach is suboptimal for some reason, we'll repeat the suboptimal work multiple times. Edit: if we do that, we don't even have to change the expanded postings cache key: the expanded postings don't depend on the strategy how we got them. |
|
I ran the benchmarks Benchmarks.txt The most notable point for me is that on all benchmarks, which involve the |
|
we discussed Oleg's comment offline and this is the TL;DR The proposed implementation in this PR is less efficient and shouldn't have any theoretical benefits over caching the already filtered postings. However, caching the filtered list is non-trivial - it involves
We agreed that it's ok to have the first iteration like this and then think about improving it. I hope that the added cost of filtering is dominated by the saved cost of processing large (and uncacheable) postings lists. I've added this item to the parent issue #4593 |
pracucci
approved these changes
Apr 5, 2023
| promise, loaded = r.expandedPostingsPromise(ctx, ms, stats) | ||
| returnRefs, cached, returnErr = promise(ctx) | ||
| returnRefs, unappliedMatchers, cached, returnErr = promise(ctx) |
There was a problem hiding this comment.
Naming: have you considered "lazyMatchers" given they get applied later? I don't feel strong about it, cause there's a risk of confusing them with the lazy posting groups.
There was a problem hiding this comment.
I'm also not sure about mixing them with the lazy posting groups. Especially because lazy means different things in the two usages (used later vs resolved later).
What do you think about deferredMatchers, postponedMatchers, delayedMatchers?
There was a problem hiding this comment.
I do feel pretty strong against deferredMatchers. The description of this PR talks about unapplied matchers, and IMO that doesn't require extra knowledge of the topic to understand. I could also buy postponedMatchers, or as I suggested in a separate comment, pendingMatchers. But I think deferred introduces a new term that increases congnitive load here.
There was a problem hiding this comment.
I think pendingMatchers sounds good too. I'll wait for Marco to take a look too before renaming again.
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>
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>
10 tasks
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
|
i looked into the increased memory and CPU of the benchmarks with So what these test cases do is select a very small number of all label values for the So in these benchmarks the memory was dominated by the wrong estimation of the number of values that will be selected. Now we increased the memory footprint of the item of each label value item (it was only So the benchmarks showed almost a 100% increase. I'll see if we can be better in doing these estimations
This wasn't caused by this PR, but this PR makes it worse. |
|
so i tried running the filter twice, and counting the matches before running it again to collect the matches. This showed some good improvement in some benchmarks but also regression on others (ebfe653) BucketIndexReader_ExpandedPostings with two passesthen i tried using the prefix to estimate where the last match will be (387b07f). This showed better results overall but still some regressions (e.g. when the prefix is also the value, since we now binary search twice; maybe we should do linear search?). BucketIndexReader_ExpandedPostings with end prefix estimation too
I think we should continue this discussion and work in another PR. But I am not sure if we want to merge that before merging this PR? Marco, what do you think? |
I agree to merge this PR and continue the discussion separately. The regression doesn't look terrible to me. I just have one last easy idea 👇
Does it help a bit if we change |
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
It didn't seem to work (fefb871), I think embedding structs is mostly syntactic sugar. BucketIndexReader_ExpandedPostings with inlined field (compared to this PR)BucketIndexReader_ExpandedPostings with inlined field (compared to main) |
|
I managed to write the test I intended. Is there anything else to address here or can we merge this? |
colega
reviewed
Apr 10, 2023
colega
approved these changes
Apr 10, 2023
1 task
LGTM! Happy to merge this. |
Do you have an opinion about the |
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
|
ok, I changed "deferred" with "pending" in the package. I'll merge later today if there are no objections |
dimitarvdimitrov
deleted the
dimitar/expanded-postings-unapplied-matchers
branch
I do, but I think getting this stuff done is more important than keep talking about naming :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does
bucketIndexReader.ExpandedPostingsto be able to handle partially resolved expanded postings. A partially resolved expanded posting list for a query is a posting list where not all the matchers from the query have been appliedTotal size of posting groups
This PR uses the
LabelValuesOffsets(instead ofLabelValues) andPostingsOffsetmethods of the index header reader to calculate the size of each posting list and sum them to give the size of the whole posting group.This is expected to increase the memory footprint of ExpandedPostings, although not by a lot. I am in the process of running benchmarks now.
Partially resolved expanded postings lists
This PR introduces a pluggable
postingsSelectionStrategyinterface which thebucketIndexReaderuses to select only some posting groups. Currently, this interface has only one implementationselectAllStrategy, which just selects all posting groups.The plan is to make the strategy selectable via a configuration option.
After running the strategy, the index reader fetches and then intersects/subtracts only the selected posting groups. The reader returns the original matchers of the posting groups that weren't selected - called "unapplied matchers" (naming suggestions welcome).
Caching
The unapplied matchers are stored in the cache item for expanded postings, so they can be applied in each cache fetch. Currently the code doesn't attempt to deal with applying the unapplied matchers, so ExpandedPostings returns an error if it finds any in a cache item. But since
selectAllStrategynever returns unapplied matchers, this case shouldn't be reached.This PR also changes the cache key of expanded postings in order to be able to run different strategies concurrently by different store-gateway replicas. The cache key now contains the name of the strategy that was used to derive the partial postings and the unapplied matchers. This has the side effect that the expanded postings cache will be "invalidated" after rolling out this change.
Which issue(s) this PR fixes or relates to
Related to #4593
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]