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

(chore) Bloom shipper: Extend Interval struct with utility functions #11841

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

chaudum
Copy link
Contributor

@chaudum chaudum commented Jan 31, 2024

What this PR does / why we need it:

Even though it is mainly used by the bloom shipper and bloom gateway, it should reside next to the FingerprintBounds, which has similar characteristics and utility functions.

Update:

I reverted moving the struct, but kept the utility functions.

@chaudum chaudum requested a review from a team as a code owner January 31, 2024 15:52
@chaudum chaudum changed the title Move Interval struct to bloom library (chore) Blooms: Move Interval struct to bloom library Jan 31, 2024
@chaudum chaudum requested review from owen-d and salvacorts January 31, 2024 15:55
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

I'm not sure Interval makes sense to put in the v1 pkg -- there's nothing in there that specifically uses it? Not really against it either, but you'll want to actually move the struct definition so it compiles :)

@chaudum
Copy link
Contributor Author

chaudum commented Jan 31, 2024

I'm not sure Interval makes sense to put in the v1 pkg -- there's nothing in there that specifically uses it? Not really against it either, but you'll want to actually move the struct definition so it compiles :)

I don't have strong opinions on where the struct resides. I'll move it back to the shipper then.

@chaudum chaudum force-pushed the chaudum/move-interval-struct branch from d2233ea to c747829 Compare January 31, 2024 20:38
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 31, 2024
Similar to the FingerprintBounds struct in the blooms/v1 package.

Signed-off-by: Christian Haudum <[email protected]>
@chaudum chaudum force-pushed the chaudum/move-interval-struct branch from c747829 to 7fcbab7 Compare January 31, 2024 20:52
@chaudum chaudum changed the title (chore) Blooms: Move Interval struct to bloom library (chore) Bloom shipper: Extend Interval struct with utility functions Jan 31, 2024
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

LGTM

@chaudum chaudum enabled auto-merge (squash) February 1, 2024 07:24
@chaudum chaudum merged commit 1191f88 into main Feb 1, 2024
8 checks passed
@chaudum chaudum deleted the chaudum/move-interval-struct branch February 1, 2024 07:48
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
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