Patrik Sundberg has posted comments on this change. Change subject: KUDU-1988: add support for advertised host:port info. ......................................................................
Patch Set 4: (5 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, yo Done Line 210: for (const Sockaddr addr : rpc_advertised_addresses_) { > Make this a 'const Sockaddr&', otherwise it does an additional implicit cop Done 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 interf I have not checked yet, was meant to today but got sidelined. My thinking was that as the advertised IP tends to by definition be a "public" IP, it tends to always be resolvable by hosts on both internal and external networks. That's the case in my test lab using docker swarm, and would be the case using kubernetes on e.g. Google Cloud as well (my typical prod environment). I will for sure check though! 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? possibly - I used how it's done for http_address_ up at line 120 as template, figured perhaps was a reason why it's 80 there and not opts.port.Looking again now I see http_address_ in the ctor is host:opts.port, and then it defaults to port 80 later in BuildListenSpec. So probably, yes I'll change to opts.port. Line 210: } > Same comment here about checking emptiness upfront vs on each access. understood, I'll work out best way to initialize. -- 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 <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Patrik Sundberg <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
