Skip to content
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

refactor: Limit processing of blocks to requested fingerprint ranges in bloom gateway #11987

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

chaudum
Copy link
Contributor

@chaudum chaudum commented Feb 19, 2024

What this PR does / why we need it:

This PR limits the amount of data being processed for a single multiplexed iteration to the union of the fingerprint bounds of its requests, instead of looking at blocks from the complete fingerprint range.

pkg/bloomgateway/util.go Outdated Show resolved Hide resolved
@chaudum chaudum changed the title Limit processing of blocks to requested fingerprint ranges Bloom Gateway: Limit processing of blocks to requested fingerprint ranges Feb 19, 2024
@chaudum chaudum requested review from owen-d and salvacorts February 19, 2024 15:03
@chaudum chaudum marked this pull request as ready for review February 19, 2024 15:03
@chaudum chaudum requested a review from a team as a code owner February 19, 2024 15:03
v1.NewBounds(0, 2),
v1.NewBounds(5, 19),
v1.NewBounds(25, 29),
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add a test where there is no overlap but the comparison is between the other two:

mb := bounds(0,10) + bounds (30,40)
target = bounds(15,25)


type MultiFingerprintBounds []v1.FingerprintBounds

func (mb MultiFingerprintBounds) Union(target v1.FingerprintBounds) MultiFingerprintBounds {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for this to behave as expected, the multi-bounds must be
a) sorted already
b) contain disjoint (non-overlapping) ranges

Otherwise you can end up with many overlapping ranges in the result set as well.

Consider:

multi = [(0,6), (5,15)]
target = (8,10)

result = [(0,6), (5,15)] // can be further reduced

Thinking about it, it's probably not a real concern we have if we can ensure the multi-bounds are ordered and non overlapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I solved the issue by using a different algorithm.

@chaudum chaudum requested a review from owen-d February 20, 2024 07:57
This PR limits the amount of data being processed for a single
multiplexed iteration to the union of the fingerprint bounds of its
requests, instead of looking at blocks from the complete fingerprint
range.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum chaudum force-pushed the chaudum/bloomgatway-limit-fp-range branch from e29534d to c8fc064 Compare February 21, 2024 07:08
@chaudum chaudum changed the title Bloom Gateway: Limit processing of blocks to requested fingerprint ranges refactor: Limit processing of blocks to requested fingerprint ranges in bloom gateway Feb 21, 2024
@chaudum chaudum merged commit b47f2bf into main Feb 21, 2024
11 checks passed
@chaudum chaudum deleted the chaudum/bloomgatway-limit-fp-range branch February 21, 2024 22:20
onelapahead pushed a commit to onelapahead/loki that referenced this pull request Feb 22, 2024
…in bloom gateway (grafana#11987)

This PR limits the amount of data being processed for a single multiplexed iteration to the union of the fingerprint bounds of its requests, instead of looking at blocks from the complete fingerprint range.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
…in bloom gateway (grafana#11987)

This PR limits the amount of data being processed for a single multiplexed iteration to the union of the fingerprint bounds of its requests, instead of looking at blocks from the complete fingerprint range.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants