Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13266 )
Change subject: WIP [dns_resolver] KUDU-2791 TTL cache in DNS resolver ...................................................................... Patch Set 3: (1 comment) > (1 comment) > > Could you split up the patch into at least two parts? > 1. Making DnsResolver the class you need it to be. > 2. Plumbing it into new areas of the codebase? Yep. Actually, I started like that but I was not sure that wrapping DnsResolver::ResolveAddresses(const HostPort&, std::vector<SockAddr>*) into a non-static method would be welcome. But if it's explicitly requested -- sure, I'll do. http://gerrit.cloudera.org:8080/#/c/13266/3/src/kudu/util/net/dns_resolver.h File src/kudu/util/net/dns_resolver.h: http://gerrit.cloudera.org:8080/#/c/13266/3/src/kudu/util/net/dns_resolver.h@73 PS3, Line 73: class AsyncDnsResolver: public DnsResolver { > Why model this as a subclass? Why can't we just add async methods to DnsRes I thought it makes sense to have a synchronous-only version. The idea was to use the synchronous-only version in all the places where we use HostPort::ResolveAddresses() -- that would allow to be type-safe and avoid spawning an extra thread where it's not needed. I hope it sounds reasonable, but I'm not against the idea of using the same pattern everywhere if we don't care much about an extra thread. -- To view, visit http://gerrit.cloudera.org:8080/13266 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia1bbd55a8231fd541d2087f9202f24e80bc79f0b Gerrit-Change-Number: 13266 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 28 May 2019 23:48:43 +0000 Gerrit-HasComments: Yes
