Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17850 )
Change subject: [mini-cluster] allow using aliases instead of addresses in EMC ...................................................................... Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/17850/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17850/3//COMMIT_MSG@7 PS3, Line 7: allow using aliases instead of addresses in EMC > nit: would be nice to explicitly outline how this is supposed (not) working Done http://gerrit.cloudera.org:8080/#/c/17850/3//COMMIT_MSG@24 PS3, Line 24: a serve > nit: a server Done http://gerrit.cloudera.org:8080/#/c/17850/3//COMMIT_MSG@25 PS3, Line 25: host_for_test is ad > I'm curious when and how this breaks/works in situation when specifying --r It does work -- the unix domain socket is advertised separately from the rest of the hostports registered by each tserver. http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/integration-tests/dns_alias-itest.cc File src/kudu/integration-tests/dns_alias-itest.cc: http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/integration-tests/dns_alias-itest.cc@43 PS3, Line 43: DECLARE_string(dns_addr_resolution_override); > warning: using decl 'Substitute' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/integration-tests/dns_alias-itest.cc@76 PS3, Line 76: opts.num_masters = 3; > nit: does it make sense to avoid relying on localhost IP address pattern an Done http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/integration-tests/dns_alias-itest.cc@84 PS3, Line 84: > nit: maybe, using ElementDeleter would be a bit cleaner? Done I guess it looks closer to the RAII pattern, sure. http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/integration-tests/dns_alias-itest.cc@93 PS3, Line 93: ptr<ExternalMiniCluster> cluster_; > nit: would be nice to add a comment to explain why to have this condition i Done, indeed there isn't a point in waiting on both. http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/integration-tests/dns_alias-itest.cc@97 PS3, Line 97: TEST_F(DnsAliasITest, TestBasic) { > nit: does it make sense to add NO_FATALS(cluster_->AssertNoCrashes()) ? Done http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/master/sys_catalog.cc@1128 PS3, Line 1128: CHECK( > nit: why not to use CHECK() here? IIUC, this is executed just once in mast Fair point -- I originally assumed this would typically result in a segfault, but it's a good point that it may not be so straightforward and may indeed look like a less obvious memory corruption instead. http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/server/webserver.h File src/kudu/server/webserver.h: http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/server/webserver.h@60 PS3, Line 60: Status GetBoundHostPorts(std::vector<HostPort>* hostports) const WARN_UNUSED_RESULT; > warning: function 'kudu::Webserver::GetBoundHostPorts' has a definition wit Done http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/server/webserver.h@65 PS3, Line 65: Status GetAdvertisedHostPorts(std::vector<HostPort>* hostports) const WARN_UNUSED_RESULT; > warning: function 'kudu::Webserver::GetAdvertisedHostPorts' has a definitio Done http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/util/net/net_util.cc@522 PS3, Line 522: > What if addrs[0] isn't an IP address (i.e. !addrs[0].is_ip()) ? Is it OK t Removed this in the most recent iteration. -- To view, visit http://gerrit.cloudera.org:8080/17850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09a22636748a13f282406119b52021184d92a76f Gerrit-Change-Number: 17850 Gerrit-PatchSet: 5 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 21 Sep 2021 23:35:52 +0000 Gerrit-HasComments: Yes
