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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 05 Feb 2020 22:35:55 +0000 Gerrit-HasComments: Yes
