Michael Ho has posted comments on this change. Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: PS2, Line 172: DCHECK_GT(FLAGS_krpc_port, 0); > I don't think there's a hard and fast rule. In general I think it's not a b As far as I understand, the only other user of this constructor (other than ExecEnv::ExecEnv()) is in-process-server.cc. In that case, it's already setting the FLAGS_be_port in InProcessImpalaServer::StartWithEphemeralPorts(), right ? In addition, there are quite a number of places in the code which access FLAGS_*_port directly so we have a leaky encapsulation anyway. How about we simplify things by merging the two constructors of ExecEnv::ExecEnv() and use FLAGS directly in c'tor ? http://gerrit.cloudera.org:8080/#/c/7760/3/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: 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 i We can replace krpc_svc_address with krpc_svc_port as the port can be different (in theory) for different hosts. Using krpc_svc_address instead of krpc_svc_port is easier as it requires assembling it at one place instead of spreading it out to multiple call sites. That said, it's not terribly expensive to assemble the address either. It just seems more consistent with the existing 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 fro Either way seems fine to me but we still need to propagate the port number if not the entire address. So, it's a matter of shifting MakeNetworkAddress() to the consumers or creating it once here in the producer. Please let me know if you have a strong preference. -- 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
