Alexey Serbin has posted comments on this change. Change subject: KUDU-1988: add support for advertised host:port info. ......................................................................
Patch Set 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/6827/10/src/kudu/master/master.cc File src/kudu/master/master.cc: PS10, Line 241: web_server()->GetAdvertisedAddresses Does it make sense to check for return code in this case? http://gerrit.cloudera.org:8080/#/c/6827/10/src/kudu/server/rpc_server-test.cc File src/kudu/server/rpc_server-test.cc: Line 19: nit: consider adding at least #include <memory> #include <vector> #include <gtest/gtest.h> #include "kudu/util/net/sockaddr.h" PS10, Line 38: string advertised = use_advertised_addresses(); nit: consider moving this after that if (!bind.empty()) { ... } closure PS10, Line 50: nit: strange shift (supposed to be 4 spaces) PS10, Line 55: nit: strange shift (supposed to be 4 spaces) PS10, Line 66: const string nit: what's use to return const by value? Maybe, turn this into 'const string&' ? or 'const char*' ? PS10, Line 91: advertised_addrs.size(), 1 here is elsewhere in this file: the expected value should come first, that really helps when reading output when assertion fires. PS10, Line 94: ASSERT_EQ(advertised_addrs[0].host(), "127.0.0.1"); : ASSERT_EQ(advertised_addrs[0].port(), 9999); nit for here and elsewhere in the test: I think for readability it would be better to place those asserts close to the place where advertised_addrs are set, i.e. just after ASSERT_EQ(1, advertised_addrs.size()); PS10, Line 96: bound_addrs[0] does it make sense to add an assert on the size of bound_addrs prior to accessing its first element? http://gerrit.cloudera.org:8080/#/c/6827/10/src/kudu/server/rpc_server.cc File src/kudu/server/rpc_server.cc: PS10, Line 45: DEFINE_string(rpc_bind_addresses, "0.0.0.0", : "Comma-separated list of addresses to bind to for RPC connections. " : "Currently, ephemeral ports (i.e. port 0) are not allowed."); : TAG_FLAG(rpc_bind_addresses, stable); : : DEFINE_string(rpc_advertised_addresses, "", : "Comma-separated list of addresses to advertise externally for RPC connections. " : "Currently, ephemeral ports (i.e. port 0) are not allowed."); : TAG_FLAG(rpc_advertised_addresses, advanced); Should advertised addresses be a subset of bind addresses? If yes, consider adding a group validator on that. -- 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: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Patrik Sundberg <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Patrik Sundberg <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
