[kudu-CR] Add cloning support to BlockBloomFilter and associated allocator

["Bankim Bhavsar (Code Review)" <ge...@cloudera.org>]

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 <ba...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 05 Feb 2020 22:35:55 +0000
Gerrit-HasComments: Yes