Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port UpdateFilter() and PublishFilter() to KRPC ...................................................................... Patch Set 11: (22 comments) http://gerrit.cloudera.org:8080/#/c/13882/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13882/11//COMMIT_MSG@11 PS11, Line 11: Misc note: fixed the memory leak in data-stream-service.cc. This sort of stuff shouldn't be included in the commit message http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h File be/src/runtime/backend-client.h: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h@25 PS11, Line 25: //#include "gen-cpp/control_service.pb.h" : //#include "gen-cpp/data_stream_service.pb.h" try not to leave commented out code in patches you submit for review http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h@37 PS11, Line 37: : try not to include unrelated formatting changes in patches you submit for review http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h@47 PS11, Line 47: /// We intentionally disable this clang warning as we intend to hide the : /// the same-named functions defined in the base class. : #pragma clang diagnostic push : #pragma clang diagnostic ignored "-Woverloaded-virtual" : : #pragma clang diagnostic pop this can all be removed since you removed the code that was causing the clang warning http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@45 PS11, Line 45: #include "gen-cpp/data_stream_service.proxy.h" : #include "service/data-stream-service.h" nit: alphabetize http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@539 PS11, Line 539: TUniqueId qid = ProtoToQueryId(rpc_params.dst_query_id()); Since this is only used once, you can just put the call inside the DCHECK. That will also save the cost of doing the conversion on release builds when the DCHECK is elided. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@551 PS11, Line 551: krpc_address, krpc_address.hostname This is wrong, see: https://gerrit.cloudera.org/#/c/13818/ (you'll just need to do the same thing as the other calls to GetProxy in this class) http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@555 PS11, Line 555: // TODO: Retry. Drop this. GetProxy() is not something that we'll probably ever retry. If you go look through the code for it, I think the only way it can fail is if the network address doesn't parse correctly, which is not something that will be fixed by retrying. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@556 PS11, Line 556: LOG(INFO) WARNING http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@566 PS11, Line 566: LOG(INFO) WARNING http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@563 PS11, Line 563: do { : rpc_status = proxy->PublishFilter(rpc_params, &res, &controller); : if (!rpc_status.ok()) { : LOG(INFO) << "PublishFilterPB() failed: " << rpc_status.message().ToString(); : controller.Reset(); : } : } while (!rpc_status.ok()); Let's keep it like it was before an not retry. We may someday want to consider retrying, but its best to do things incrementally and keep this patch focused just on the refactor. That allows us to think stuff like this through more when we actually do it, eg. you wouldn't want to retry like this because if it never succeeds, say because the impalad you're trying to send to went down, you're stuck in an infinite loop. But then that starts opening up questions like how many times should we retry, how long should we wait between retries, should we retry regardless of what the error is or are some errors clearly non-recoverable and we can stop, etc. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-filter-state.h File be/src/runtime/coordinator-filter-state.h: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-filter-state.h@68 PS11, Line 68: * I think we can avoid making this change, see my comment in coordinator.cc http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.h@32 PS11, Line 32: #include "gen-cpp/control_service.pb.h" I don't think you need to import control_service here http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@926 PS11, Line 926: no need to add more whitespace http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@976 PS11, Line 976: // To check whether or not the following usage of Swap function is correct remove this http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@977 PS11, Line 977: rpc_params.mutable_bloom_filter()->Swap(aggregated_filter); I think we can swap this around, i.e. aggregated_filter.Swap(rpc_params.mutable_bloom_filter()) to avoid the change in coordinator-filter-state.h http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@992 PS11, Line 992: UniqueIdPB* qid = new UniqueIdPB; : TUniqueIdToUniqueIdPB(query_id(), qid); : rpc_params.set_allocated_dst_query_id(qid); This can be simplified to : TUniqueIdTOUniqueIdPB(query_id(), rpc_params.mutable_dst_query_id()); http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@998 PS11, Line 998: LOG(INFO) << "target_fragment_idxs.size(): " << target_fragment_idxs.size(); remove this http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@1008 PS11, Line 1008: cleanup: The formatting was correct before, see the output of clang-format http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@1040 PS11, Line 1040: // Workaround for fact that parameters are const& for Thrift RPCs update this http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@1066 PS11, Line 1066: formatting http://gerrit.cloudera.org:8080/#/c/13882/11/common/protobuf/common.proto File common/protobuf/common.proto: http://gerrit.cloudera.org:8080/#/c/13882/11/common/protobuf/common.proto@43 PS11, Line 43: this is a union over all possible return types nit: capitalize and period Obviously we're somewhat inconsistent about this across the codebase, but we try to be consistent within individual files. -- 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: 11 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: Fri, 26 Jul 2019 19:42:58 +0000 Gerrit-HasComments: Yes
