Michael Ho 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 24: (6 comments) http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/coordinator.cc@1134 PS24, Line 1134: std::string( : reinterpret_cast<const char*>(sidecar_slice.data()), sidecar_slice.size()); Why not std::move(sidecar_slice.ToString()) ? http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@174 PS24, Line 174: UpdateFilterParamsPB* params = obj_pool_.Add(new UpdateFilterParamsPB); Can you please double check if the parameters need to be preserved beyond this function ? According to (https://github.com/apache/impala/blob/master/be/src/kudu/rpc/proxy.h#L79-L84), it can be freed once the async RPC call is done. http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@175 PS24, Line 175: UpdateFilterResultPB* res = obj_pool_.Add(new UpdateFilterResultPB); : RpcController* controller = obj_pool_.Add(new RpcController); I wonder if we can keep these in thread local storage and initialize them once at init time. I suppose we don't invoke this RPC more than once per fragment instance thread, right ? Otherwise, it seems a bit wasteful to keep accumulating these objects in obj_pool_. http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/runtime/runtime-filter-bank.cc@320 PS24, Line 320: // Wait for all inflight rpcs to complete before closing the filters. : { : std::unique_lock<SpinLock> l1(num_inflight_rpcs_lock_); : while (num_inflight_rpcs_ > 0) { : krpcs_done_cv_.wait(l1); : } : } : : lock_guard<mutex> l2(runtime_filter_lock_); : closed_ = true; Do you need to set closed_ to true before waiting for all in-flight RPCs to drain ? Otherwise, you may break out of the critical section above and then a thread may sneak in and try to issues RPC again. Or is it impossible because it's the same fragment instance thread which called RuntimeFilterBank::UpdateFilterFromLocal() ? http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/service/data-stream-service.cc File be/src/service/data-stream-service.cc: http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/service/data-stream-service.cc@137 PS24, Line 137: Substitute("Query State not found for query_id=$0", : PrintId(ProtoToQueryId(req->dst_query_id())) This can be refactored into a single string object and reuse them here and below in RespondAndReleaseRpc(). http://gerrit.cloudera.org:8080/#/c/13882/24/be/src/service/data-stream-service.cc@141 PS24, Line 141: this-> no need for this -- 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: 24 Gerrit-Owner: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Wed, 25 Sep 2019 16:58:23 +0000 Gerrit-HasComments: Yes
