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

Replace min/max token with TokenRange in bloom ring utilities #11960

Merged
merged 7 commits into from
Feb 15, 2024

Conversation

chaudum
Copy link
Contributor

@chaudum chaudum commented Feb 15, 2024

This PR replaces min/max token fields with a TokenRange field that uses the Range[uint32] type.
The Range[uint32] uses similar semantics as the FingerprintBounds we use for fingerprint ranges.

@chaudum chaudum requested a review from a team as a code owner February 15, 2024 09:45
@chaudum chaudum requested a review from salvacorts February 15, 2024 09:46
@chaudum chaudum force-pushed the chaudum/ring-tokenrange branch from 8e083ba to f9399c8 Compare February 15, 2024 09:51
Copy link
Contributor

@salvacorts salvacorts left a comment

Choose a reason for hiding this comment

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

I think this is great. Left some comments. I think being able to map the tokens to whichever key-space we want by using generics would be super handy.

pkg/bloomutils/ring.go Outdated Show resolved Hide resolved
pkg/bloomutils/ring.go Outdated Show resolved Hide resolved
@chaudum
Copy link
Contributor Author

chaudum commented Feb 15, 2024

I think this is great. Left some comments. I think being able to map the tokens to whichever key-space we want by using generics would be super handy.

Yeah, good idea

@chaudum chaudum force-pushed the chaudum/ring-tokenrange branch from af12e2f to e9a0f90 Compare February 15, 2024 11:33
Similar to FingerprintBounds

Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
@chaudum chaudum force-pushed the chaudum/ring-tokenrange branch from c030ec9 to 128c0fb Compare February 15, 2024 11:52
@chaudum chaudum enabled auto-merge (squash) February 15, 2024 11:52
Signed-off-by: Christian Haudum <[email protected]>
@chaudum chaudum merged commit 5967cee into main Feb 15, 2024
7 of 8 checks passed
@chaudum chaudum deleted the chaudum/ring-tokenrange branch February 15, 2024 12:16
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
…ana#11960)

This PR replaces min/max token fields with a `TokenRange` field that uses the `Range[uint32]` type.
The `Range[uint32]` uses similar semantics as the `FingerprintBounds` we use for fingerprint ranges.

Signed-off-by: Christian Haudum <[email protected]>
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.

2 participants