Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17849 )
Change subject: [util] allow DnsResolver to refresh DNS addresses ...................................................................... Patch Set 1: Verified+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/17849/1/src/kudu/util/net/dns_resolver-test.cc File src/kudu/util/net/dns_resolver-test.cc: http://gerrit.cloudera.org:8080/#/c/17849/1/src/kudu/util/net/dns_resolver-test.cc@89 PS1, Line 89: looksup nit: lookups http://gerrit.cloudera.org:8080/#/c/17849/1/src/kudu/util/net/dns_resolver-test.cc@121 PS1, Line 121: EXPECT_FALSE nit: switch to ASSERT_FALSE() here -- if the container with addresses is empty, there is no sense to continue with further checks below (even though it's turns to be an empty for() loop)? http://gerrit.cloudera.org:8080/#/c/17849/1/src/kudu/util/net/dns_resolver-test.cc@142 PS1, Line 142: EXPECT_OK Switch to ASSERT_OK here -- if ResolveAddresses() fails, there is no sense to continue and try to call validate_addrs() below? http://gerrit.cloudera.org:8080/#/c/17849/1/src/kudu/util/net/net_util.cc File src/kudu/util/net/net_util.cc: http://gerrit.cloudera.org:8080/#/c/17849/1/src/kudu/util/net/net_util.cc@209 PS1, Line 209: host_and_addr[0] == host_ nit: should this comparison be performed in a case-insensitive manner to reflect that DNS names are note case-sensitive (at least when using ASCII symbols)? -- To view, visit http://gerrit.cloudera.org:8080/17849 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0616f3e6fb50aba271f106b05d1926fc46a53ed0 Gerrit-Change-Number: 17849 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 18 Sep 2021 23:54:12 +0000 Gerrit-HasComments: Yes
