Michael Ho has posted comments on this change. Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations ......................................................................
Patch Set 2: (4 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); > Add a TODO to add this to the c'tor - better to reduce the dependency on fl I am actually quite confused as to the guideline for using FLAGS in ExeEnv. We seem to use them quite extensively here and other functions. It'd be great if you can clarify the guideline. A previous version of this patch actually passes FLAGS_krpc_port to c'tor but it seems a bit redundant as it's actually not used. http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/backend-config.h File be/src/scheduling/backend-config.h: PS2, Line 62: TBackendDescriptor > Comment on lifetime of pointer. Will do. http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: PS2, Line 95: TNetworkAddress krpc_svc_addr = MakeNetworkAddress(ip, FLAGS_krpc_port); > can't you get this from ExecEnv::GetInstance instead? It's actually not part of the ExecEnv. I guess it can be but we have to wait till ExecEnv::StartServices() to resolve the hostname to an IP address so may need to do it first thing in ExecEnv::StartServices() before calling Scheduler->Init(). http://gerrit.cloudera.org:8080/#/c/7760/2/be/src/scheduling/scheduler.h File be/src/scheduling/scheduler.h: Line 323: /// backend or the local host itself. > comment on lifetime of return value. Will do. -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
