Dan Burkert has posted comments on this change. Change subject: KUDU-1988: add support for advertised host:port info. ......................................................................
Patch Set 10: (35 comments) http://gerrit.cloudera.org:8080/#/c/6827/10//COMMIT_MSG Commit Message: Line 7: KUDU-1988: add support for advertised host:port info. > Can you add more detail info here? Done 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? Done 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 Done 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 Done PS10, Line 30: RpcServerAdvertisedAddressesTest() { : } > Nit: can omit Done PS10, Line 38: string advertised = use_advertised_addresses(); > nit: consider moving this after that if (!bind.empty()) { ... } closure Done 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 th Done Line 48: gscoped_ptr<MetricRegistry> metric_registry(new MetricRegistry()); > Nit: should use unique_ptr. Done PS10, Line 50: > nit: strange shift (supposed to be 4 spaces) Done Line 53: std::shared_ptr<rpc::Messenger> messenger; > Add "using std::shared_ptr" and omit the prefix. Done PS10, Line 55: > nit: strange shift (supposed to be 4 spaces) Done 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 Done Line 59: builder.Build(&messenger); > ASSERT_OK on this. Done PS10, Line 60: CHECK_OK(server_->Init(messenger)); : CHECK_OK(server_->Bind()); > It's OK to use ASSERT_OK() in SetUp, I believe. Done PS10, Line 66: const string > nit: what's use to return const by value? Maybe, turn this into 'const str I've dropped the const. Line 69: gscoped_ptr<RpcServer> server_; > unique_ptr here too. Done PS10, Line 91: advertised_addrs.size(), 1 > here is elsewhere in this file: the expected value should come first, that Done 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 addr Done 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 This has been refactored a bit from Adar's suggestion, so I don't think I can do this anymore. PS10, Line 96: bound_addrs[0] > does it make sense to add an assert on the size of bound_addrs prior to acc Done http://gerrit.cloudera.org:8080/#/c/6827/8/src/kudu/server/rpc_server.cc File src/kudu/server/rpc_server.cc: Line 117: if (!options_.rpc_advertised_addresses.empty()) { > This should guard against port 0, as on line 111 above. Done Line 210: > Same comment as in the webserver; this can just use the copy ctor. Done 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 n Done 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, conside No, most likely they will be completely separate. 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 This is consistent with how it's handled above on line 111. 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_ro Done Line 265: AddDefaultPathHandlers(server_.get()); > Is this necessary? We're not actually accessing the webserver. Done 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? Done http://gerrit.cloudera.org:8080/#/c/6827/8/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: Line 207: > This should check for 0 port as well. Done Line 308: for (int i = 0; i < num_addrs; i++) { > This can be simpler as a copy construct: Done Line 314: return Status::OK(); > warning: parameter 'connection' is unused [misc-unused-parameters] Done 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. Can we punt on this? All of the validation in the webserver is currently done by Status, and changing this would mean rewriting a bunch of validation tests. http://gerrit.cloudera.org:8080/#/c/6827/8/src/kudu/server/webserver.h File src/kudu/server/webserver.h: Line 62: virtual void RegisterPathHandler(const std::string& path, const std::string& alias, > warning: default arguments on virtual or override methods are prohibited [g ignoring 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? No, this flag doesn't split the address from the port like webserver_interface/webserver_port do. 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 Done -- 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
