Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10855 )
Change subject: IMPALA-7213, IMPALA-7241: Port ReportExecStatus() RPC to use KRPC ...................................................................... Patch Set 16: (4 comments) http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/rpc/rpc-mgr.h@146 PS16, Line 146: service_name admittedly it's clever that you were able to hack the username in order to get separate connections, but I'm finding it somewhat strange to follow when looking at the code. Here, it's not really clear why you need to pass the service name if you're already passing the service class type -- the service name is already available there by calling P::static_service_name(), so the parameter seems redundant. Given that, as more of these services get converted over to KRPC, we probably just want a 'data plane' and 'control plane', maybe it makes more sense to add a new enum type like 'CommunicationPlane' with options kDataPlane and kControlPlane? Internally, you could still implement this by hacking the username for now, but at least it makes it clearer what the purpose of this parameter is. http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/coordinator-backend-state.cc@506 PS16, Line 506: { why is this extra indentation block here? doesn't seem like there is any RAII or lock acquisition that needs this scope http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/10855/16/be/src/runtime/fragment-instance-state.h@107 PS16, Line 107: const nit: const non-reference parameters dont make much sense http://gerrit.cloudera.org:8080/#/c/10855/12/common/protobuf/common.proto File common/protobuf/common.proto: http://gerrit.cloudera.org:8080/#/c/10855/12/common/protobuf/common.proto@33 PS12, Line 33: fixed > Agreed that they don't tend to be small. Does using fixed64 make the encodi yea, not that it really matters for this use case, but figured it's more instructive -- To view, visit http://gerrit.cloudera.org:8080/10855 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7638583b433dcac066b87198e448743d90415ebe Gerrit-Change-Number: 10855 Gerrit-PatchSet: 16 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Michal Ostrowski <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 10 Oct 2018 23:18:32 +0000 Gerrit-HasComments: Yes
