Dan Burkert has posted comments on this change. Change subject: KUDU-1988: add support for advertised host:port info. ......................................................................
Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/6827/4/src/kudu/server/rpc_server.cc File src/kudu/server/rpc_server.cc: Line 121: } Instead of doing the empty check again below in GetAdvertisedAdddresses, you could instead copy rpc_bind_addresses_ to rpc_advertised_addresses_ in an else block here. Line 210: for (const Sockaddr addr : rpc_advertised_addresses_) { Make this a 'const Sockaddr&', otherwise it does an additional implicit copy. http://gerrit.cloudera.org:8080/#/c/6827/4/src/kudu/server/rpc_server.h File src/kudu/server/rpc_server.h: Line 70: Status GetAdvertisedAddresses(std::vector<Sockaddr>* addresses) const WARN_UNUSED_RESULT; Have you tested this in your container setup? I'm worried that this interface is forcing hostnames to be resolved during server startup, so if a hostname is passed that can't be resolved by the server, it will fail. Similarly, if a hostname is passed that resolves differently in the container and externally, it won't work. Changing this to use HostPort instead of Sockaddr would be 'late binding', but it would require some changes in the caller. See https://github.com/apache/kudu/blob/master/src/kudu/util/net/net_util.h#L65 http://gerrit.cloudera.org:8080/#/c/6827/2/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: PS2, Line 301: r< nit: remove trailing whitespace http://gerrit.cloudera.org:8080/#/c/6827/4/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: Line 208: 80, Should this be opts.port? Line 210: } Same comment here about checking emptiness upfront vs on each access. -- To view, visit http://gerrit.cloudera.org:8080/6827 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6735ca5630fc4c426bf72d0b21d6ef452173a890 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Patrik Sundberg <patrik.sundb...@gmail.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
