Adar Dembo has posted comments on this change. Change subject: KUDU-1988: add support for advertised host:port info. ......................................................................
Patch Set 10: (19 comments) http://gerrit.cloudera.org:8080/#/c/6827/10/src/kudu/server/CMakeLists.txt File src/kudu/server/CMakeLists.txt: Line 110: ADD_KUDU_TEST(rpc_server-test) Nit: resort http://gerrit.cloudera.org:8080/#/c/6827/10/src/kudu/server/rpc_server-test.cc File src/kudu/server/rpc_server-test.cc: PS10, Line 30: RpcServerAdvertisedAddressesTest() { : } Nit: can omit Line 42: opts.rpc_bind_addresses = "127.0.0.1:9898"; Can we avoid hardcoding ports in the bound addresses? It's going to make the test flaky. Can we test ephemeral ports only? Line 48: gscoped_ptr<MetricRegistry> metric_registry(new MetricRegistry()); Nit: should use unique_ptr. Line 53: std::shared_ptr<rpc::Messenger> messenger; Add "using std::shared_ptr" and omit the prefix. PS10, Line 54: builder.set_num_reactors(1) : .set_min_negotiation_threads(1) : .set_max_negotiation_threads(1) : .set_metric_entity(metric_entity) : .enable_inbound_tls(); Are all of these non-default settings needed for the test? If not, can you remove them? If so, can you add a little comment for each explaining why? Line 59: builder.Build(&messenger); ASSERT_OK on this. PS10, Line 60: CHECK_OK(server_->Init(messenger)); : CHECK_OK(server_->Bind()); It's OK to use ASSERT_OK() in SetUp, I believe. Line 69: gscoped_ptr<RpcServer> server_; unique_ptr here too. PS10, Line 89: vector<Sockaddr> advertised_addrs; : ASSERT_OK(server_->GetAdvertisedAddresses(&advertised_addrs)); : ASSERT_EQ(advertised_addrs.size(), 1); : vector<Sockaddr> bound_addrs; : ASSERT_OK(server_->GetBoundAddresses(&bound_addrs)); Maybe add a test method that fetches the server's advertised and bound addresses in one go? http://gerrit.cloudera.org:8080/#/c/6827/10/src/kudu/server/rpc_server.cc File src/kudu/server/rpc_server.cc: Line 51: "Comma-separated list of addresses to advertise externally for RPC connections. " Would be nice to include a sentence or two about how when to use this and not just rely on rpc_bind_addresses. PS10, Line 124: LOG(FATAL) << "Advertising an ephemeral port is not supported (RPC advertised address " : << "configured to " << addr.ToString() << ")"; We could do this via gflag validation, no? Just look for a :0 at the end of an address? http://gerrit.cloudera.org:8080/#/c/6827/10/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: Line 241: static_dir_ = GetTestPath("webserver-docroot"); This isn't used by any tests; can we just create it inline with opts.doc_root = ... ? Line 265: AddDefaultPathHandlers(server_.get()); Is this necessary? We're not actually accessing the webserver. Line 287: int32 use_webserver_port() const override { return 9999; } Again, concerned about the flakiness of hard-coding a port here. PS10, Line 298: vector<Sockaddr> advertised_addrs; : ASSERT_OK(server_->GetAdvertisedAddresses(&advertised_addrs)); : ASSERT_EQ(advertised_addrs.size(), 1); : vector<Sockaddr> bound_addrs; : ASSERT_OK(server_->GetBoundAddresses(&bound_addrs)); Add a test method to fetch both sets of addresses? http://gerrit.cloudera.org:8080/#/c/6827/10/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: PS10, Line 220: return Status::InvalidArgument("advertising an ephemeral webserver port is not supported", : addr.ToString()); Again, let's validate this with a gflag validator. http://gerrit.cloudera.org:8080/#/c/6827/10/src/kudu/server/webserver_options.cc File src/kudu/server/webserver_options.cc: Line 46: DEFINE_string(webserver_advertised_addresses, "", For consistency, shouldn't this be named webserver_advertised_interface? Line 47: "Addresses to advertise externally for web connections." Would be nice to add a sentence or two explaining when to use this in lieu of webserver_interface. -- 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: 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
