Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14388 )
Change subject: IMPALA-9026: Use resolved IP address for statestore subscriber ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/14388/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14388/2//COMMIT_MSG@19 PS2, Line 19: coordinator nit: line too long http://gerrit.cloudera.org:8080/#/c/14388/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/14388/2/be/src/runtime/exec-env.cc@222 PS2, Line 222: // Resolve hostname to IP address. : ABORT_IF_ERROR(HostnameToIpAddr(FLAGS_hostname, &ip_address_)); : : // KRPC relies on resolved IP address. : krpc_address_.__set_hostname(ip_address_); whats the reason for moving these from Init() to the constructor? http://gerrit.cloudera.org:8080/#/c/14388/2/be/src/statestore/statestore-subscriber.cc File be/src/statestore/statestore-subscriber.cc: http://gerrit.cloudera.org:8080/#/c/14388/2/be/src/statestore/statestore-subscriber.cc@262 PS2, Line 262: hostname document that the hostname can now either be the actual hostname, or the ip_address of the host? another approach might be to add an optional ip_address field to the Thrift struct; users of TNetworkAddress first check if the ip_address is present, if not, it uses the hostname. Might not be worth the effort, but thought I would mention it. -- To view, visit http://gerrit.cloudera.org:8080/14388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb8302dec0e52beb9f0b88306a51c38ff42a63a2 Gerrit-Change-Number: 14388 Gerrit-PatchSet: 2 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Wed, 09 Oct 2019 17:19:30 +0000 Gerrit-HasComments: Yes
