Alexey Serbin 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 3: (9 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 if mini-cluster uses unix sockets for communication? http://gerrit.cloudera.org:8080/#/c/17850/3//COMMIT_MSG@24 PS3, Line 24: servers nit: a server http://gerrit.cloudera.org:8080/#/c/17850/3//COMMIT_MSG@25 PS3, Line 25: host_ports_for_test I'm curious when and how this breaks/works in situation when specifying --rpc_listen_on_unix_domain_socket=true as well? Does it makes sense to add some sort of flag validator to fail early in such case with proper diagnostics, or the way how it works/doesn't work together provides all the necessary information? 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@76 PS3, Line 76: ASSERT_STR_NOT_CONTAINS(master_addrs, "127."); nit: does it make sense to avoid relying on localhost IP address pattern and maybe use Sockaddr::ParseFromNumericHostPort() to make sure the addresses cannot be parsed since they should not be represented as IPv4 addresses? http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/integration-tests/dns_alias-itest.cc@84 PS3, Line 84: STLDeleteElements(&tservers); nit: maybe, using ElementDeleter would be a bit cleaner? http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/integration-tests/dns_alias-itest.cc@93 PS3, Line 93: w.rows_inserted() < 10 && w.batches_completed() < 10 nit: would be nice to add a comment to explain why to have this condition instead of simply w.rows_inserted() < 10 or w.batches_completed() < 10, otherwise maybe simplify this criterion by removing an extra comparison? http://gerrit.cloudera.org:8080/#/c/17850/3/src/kudu/integration-tests/dns_alias-itest.cc@97 PS3, Line 97: } nit: does it make sense to add NO_FATALS(cluster_->AssertNoCrashes()) ? 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: DCHECK nit: why not to use CHECK() here? IIUC, this is executed just once in master lifecycle and not in hot path, right? My concern is that in case if RPC server could not bind, this might lead to memory corruption when running HostPortToPB() on non-existent element. 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: addrs[0] What if addrs[0] isn't an IP address (i.e. !addrs[0].is_ip()) ? Is it OK to crash here in DEBUG mode, is some other handling is preferred in such a case? -- 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: 3 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: Mon, 20 Sep 2021 22:01:28 +0000 Gerrit-HasComments: Yes
