Skip to content

Modify BloomDimFilter to use BloomFilter instead of BloomKFilter which is not thread safe#6547

Closed
clintropolis wants to merge 2 commits intoapache:masterfrom
clintropolis:bloomkfilter-not-thread-safe
Closed

Modify BloomDimFilter to use BloomFilter instead of BloomKFilter which is not thread safe#6547
clintropolis wants to merge 2 commits intoapache:masterfrom
clintropolis:bloomkfilter-not-thread-safe

Conversation

@clintropolis
Copy link
Copy Markdown
Member

@clintropolis clintropolis commented Oct 30, 2018

Fixes #6546

Query json syntax is modified to match the change:

before:

{
  "type" : "bloom",
  "dimension" : <dimension_name>,
  "bloomKFilter" : <serialized_bytes_for_BloomKFilter>,
  "extractionFn" : <extraction_fn>
}

after:

{
  "type" : "bloom",
  "dimension" : <dimension_name>,
  "bloomFilter" : <serialized_bytes_for_BloomFilter>,
  "extractionFn" : <extraction_fn>
}

clintropolis
clintropolis commented Oct 30, 2018
View reviewed changes
Comment thread ...ions-core/druid-bloom-filter/src/main/java/org/apache/druid/query/filter/BloomDimFilter.java
public boolean applyFloat(float input)
{
return bloomKFilter.testFloat(input);
return bloomFilter.testDouble(input);
Copy link
Copy Markdown
Member Author

@clintropolis clintropolis Oct 30, 2018

Choose a reason for hiding this comment

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

I especially don't like this change, going to try and find something better to do here.

@gianm gianm requested a review from nishantmonu51 October 30, 2018 09:38
@gianm gianm added this to the 0.13.0 milestone Oct 30, 2018
@gianm
Copy link
Copy Markdown
Contributor

gianm commented Oct 30, 2018

@nishantmonu51 - as the original author of #6222, does this change still work for you?

Also, tagging as 0.13.0, since the thread-safety bug should be resolved one way or the other before bloom filter filters are released.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Nov 1, 2018

@nishantmonu51 any thoughts on this one?

@clintropolis
Copy link
Copy Markdown
Member Author

clintropolis commented Nov 2, 2018 •
edited
Loading

An alternative to using BloomFilter instead of BloomKFilter is to maybe bring BloomKFilter into Druid and make the mask be thread local. I'm less into doing that for a backport though maybe.

@clintropolis clintropolis mentioned this pull request Nov 7, 2018
Merged
@b-slim
Copy link
Copy Markdown
Contributor

b-slim commented Nov 7, 2018

as per comment #6584 (comment)
this is not the way to fix the bug thought.

@clintropolis
Copy link
Copy Markdown
Member Author

clintropolis commented Nov 8, 2018

Closing in favor of #6584

@clintropolis clintropolis deleted the bloomkfilter-not-thread-safe branch November 27, 2018 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nishantmonu51 nishantmonu51 Awaiting requested review from nishantmonu51

Assignees

@jihoonson jihoonson

Labels

None yet

Projects

None yet

Milestone

0.13.0

Development

Successfully merging this pull request may close these issues.

4 participants

@clintropolis @gianm @b-slim @jihoonson