Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17839 )
Change subject: wip [rpc] KUDU-75: refresh DNS entries if proxies hit a network error ...................................................................... Patch Set 3: Code-Review+1 (5 comments) overall looks good to me, just a nits for a while http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/proxy.h File src/kudu/rpc/proxy.h: http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/proxy.h@80 PS3, Line 80: the hostport if not given nit: could you rephrase a bit this part of the sentence? I found it hard to parse :) http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/rpc-test-base.h@470 PS3, Line 470: Proxy &p nit: IIRC, current code style suggests using pointers instead of non-const references http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/util/net/dns_resolver-test.cc File src/kudu/util/net/dns_resolver-test.cc: http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/util/net/dns_resolver-test.cc@67 PS3, Line 67: resolver.ResolveAddresses(HostPort("localhost", 12345), &addrs); Should this be wrapped into ASSERT_OK()? http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/util/net/dns_resolver-test.cc@75 PS3, Line 75: for (const Sockaddr& addr : addrs) { : LOG(INFO) << "Address: " << addr.ToString(); : EXPECT_TRUE(HasPrefixString(addr.ToString(), "127.")); : EXPECT_TRUE(HasSuffixString(addr.ToString(), ":12345")); : } nit: instead, maybe store the results from first resolution at line 67 and make sure the same is resolved second time? Also, maybe switch the order of setting FLAGS_dns_addr_resolution_override and RefreshAddressesAsync() calls, so the sequence would be something like: 1. ResolveAddresses() and store the reference results 2. set FLAGS_dns_addr_resolution_override 3. call RefreshAddressesAsync() and make sure the addresses are the referenced fake ones 4. unset FLAGS_dns_addr_resolution_override 5. call RefreshAddressesAsync() and compare with the reference results http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/util/net/dns_resolver-test.cc@88 PS3, Line 88: } nit: add a check for the port number as well? -- To view, visit http://gerrit.cloudera.org:8080/17839 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I777d169bd3a461294e5721f05071b726ced70f7e Gerrit-Change-Number: 17839 Gerrit-PatchSet: 3 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, 11 Sep 2021 06:08:15 +0000 Gerrit-HasComments: Yes
