proxystore: stabilize order for duplicates in response losertree #8415
Conversation
This file contains hidden or 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
MichaHoffmann
force-pushed
the
mhoffmann/proxy-stabilize-duplicates-in-response-losertree
branch
3 times, most recently
from
13cc669 to
4d39b27
Compare
saswatamcode
previously approved these changes
Aug 8, 2025
GiedriusS
reviewed
Aug 12, 2025
Currently the order of responses in the losertree on collisions in the labelset is random. This can happen if we drop the replica label in an endpoint. In the case of sidecars the order of responses has effect on deduplication. The primary iterator is used until we find a large enough gap to failover to the replica iterator, where primary and replica is determined by the order they are returned from the proxy losertree. This can lead to slight differences if we repeat a query since different sidecars have scraped at different times possibly. Using the store labelset as tiebreaker is an attempt at stabilizing this. Signed-off-by: Michael Hoffmann <[email protected]>
MichaHoffmann
force-pushed
the
mhoffmann/proxy-stabilize-duplicates-in-response-losertree
branch
from
4d39b27 to
9105dd6
Compare
GiedriusS
reviewed
Aug 25, 2025
| var maxVal *storepb.SeriesResponse = storepb.NewSeriesResponse(nil) | ||
| // It's agnostic to duplicates and overlaps, it forwards all duplicated series ordered by the labelset of their endpoint. | ||
| func NewProxyResponseLoserTree(seriesSets ...respSet) *proxyResponseLoserTree { | ||
| var maxVal seriesResponseWithStoreLabelset = seriesResponseWithStoreLabelset{} |
There was a problem hiding this comment.
Suggested change
| var maxVal seriesResponseWithStoreLabelset = seriesResponseWithStoreLabelset{} | |
| var maxVal = seriesResponseWithStoreLabelset{} |
Type can be inferred from the right-side
| tree: losertree.New( | ||
| seriesSets, | ||
| maxVal, | ||
| func(s respSet) seriesResponseWithStoreLabelset { |
There was a problem hiding this comment.
At() is called more than once for every item in the set. Could we upgrade type respSet interface to be:
type respSet interface {
Close()
At() seriesResponseWithStoreLabelset
?
Plus, s.Labelset() allocates a new string each time. That's not good in a hot path. Internally, it's just:
storeLabelSets []labels.Labels
I think it should be much faster to compare these things directly instead of allocating a bunch of strings that will be identical across millions of series +/-.
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.
Currently the order of responses in the losertree on collisions in the labelset is random. This can happen if we drop the replica label in an endpoint.
In the case of sidecars the order of responses has effect on deduplication. The primary iterator is used until we find a large enough gap to failover to the replica iterator, where primary and replica is determined by the order they are returned from the proxy losertree. This can lead to slight differences if we repeat a query since different sidecars have scraped at different times possibly. Using the store labelset as tiebreaker is an attempt at stabilizing this.
Changes
Verification