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.

Sign up for GitHub

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

Jump to bottom

Add configurable bloom filters #644

Merged
merged 20 commits into from
May 20, 2021
Merged

Add configurable bloom filters #644

merged 20 commits into from
May 20, 2021

Conversation

annanay25
Copy link
Contributor

@annanay25 annanay25 commented Apr 12, 2021 •
edited
Loading

Signed-off-by: Annanay [email protected]

What this PR does:
Provides a way to shard out the bloom filter based on the false positive rate and shard size provided.

Which issue(s) this PR fixes:
Fixes #419

Checklist

  • Tests updated
  • NA Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

joe-elliott
joe-elliott reviewed Apr 12, 2021
View reviewed changes
@annanay25 annanay25 mentioned this pull request May 6, 2021
Closed
annanay25
annanay25 commented May 14, 2021
View reviewed changes
@annanay25 annanay25 marked this pull request as ready for review May 17, 2021 07:04
@annanay25
Copy link
Contributor Author

annanay25 commented May 17, 2021 •
edited
Loading

something to note - with the following parameters:

bloomFP:          0.05,
shardSize:        250*1024,
estimatedObjects: 10_000_000,

the package creates 244 shards. we'll just have that many bloom files in object storage

joe-elliott
joe-elliott reviewed May 17, 2021
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Some thoughts.

I didn't dig super hard on the tests, but I think testStreamingBlockToBackendBlock will test that the bloom filters are functioning correctly.

the package creates 244 shards. we'll just have that many bloom files in object storage
Yeah, I think this will be ok :). If not we're about to find out.

@annanay25
Add more testing
3b77daf 
Signed-off-by: Annanay <[email protected]>
mdisibio
mdisibio requested changes May 18, 2021
View reviewed changes
Copy link
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

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

Nice work, this is much smoother than expected! Left a few comments.

annanay25 added 10 commits May 19, 2021 13:53
@annanay25
Address comments, rename config key, Write() => Marshal()
da69f28 
Signed-off-by: Annanay <[email protected]>
@annanay25
Fix tests
9e8e807 
Signed-off-by: Annanay <[email protected]>
@annanay25
Sanity checks on shardCount
3bb6a8b 
Signed-off-by: Annanay <[email protected]>
@annanay25
Merge branch 'main' into cfgble-bloom
f3d767e 
Signed-off-by: Annanay <[email protected]>
@annanay25
Fix test
7534279 
Signed-off-by: Annanay <[email protected]>
@annanay25
More testing around bloom shard counts
1d4c253 
Signed-off-by: Annanay <[email protected]>
@annanay25
fix TestBlockCleanup
1d8ebe2 
Signed-off-by: Annanay <[email protected]>
@annanay25
CHANGELOG
a04d579 
Signed-off-by: Annanay <[email protected]>
@annanay25
Correctly set meta.TotalObjects in newAppendBlockFromFile
7d59c14 
Signed-off-by: Annanay <[email protected]>
@annanay25
make fmt
072460b 
Signed-off-by: Annanay <[email protected]>
joe-elliott
joe-elliott reviewed May 19, 2021
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Two final thoughts and then I'm good. @mdisibio anything else?

@mdisibio
Copy link
Contributor

LGTM after reducing maxShardCount.

@annanay25 annanay25 merged commit c61b678 into grafana:main May 20, 2021
@annanay25 annanay25 deleted the cfgble-bloom branch May 20, 2021 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe-elliott joe-elliott joe-elliott left review comments

@mdisibio mdisibio mdisibio approved these changes

@dgzlopes dgzlopes Awaiting requested review from dgzlopes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Bloom Filter Sharding Configurable
3 participants
@annanay25 @mdisibio @joe-elliott