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

Reply via email to