Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14745 )
Change subject: Import Impala's blocked based BloomFilter ...................................................................... Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/14745/5/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/14745/5/src/kudu/util/block_bloom_filter.cc@52 PS5, Line 52: "Close() should have been called before the object is destroyed."; > a bit of a strange API, but I guess we are borrowing this from Impala Yeah if the convention in Kudu is to release resources in the destructor, might as well do that. The reasons for doing this in Impala were to a) make it explicit where resources are released and in what order (there's various pitfalls if you use smart pointers and have it happen implicitly) and b) avoid issues with dangling pointers to control structures - if you use the destructor to release resources, then it means you need to be sure that you don't leave dangling pointers to the object. This pattern works well in Impala query execution where these things are important, and where we have convenient points where we can deallocate a whole graph of objects at one (the query finishing). http://gerrit.cloudera.org:8080/#/c/14745/5/src/kudu/util/block_bloom_filter.cc@64 PS5, Line 64: DCHECK(log_num_buckets_ <= 32) << "Bloom filter too large. log_space_bytes: " > should this be either a CHECK or a return of InvalidArgument? seems like a Yeah, this invariant I think came from the Impala planner not generating filters above a size, so might not make sense in this context. -- To view, visit http://gerrit.cloudera.org:8080/14745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I89c54a051c5093cf5fb81481a47a0a6677d7d906 Gerrit-Change-Number: 14745 Gerrit-PatchSet: 5 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 27 Nov 2019 23:23:32 +0000 Gerrit-HasComments: Yes
