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

Bloom sharding #192

Merged
merged 19 commits into from
Oct 29, 2020
Merged

Bloom sharding #192

merged 19 commits into from
Oct 29, 2020

Conversation

annanay25
Copy link
Contributor

@annanay25 annanay25 commented Sep 30, 2020 •
edited
Loading

Bloom filter sharding is implemented to reduce bloom sizes for larger blocks, this makes them easily cacheable. Slight but not significant reduction in false positive rate is also expected as there is another round of hashing involved.

Fixes #139

@annanay25
Checkpoint: Initial commit of bloom package
d0f8086 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Checkpoint: Migrate more pkgs to tempo/pkg/bloom
255ff97 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Checkpoint: add WriteTo
73b676d 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Checkpoint: migrated more backends to new interface
379812f 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Checkpoint: migrated all backends and interfaces
2d2089c 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Checkpoint: stuff compiles
d1c8c57 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Checkpoint: docker-compose works
6406369 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Merge branch 'master' into bloom-sharding
2eb4c0e
@annanay25
Small additions
451a459 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
lint
0777d38 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
joe-elliott
joe-elliott reviewed Sep 30, 2020
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 comments. This definitely looks like it's headed in the right direction.

I'm currently unsure on the way to hash into different buckets. Torn between just modding the value and using a real hash function.

pkg/bloom/bloom.go Outdated Show resolved Hide resolved
pkg/bloom/bloom.go Outdated Show resolved Hide resolved
tempodb/backend/cache/cache.go Outdated Show resolved Hide resolved
tempodb/backend/backend.go Outdated Show resolved Hide resolved
tempodb/backend/util/util.go Outdated Show resolved Hide resolved
@annanay25
Checkpoint: compile
7295c7f 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Merge branch 'master' into bloom-sharding
9161782 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25 annanay25 marked this pull request as ready for review October 16, 2020 15:44
joe-elliott
joe-elliott approved these changes Oct 16, 2020
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 ci issues. looks like you have some test to fix. one unfortunate comment. apologies for sending you down a bad path.

other than that looks good!

pkg/bloom/bloom.go Outdated Show resolved Hide resolved
tempodb/encoding/bloom/bloom.go Outdated Show resolved Hide resolved
@annanay25
Add a real test, fix lint
0f3ca00 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Merge branch 'master' into bloom-sharding
7c29747 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Fix disk cache for sharded bloom filter
6fafd5a 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
fix append block test failure
8bcaf09 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
Merge branch 'master' into bloom-sharding
e3c1d78 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
@annanay25
make vendor-check
cbf131f 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
joe-elliott
joe-elliott requested changes Oct 29, 2020
View reviewed changes
Copy link
Member

@joe-elliott joe-elliott left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  • Remove the check on numTenants
  • Add a changelog entry and note it is a breaking change

patch up these two things let's get this merged :)

tempodb/tempodb.go Outdated Show resolved Hide resolved
@annanay25
Remove check on numTenants, add CHANGELOG entry
5a8bf1b 
Signed-off-by: Annanay <annanayagarwal@gmail.com>
joe-elliott
joe-elliott approved these changes Oct 29, 2020
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.

lgtm

@annanay25 annanay25 merged commit d91c415 into master Oct 29, 2020
@annanay25 annanay25 deleted the bloom-sharding branch October 29, 2020 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdisibio mdisibio mdisibio left review comments

@joe-elliott joe-elliott joe-elliott approved these changes

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

Successfully merging this pull request may close these issues.

Reduce Bloom Filter Sizes
3 participants
@annanay25 @joe-elliott @mdisibio