Alexey Serbin has posted comments on this change. Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies ......................................................................
Patch Set 2: (5 comments) some nits 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 if (addrs.empty()) ? 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 the signature to Status ResolveSockaddr(Sockaddr* addr, std::string* host = nullptr); ? 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 somewhere? If adding the 'const' modifier is feasible, consider adding it for other members as well. 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_port_.host() and use those as arguments for 3 proxy constructors? 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: return str; -- 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
