Dan Hecht has posted comments on this change. Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/common/global-flags.cc File be/src/common/global-flags.cc: PS3, Line 39: Kudu RPC that seems confusing since it has nothing to do with Kudu from the users' perspective. maybe it should say "KRPC-based ImpalaInternalService is exported"? or will this port be used outside of that? http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/backend-config.h File be/src/scheduling/backend-config.h: PS3, Line 60: hostname if it's a host name, why is the type TNetworkAddress as opposed to Hostname (like in LookupBackendIp())? http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: PS3, Line 95: FLAGS_krpc_port do we have a check anywhere that this flag is set, and give an error message if not? Line 96: local_backend_descriptor_.__set_krpc_svc_address(krpc_svc_addr); why does that have to get saved in the backend descriptor? since the port is constant and we already have the ip, why not just use those fields rather than creating another? Line 241: DCHECK(desc != nullptr); isn't this dcheck really saying that if LookupBackendDesc() returned nullptr, then we better have host == local_backend_descriptor_.address? If so, seems more direct to just DCHECK that instead of make it an if-stmt. Line 711: // between ComputeScanRangeAssignment() and ComputeFragmentExecParams(). I don't understand that. isn't this just copying the shared_ptr, not the BackendConfig itself? so how does it avoid the inconsistency? http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 1660: MakeNetworkAddress(ip, FLAGS_krpc_port)); another reason to not add krpc_svc_address but instead just generate it from ip and the krpc_port on the fly. -- To view, visit http://gerrit.cloudera.org:8080/7760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8707bfb5028bbe81d2a042fcf3e6e19f4b719a72 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
