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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 08 Feb 2020 01:00:32 +0000 Gerrit-HasComments: Yes
