Dan Hecht has posted comments on this change. Change subject: IMPALA-4856: Include KRPC services in plan fragment's destinations ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/common/global-flags.cc File be/src/common/global-flags.cc: PS4, Line 39: Kudu still says Kudu. Isn't that confusing since it has nothing to do with kudu (other than being derived from it)? Maybe just call it "KRPC". Or should we make this flag hidden so users don't find it until the feature is finished? http://gerrit.cloudera.org:8080/#/c/7760/4/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: Line 72: // doesn't have to resolve it on every heartbeat. would help to update that comment to include KRPC needing resolved IP. PS4, Line 231: DCHECK nit: DCHECK_EQ http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: Line 353: // Initiating coordinator. would be nice to indicate this is the thrift address. Line 407: // ... which is being executed on this server is that the thrift address? would be nice to comment that. http://gerrit.cloudera.org:8080/#/c/7760/4/common/thrift/StatestoreService.thrift File common/thrift/StatestoreService.thrift: Line 53: // Network address of the Impala service on this backend specify that's the thrift address PS4, Line 73: svc_ maybe just call it krpc_address for consistency e.g. address/krpc_address and server/krpc_server naming. the 'address' field is also that of a "service". okay to ignore if you feel this naming is better. -- 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: 4 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
