[kudu-CR] Add cloning support to BlockBloomFilter and associated allocator
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/15121 )
Change subject: Add cloning support to BlockBloomFilter and associated allocator
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15121/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15121/1//COMMIT_MSG@11
PS1, Line 11: Since cloning needs to be supported,
DefaultBlockBloomFilterAllocator
: is no longer a singleton.
> What did you think about my other suggestion, to return shared_ptr, which w
There were couple of challenges with returning shared_ptr for Clone() function
with singleton default allocator when I tried last time after using PIMPL
pattern in client for the bloom filter and allocator in KuduBloomFilter.
Earlier in KuduBloomFilter, raw ptrs to bloom filter and allocator were stored
since can't use smart ptrs in client facing header file scan_predicate.h
So converting a shared_ptr to raw ptr and storing it was not possible because
unlike unique_ptr there is no release() in shared_ptr.
In latest patchset of the client, instead of storing raw ptrs to bloom filter
and allocator hiding them with a "data" ptr. The KuduBloomFilter::Data in turn
can store smart ptrs to bloom filter and allocator.
Next was figuring out how to implement a Singleton that returns a shared_ptr
for getInstance() and Clone() method. Implemented using std::call_once() and
storing the instance as a static 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: std::
> Nit: add a using for this.
What's the threshold for "using" directive?
http://gerrit.cloudera.org:8080/#/c/15121/3/src/kudu/util/block_bloom_filter.cc@138
PS3, Line 138: if (!s.ok()) {
: return nullptr;
: }
> It's nice to preserve Statuses when we can, so I'd prefer if Clone returned
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: 3
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:35:55 +
Gerrit-HasComments: Yes
[kudu-CR] Add cloning support to BlockBloomFilter and associated allocator
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/15121 )
Change subject: Add cloning support to BlockBloomFilter and associated allocator
..
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15121/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15121/1//COMMIT_MSG@11
PS1, Line 11: Since cloning needs to be supported,
DefaultBlockBloomFilterAllocator
: is no longer a singleton.
> Changed to returning unique_ptr
What did you think about my other suggestion, to return shared_ptr, which would
let you preserve the default allocator as a singleton?
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: std::
Nit: add a using for this.
http://gerrit.cloudera.org:8080/#/c/15121/3/src/kudu/util/block_bloom_filter.cc@138
PS3, Line 138: if (!s.ok()) {
: return nullptr;
: }
It's nice to preserve Statuses when we can, so I'd prefer if Clone returned the
Status and passed the cloned object via OUT parameter.
--
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: 3
Gerrit-Owner: Bankim Bhavsar
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 31 Jan 2020 23:23:25 +
Gerrit-HasComments: Yes
[kudu-CR] 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 (#3). Change subject: Add cloning support to BlockBloomFilter and associated allocator .. Add cloning support to BlockBloomFilter and associated allocator Cloning of predicate data is required in the client, hence this change. Since cloning needs to be supported, DefaultBlockBloomFilterAllocator is no longer a singleton. 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, 80 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/15121/3 -- 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: 3 Gerrit-Owner: Bankim Bhavsar Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] Add cloning support to BlockBloomFilter and associated allocator
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/15121 )
Change subject: Add cloning support to BlockBloomFilter and associated allocator
..
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15121/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15121/1//COMMIT_MSG@11
PS1, Line 11: Since cloning needs to be supported,
DefaultBlockBloomFilterAllocator
: is no longer a singleton.
> Another option is to have Allocator::Clone return the new allocator in a sh
Changed to returning unique_ptr
http://gerrit.cloudera.org:8080/#/c/15121/1/src/kudu/util/block_bloom_filter.cc
File src/kudu/util/block_bloom_filter.cc:
http://gerrit.cloudera.org:8080/#/c/15121/1/src/kudu/util/block_bloom_filter.cc@134
PS1, Line 134: Status
BlockBloomFilter::Clone(BlockBloomFilterBufferAllocatorIf* allocator,
> Nit: don't need the various this-> qualifiers in here.
Done
http://gerrit.cloudera.org:8080/#/c/15121/1/src/kudu/util/block_bloom_filter.cc@137
PS1, Line 137: auto cleanup = MakeScopedCleanup([&]() {
: delete bf_clone;
: });
> Use unique_ptr instead?
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: 2
Gerrit-Owner: Bankim Bhavsar
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 31 Jan 2020 23:03:37 +
Gerrit-HasComments: Yes
[kudu-CR] Add cloning support to BlockBloomFilter and associated allocator
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/15121 )
Change subject: Add cloning support to BlockBloomFilter and associated allocator
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15121/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15121/1//COMMIT_MSG@11
PS1, Line 11: Since cloning needs to be supported,
DefaultBlockBloomFilterAllocator
: is no longer a singleton.
Another option is to have Allocator::Clone return the new allocator in a
shared_ptr. That gives allocators the freedom to either ref the singleton (as
the default allocator would do), or to really clone the allocator (as the arena
allocator might). Either way, clients "destroy" the cloned allocator by
dereffing the shared_ptr.
Regardless, I'd modify the Clone() signatures so that the cloned object is
returned in some sort of smart pointer. Either unique_ptr if the caller
exclusively owns the clone, or shared_ptr if you want the above behavior.
http://gerrit.cloudera.org:8080/#/c/15121/1/src/kudu/util/block_bloom_filter.cc
File src/kudu/util/block_bloom_filter.cc:
http://gerrit.cloudera.org:8080/#/c/15121/1/src/kudu/util/block_bloom_filter.cc@134
PS1, Line 134: Status
BlockBloomFilter::Clone(BlockBloomFilterBufferAllocatorIf* allocator,
Nit: don't need the various this-> qualifiers in here.
http://gerrit.cloudera.org:8080/#/c/15121/1/src/kudu/util/block_bloom_filter.cc@137
PS1, Line 137: auto cleanup = MakeScopedCleanup([&]() {
: delete bf_clone;
: });
Use unique_ptr instead?
--
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: 1
Gerrit-Owner: Bankim Bhavsar
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 29 Jan 2020 00:42:10 +
Gerrit-HasComments: Yes
[kudu-CR] Add cloning support to BlockBloomFilter and associated allocator
Bankim Bhavsar has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15121 Change subject: Add cloning support to BlockBloomFilter and associated allocator .. Add cloning support to BlockBloomFilter and associated allocator Cloning of predicate data is required in the client, hence this change. Since cloning needs to be supported, DefaultBlockBloomFilterAllocator is no longer a singleton. 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, 84 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/15121/1 -- 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: newchange Gerrit-Change-Id: I74b24ce0959930a3ee7d5df77874e42be0b50b41 Gerrit-Change-Number: 15121 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar
