Todd Lipcon has posted comments on this change. Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: PS2, Line 219: addrs.size() > nit: while you are at it, would it make sense to update this to Done http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/master/ts_descriptor.h File src/kudu/master/ts_descriptor.h: PS2, Line 129: std::string* host > nit: would it make sense to have the second parameter optional, i.e. change it's a private method and both places we call it, we need the hostname, so not sure the value of making it optional. http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/rpc/connection_id.h File src/kudu/rpc/connection_id.h: PS2, Line 67: > nit: const? Or it's not feasible because of id0 = id1 assignments somewher yea, not feasible due to assignment in proxy.cc http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: PS2, Line 61: tserver::TabletServer::kDefaultPort, &addresses)); : generic_proxy_.reset(new server::GenericServiceProxy( : messenger_, addresses[0], host_port_.host())); : ts_proxy_.reset(new tserver::TabletServerServiceProxy( : messenger_, addresses[0], host_port_.host())); : consensus_proxy_.reset(new consensus::ConsensusServiceProxy( : messenger_, addresses[0], host_port_.host())); > style nit: maybe, introduce variables (references) to address[0] and host_p Done http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/util/net/sockaddr.cc File src/kudu/util/net/sockaddr.cc: PS2, Line 110: string( > nit: consider dropping this for brevity: Done -- To view, visit http://gerrit.cloudera.org:8080/7687 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
