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 <ban...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 29 Jan 2020 00:42:10 +0000 Gerrit-HasComments: Yes
