Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17839 )
Change subject: [rpc] KUDU-75: refresh DNS entries if proxies hit a network error ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/17839/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17839/3//COMMIT_MSG@12 PS3, Line 12: case nodes are assigned different addresses; today we just retry at : the same location forever. : - KUDU-1620: tablet con > I like the idea of a mode using a config that turns on resolving DNS in cas Yeah, this is essentially the way I've landed on via the addition of a new constructor. http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/connection_id.h File src/kudu/rpc/connection_id.h: http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/connection_id.h@49 PS3, Line 49: std::m > std::move() ? Done 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: > nit: could you rephrase a bit this part of the sentence? I found it hard t Done http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/proxy.cc File src/kudu/rpc/proxy.cc: http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/proxy.cc@106 PS3, Line 106: // By default, we set the real user to the currently logged-in user. : // Effective user and password remain blank. : string real_user; : Status s = GetLoggedInUser(&real_user); : i > Okay to proceed in case of failure? Yep, I'll leave a comment. http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/proxy.cc@185 PS3, Line 185: const > successfully Done http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/rpc/proxy.cc@201 PS3, Line 201: RefreshDnsAndEnqueueRequest(method, req, response, controller, callback); : return; : } : : // Otherwise, just enqueue the request, but retry if there's a network error, : // since it's possible the physical address of the host was changed. We only : // retry once more before calling the callback. : auto refresh_ > What happens if the DNS resolution keeps failing? Will the request be indef We only try once, after which we'll just return to the caller with an error as we would today without the re-resolution. I'll leave a comment. 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 Done 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: gflags::FlagSaver saver; > Should this be wrapped into ASSERT_OK()? Done http://gerrit.cloudera.org:8080/#/c/17839/3/src/kudu/util/net/dns_resolver-test.cc@75 PS3, Line 75: EXPECT_TRUE(HasSuffixString(addr.ToString(), ":12345")); : } : // If we override the DNS lookup address, when we refresh the address, the : // cached entry gets reset. : c > nit: instead, maybe store the results from first resolution at line 67 and Done 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? Done -- 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: 4 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 16 Sep 2021 05:54:04 +0000 Gerrit-HasComments: Yes
