[kudu-CR] [util] Add cloning support to BlockBloomFilter and associated allocator
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15121 ) Change subject: [util] Add cloning support to BlockBloomFilter and associated allocator .. [util] Add cloning support to BlockBloomFilter and associated allocator Cloning of predicate data is required in the client, hence this change. Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 Reviewed-on: http://gerrit.cloudera.org:8080/15121 Reviewed-by: Adar Dembo Tested-by: Adar Dembo --- M src/kudu/util/block_bloom_filter-test.cc M src/kudu/util/block_bloom_filter.cc M src/kudu/util/block_bloom_filter.h 3 files changed, 113 insertions(+), 14 deletions(-) Approvals: Adar Dembo: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/15121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 Gerrit-Change-Number: 15121 Gerrit-PatchSet: 6 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add cloning support to BlockBloomFilter and associated allocator
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15121 ) Change subject: [util] Add cloning support to BlockBloomFilter and associated allocator .. Patch Set 5: Verified+1 Overriding Jenkins, failure was unrelated to this change. -- To view, visit http://gerrit.cloudera.org:8080/15121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 Gerrit-Change-Number: 15121 Gerrit-PatchSet: 5 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 08 Feb 2020 05:59:31 + Gerrit-HasComments: No
[kudu-CR] [util] Add cloning support to BlockBloomFilter and associated allocator
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15121 ) Change subject: [util] Add cloning support to BlockBloomFilter and associated allocator .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.h File src/kudu/util/block_bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.h@274 PS4, Line 274: class DefaultBlockBloomFilterBufferAllocator : > Lets see if I can attempt to explain the new implementation that doesn't us Makes sense, thanks for the explanation. -- To view, visit http://gerrit.cloudera.org:8080/15121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 Gerrit-Change-Number: 15121 Gerrit-PatchSet: 5 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 08 Feb 2020 05:59:07 + Gerrit-HasComments: Yes
[kudu-CR] [util] Add cloning support to BlockBloomFilter and associated allocator
Adar Dembo has removed a vote on this change. Change subject: [util] Add cloning support to BlockBloomFilter and associated allocator .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/15121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 Gerrit-Change-Number: 15121 Gerrit-PatchSet: 5 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add cloning support to BlockBloomFilter and associated allocator
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15121 to look at the new patch set (#5). Change subject: [util] Add cloning support to BlockBloomFilter and associated allocator .. [util] Add cloning support to BlockBloomFilter and associated allocator Cloning of predicate data is required in the client, hence this change. Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 --- M src/kudu/util/block_bloom_filter-test.cc M src/kudu/util/block_bloom_filter.cc M src/kudu/util/block_bloom_filter.h 3 files changed, 113 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/15121/5 -- To view, visit http://gerrit.cloudera.org:8080/15121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 Gerrit-Change-Number: 15121 Gerrit-PatchSet: 5 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] Add cloning support to BlockBloomFilter and associated allocator
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/15121 )
Change subject: [util] Add cloning support to BlockBloomFilter and associated
allocator
..
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.h
File src/kudu/util/block_bloom_filter.h:
http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.h@274
PS4, Line 274: class DefaultBlockBloomFilterBufferAllocator : public
BlockBloomFilterBufferAllocatorIf {
> Did you look at gutil/singleton.h to simplify some of this? Might need to i
Lets see if I can attempt to explain the new implementation that doesn't use
shared_from_this() nor Singleton from gutil.
Singleton from gutil returns raw_ptr and not smart ptr. Using
shared_from_this() in Clone() requires an already created shared_ptr on the
same object else behavior is undefined.
So in such a case, public static creation functions must create a shared_ptr so
that shared_from_this() can be returned from Clone() function on the created
object.
In order to create singleton in thread-safe fashion we can use the
gutil/singleton.h. But the raw ptr from gutil/Singleton still needs to be
assigned to the shared_ptr in the public static creation functions atomically.
So then there are 2 options:
1) Use a mutex/std::call_once() for fetching .get() from Singleton and assign
to shared_ptr
2) Rely on the static initialization which is guaranteed to be thread-safe in
C++11.
If we can rely on static initialization for thread-safety I see no need to use
Singleton from gutil. Hence decided to use Meyer's Singleton pattern instead.
Moreover since with Singleton there is a single static instance of shared_ptr,
I can simply return that in Clone() function without needing shared_from_this().
http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.cc
File src/kudu/util/block_bloom_filter.cc:
http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.cc@270
PS4, Line 270:// Can't use std::make_shared<> due to
private default constructor.
> We have a workaround for this in util/make_shared.h, which can work as long
Done
--
To view, visit http://gerrit.cloudera.org:8080/15121
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41
Gerrit-Change-Number: 15121
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 08 Feb 2020 01:00:32 +
Gerrit-HasComments: Yes
[kudu-CR] [util] Add cloning support to BlockBloomFilter and associated allocator
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/15121 )
Change subject: [util] Add cloning support to BlockBloomFilter and associated
allocator
..
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.h
File src/kudu/util/block_bloom_filter.h:
http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.h@274
PS4, Line 274: class DefaultBlockBloomFilterBufferAllocator : public
BlockBloomFilterBufferAllocatorIf {
Did you look at gutil/singleton.h to simplify some of this? Might need to
inherit enable_shared_from_this in order to convert a class instance into a
shared_ptr.
http://gerrit.cloudera.org:8080/#/c/15121/3/src/kudu/util/block_bloom_filter.cc
File src/kudu/util/block_bloom_filter.cc:
http://gerrit.cloudera.org:8080/#/c/15121/3/src/kudu/util/block_bloom_filter.cc@133
PS3, Line 133: }
> What's the threshold for "using" directive?
Good question; we haven't really defined one. The rule of thumb I use is "if
you dropped the std:: prefix, is it still obvious that this class/function
comes from the standard library?"
For me the answer is yes with virtually all classes/collections, and "hmm maybe
not" with really simple primitive functions (like swap, move, min, max, sort,
etc.).
http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.cc
File src/kudu/util/block_bloom_filter.cc:
http://gerrit.cloudera.org:8080/#/c/15121/4/src/kudu/util/block_bloom_filter.cc@270
PS4, Line 270:// Can't use std::make_shared<> due to
private default constructor.
We have a workaround for this in util/make_shared.h, which can work as long as
you're OK making the constructor protected and not private.
--
To view, visit http://gerrit.cloudera.org:8080/15121
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41
Gerrit-Change-Number: 15121
Gerrit-PatchSet: 4
Gerrit-Owner: Bankim Bhavsar
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 05 Feb 2020 22:57:13 +
Gerrit-HasComments: Yes
[kudu-CR] [util] Add cloning support to BlockBloomFilter and associated allocator
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15121 to look at the new patch set (#4). Change subject: [util] Add cloning support to BlockBloomFilter and associated allocator .. [util] Add cloning support to BlockBloomFilter and associated allocator Cloning of predicate data is required in the client, hence this change. Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 --- M src/kudu/util/block_bloom_filter-test.cc M src/kudu/util/block_bloom_filter.cc M src/kudu/util/block_bloom_filter.h 3 files changed, 116 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/15121/4 -- To view, visit http://gerrit.cloudera.org:8080/15121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 Gerrit-Change-Number: 15121 Gerrit-PatchSet: 4 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
