Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port runtime filter from Thrift RPC to KRPC ...................................................................... Patch Set 14: (9 comments) http://gerrit.cloudera.org:8080/#/c/13882/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13882/14//COMMIT_MSG@14 PS14, Line 14: respctively typo http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/runtime/coordinator.cc@1089 PS14, Line 1089: if (params->bloom_filter().has_directory_sidecar_idx()) { Is it actually possible that this won't be set or can we DCHECK this? (I think its possible with the way you have the code structured right now, but we might want to change that, see my other comments) If it is possible, do we have the right behavior here? I guess what will happen is we'll proceed to updating the bloom filter with an empty directory, which could for example hit a DCHECK in BloomFilter::Or which expects the two directories to be the same size. http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/runtime/coordinator.cc@1099 PS14, Line 1099: .ToString() This ToString() performs a copy of the data. Just call 'size()' on the slice directly. http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/runtime/coordinator.cc@1110 PS14, Line 1110: bloom_filter_directory_ = sidecar_slice.ToString(); I think this will perform two copies - one to do ToString() and one to do the assignment. I think we should be able to do this without any copies by using 'std::swap'. You may need to make bloom_filter_directory_ a char* to make it work http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/runtime/runtime-filter-bank.cc@a103 PS14, Line 103: keep this comment http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/runtime/runtime-filter-bank.cc@208 PS14, Line 208: if (params->has_bloom_filter()) I think this is already guaranteed by the DCHECK above http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/util/bloom-filter.h@46 PS14, Line 46: using kudu::Slice; please don't use 'using' in header files http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/util/bloom-filter.h@90 PS14, Line 90: Bucket* GetDirectory() const { return directory_; } Hmm, definitely preferable to keep all of this stuff private. I think the only reason you exposed this is for bloom-filter-test.cc/bloom-filter-benchmark.cc? If so, you should be able to just make the testing classes friends of this class For bloom-filter-test.cc that should be easy, just 'friend class BloomFilterTest'. Its a little more complicated for bloom-filter-benchmark.cc since there's multiple classes there, but maybe you can add a single super class for them? Or else just list all three with a comment about what they are. If you run into issues with that, another option is to have ToProtobuf take a 'Slice*' or whatever that it sets to the directory instead of take the RpcController. http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/util/bloom-filter.cc File be/src/util/bloom-filter.cc: http://gerrit.cloudera.org:8080/#/c/13882/14/be/src/util/bloom-filter.cc@99 PS14, Line 99: LOG(ERROR) << "Cannot add outbound sidecar: " << sidecar_status.message().ToString(); What should we do in this case? Right now, callers of this function won't know that the error happened, so they'll go ahead and send the BloomFilterPB even though its not really valid, since it doesn't have a directory or always_true/false set. How about in this case we 'disable' the bloom filter, i.e. set always_true = true. Then over in the coordinator we'll know that if always_false = always_true = false it must be the case that sidecar_idx is set. -- To view, visit http://gerrit.cloudera.org:8080/13882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6b394796d250286510e157ae326882bfc01d387a Gerrit-Change-Number: 13882 Gerrit-PatchSet: 14 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Tue, 13 Aug 2019 18:05:51 +0000 Gerrit-HasComments: Yes
