Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resolve hostnames using netty's non-blocking DNS resolver #1517

Conversation

yueki1993
Copy link
Contributor

@yueki1993 yueki1993 commented Nov 13, 2020

Closes: #1498
We now use netty's non-blocking DNS resolver upon connect if optional dependency netty-dns-resolver is provided.

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

I confirmed netty's non-blocking DNS resolver is actually used by debugger.
(Debug run AtLeastOnceTest#connectionIsConnectedAfterConnect with a breakpoint at
https://github.com/netty/netty/blob/netty-4.1.53.Final/transport/src/main/java/io/netty/bootstrap/Bootstrap.java#L206)
image

@yueki1993 yueki1993 force-pushed the resolve-hostnames-by-nonblocking-netty-dns-resolver branch from 2186106 to 67b9243 Compare November 13, 2020 12:48
We now use netty's non-blocking DNS resolver upon connect.
@yueki1993 yueki1993 force-pushed the resolve-hostnames-by-nonblocking-netty-dns-resolver branch from 67b9243 to bb645d2 Compare November 13, 2020 12:53
Comment on lines 309 to 311
connectionBuilder.bootstrap().resolver(
new DnsAddressResolverGroup(new DnsNameResolverBuilder().channelType(Transports.datagramChannelClass())
.socketChannelType(Transports.socketChannelClass().asSubclass(SocketChannel.class))));
Copy link
Contributor Author

@yueki1993 yueki1993 Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Supplemental Comments]
The key point of this PR. cf: https://netty.io/news/2016/05/26/4-1-0-Final.html
If we want to use non-blocking dns resolver, we need to pass DnsAddressResolverGroup to Bootstrap.

Also note that netty encourages users to support TCP fallback in Netty 4.1.37 release note:
https://netty.io/news/2019/06/28/4-1-37-Final.html

I followed Netty 4.1.37 release note, i.e., use DnsNameResolverBuilder with DatagramChannel (UDP) and SocketChannel (TCP).

Since Transports.socketChannelClass() returns Class<? extends Channel> and DnsNameResolverBuilder#socketChannelType wants Class<? extends SocketChannel> channelType,
we need to cast the return value using asSubclass (or change the return type of Transports#socketChannelClass).
I didn't attach a test code about this but for just in case I checked we can cast known socket channels safely like:

    @Test
    void socketChannelClassCastTest() {
        // if failed ClassCastException is thrown.
        NioSocketChannel.class.asSubclass(SocketChannel.class);
        KQueueSocketChannel.class.asSubclass(SocketChannel.class);
        EpollSocketChannel.class.asSubclass(SocketChannel.class);
    }

Comment on lines 325 to +326
channelType(connectionBuilder, redisURI);
resolver(connectionBuilder, redisURI);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Supplemental Comments]
I added resolver(...) all places where channelType(...) is used.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to refactor the connect code a bit to pull things more together. I'll take care of this during the merge.

@yueki1993
Copy link
Contributor Author

[Question]
I did not add a new test regarding this PR because it is hard to verify this PR by UT or IT, and I thought it is enough to verify existing tests do not fail, but what do you think?

/**
* @return the {@link DatagramChannel} class.
*/
Class<? extends DatagramChannel> datagramChannelClass();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question; nits]
For consistency with socketChannelClass(), is it better to declare here as Class<? extends Channel> datagramChannelClass()?
It seems a bit wired because a method for TCP returns Channel while a method for UDP returns DatagramChannel.
Note that if we return Class<? extends Channel> here, we need cast the return value at
https://github.com/lettuce-io/lettuce-core/pull/1517/files#diff-954f0da1e02d2af777f9afc758b8b20f3f2df40a221f1a0e3569ef05402ccd97R310

@yueki1993 yueki1993 changed the title Resolve hostnames using netty's non-blocking DNS resolver #1498 Resolve hostnames using netty's non-blocking DNS resolver Nov 13, 2020
@yueki1993 yueki1993 marked this pull request as ready for review November 13, 2020 14:04
@codecov
Copy link

codecov bot commented Nov 13, 2020

Codecov Report

Merging #1517 (13cd028) into main (634e3d7) will increase coverage by 0.00%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #1517   +/-   ##
=========================================
  Coverage     78.86%   78.86%           
- Complexity     6290     6302   +12     
=========================================
  Files           470      471    +1     
  Lines         21022    21071   +49     
  Branches       2315     2317    +2     
=========================================
+ Hits          16578    16618   +40     
- Misses         3378     3386    +8     
- Partials       1066     1067    +1     
Impacted Files Coverage Δ Complexity Δ
...java/io/lettuce/core/resource/ClientResources.java 66.66% <ø> (ø) 2.00 <0.00> (ø)
.../java/io/lettuce/core/resource/KqueueProvider.java 20.75% <0.00%> (-1.70%) 2.00 <0.00> (ø)
src/main/java/io/lettuce/core/Transports.java 50.00% <40.00%> (ø) 3.00 <1.00> (+1.00)
...n/java/io/lettuce/core/resource/EpollProvider.java 56.60% <50.00%> (-0.54%) 4.00 <0.00> (ø)
...ce/core/resource/AddressResolverGroupProvider.java 64.70% <64.70%> (ø) 3.00 <3.00> (?)
...main/java/io/lettuce/core/AbstractRedisClient.java 83.41% <100.00%> (-0.18%) 53.00 <2.00> (+1.00) ⬇️
src/main/java/io/lettuce/core/RedisClient.java 88.38% <100.00%> (+0.09%) 91.00 <0.00> (ø)
...va/io/lettuce/core/cluster/RedisClusterClient.java 88.10% <100.00%> (+0.03%) 116.00 <0.00> (ø)
.../lettuce/core/resource/DefaultClientResources.java 91.35% <100.00%> (+0.34%) 37.00 <1.00> (+1.00)
.../coroutines/RedisSentinelCoroutinesCommandsImpl.kt 13.63% <0.00%> (-1.37%) 3.00% <0.00%> (ø%)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 634e3d7...13cd028. Read the comment docs.

@mp911de
Copy link
Collaborator

mp911de commented Nov 14, 2020

Thanks a lot for your pull request. The changes look pretty decent. Don't worry about since tag, we can add those during the merge. All changes touch internal API and not so much user-facing API.

I'm wondering whether we should introduce some means of caching (DnsCache, DnsCnameCache, AuthoritativeDnsServerCache) to ensure that we apply caching by default. It would also make sense to have this configurable via ClientResources. Same for DnsServerAddressStreamProvider.

Going further, we should probably deprecate the DNS support classes we provide by Lettuce as we plan to use Netty's DNS resolution mechanism.

I'd suggest making Netty's DNS resolution optional, that is, use it when netty-resolver-dns is on the class path for Lettuce 6.1. With Lettuce 6.2, we can deprecate the Lettuce DNS support classes and make netty-resolver-dns a mandatory dependency.

@mp911de mp911de self-assigned this Nov 14, 2020
@yueki1993
Copy link
Contributor Author

yueki1993 commented Nov 16, 2020

Thanks for your review!

To Comment1:

I'm wondering whether we should introduce some means of caching (DnsCache, DnsCnameCache, AuthoritativeDnsServerCache) to ensure that we apply caching by default.

DnsCache

Overview

I examined netty's implementation in detail. If those objects are not passed to DnsNameResolverBuilder, default implementations of cache classes are used.

Basically they follow a TTL returned by a DNS server. It seems netty does not respect networkaddress.cache.ttl and networkaddress.cache.negative.ttl so this point would be breaking changes. However I feel it is natural to obey netty's default behaviour unless we have a strong reason.

DnsCache

summary

By default the default behaviour is like:

  • For DNS records which are successfuly queried, those are cached with a TTL returned by the DNS server, and
  • Does not store negative caches.

details

By default DefaultDnsCache(minTtl=0, maxTtl=Integer.MAX_VALUE, negativeTtl=0) will be used for DnsCache
(cf: DnsNameResolverBuilder#newCache, DefaultDnsCache)

As you see the source code, the behaviour changes whether additionals is provided or not.

If additionals is not provided:

For successful dns queries, those will be cached here with a TTL returned by DNS server.

For failed dns queries, those will be cached here. However, by default negativeTtl is 0 so negative caches are purged immediately. Note that in this context ttl=0 means not cache forever but purge a cache immediately; the cache purge task is passed to EventExecutorGroup#schedule with delay=0 so the purge task is executed immediately.

Do we need to care a case of additional is provided?

Maybe no. At least I confirmed by debugger this method is used when connect so additionals seems to be empty. I am not quite familiar but it seems additional is related to Client Subnet in DNS Queries and I guess rfc or something defines the data must not be cached when additional is provided.

DefaultDnsCnameCache

summary

By default this class also follows a TTL returned by a DNS server.

details

DefaultDnsCnameCache(minTtl=0, maxTtl=Integer.MAX_VALUE) is used(link: newCnameCache(), DefaultDnsCnameCache).
In this case this class respects originalTtl for caching.

AuthoritativeDnsServerCache

summary

By default this class also follows a TTL returned by a DNS server.
On some condition netty skips caching, but I guess it is related to netty's implementation or something.

To Comment2:

It would also make sense to have this configurable via ClientResources. Same for DnsServerAddressStreamProvider

I strongly agree to make DnsNameResolverBuilder be configurable.

I will add like:

(ClientResources.java)
    ...
    DnsNameResolverBuilder dnsNameResolverBuilder();
    ...
(DefaultClientResources.java)
    ...
    public static final DnsNameResolverBuilder DEFAULT_DNS_RESOLVER_BUILDER =
        new DnsNameResolverBuilder().channelType(Transports.datagramChannelClass())
        .socketChannelType(Transports.socketChannelClass().asSubclass(SocketChannel.class));
    ...    

(Maybe I need to change Transports's scope to public.)

To Comment3:

Going further, we should probably deprecate the DNS support classes we provide by Lettuce as we plan to use Netty's DNS resolution mechanism.

I will answer to this tomorrow or later:

To Comment4:

I'd suggest making Netty's DNS resolution optional, that is, use it when netty-resolver-dns is on the class path for Lettuce 6.1. With Lettuce 6.2, we can deprecate the Lettuce DNS support classes and make netty-resolver-dns a mandatory dependency.

I will answer to this tomorrow or later:

@mp911de
Copy link
Collaborator

mp911de commented Nov 17, 2020

Regarding the configuration DnsNameResolverBuilder : That type comes from netty-resolver-dns. Using the type requires netty-resolver-dns on the class path. Instead, we should return AddressResolverGroup. When netty-resolver-dns is not on the class path, we simply default to DefaultAddressResolverGroup.INSTANCE. If netty-resolver-dns is available, then we would configure DnsNameResolverBuilder according to the submitted code.

So in general, we would be able to configure AddressResolverGroup with ClientResources.Builder.

That also simplifies the cache since a ClientResources instance holding an instance of DnsAddressResolverGroup automatically keeps track of long-lived caches that are reused as much as possible.

@yueki1993
Copy link
Contributor Author

yueki1993 commented Nov 19, 2020

Maybe I could catch up what you want to say regarding DNS cache; unless netty's cache objects are reused, those are newly created every time lettuce creates a new connection, which means DNS caches do not work as we intended.
If we keep AddressResolverGroup in ClientResources and share it with every connection, it try to reuse resolvers, thus we can reuse cache objects.

I think we have following remaining tasks:

  1. add AddressResolverGroup to ClientResources (for lettuce 6.1)
  2. make netty-resolver-dns optional (for lettuce 6.1)
  3. make netty-resolver-dns mandatory and deprecate lettuce DNS support classes (for lettuce6.2)

It seems better to separate pull requests (a pull request of task1&2, and a pull request for task3).
So is it okay to do as follows, or do you have any suggestion?

  • I will complete task 1&2 in this PR,
  • I will raise a new issue of task3, and
  • after this PR is merged or lettuce 6.1 is released, create a pull request of task3

@mp911de
Copy link
Collaborator

mp911de commented Nov 19, 2020

Making netty-resolver-dns mandatory has still a lot of time. So filing another ticket for task3 is sufficient for now.

@yueki1993
Copy link
Contributor Author

Thanks! I will tackle task1&2 at the weekend.

@yueki1993 yueki1993 changed the title Resolve hostnames using netty's non-blocking DNS resolver [WIP] Resolve hostnames using netty's non-blocking DNS resolver Nov 21, 2020
@yueki1993 yueki1993 force-pushed the resolve-hostnames-by-nonblocking-netty-dns-resolver branch from 01287aa to ec85577 Compare November 22, 2020 07:09
@@ -34,7 +34,7 @@
* @author Yohei Ueki
* @since 4.4
*/
class Transports {
public class Transports {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Supplemental Comments]
I made this public because this is now referenced by outside of the package.
ec85577#diff-724688b7234bd44bd8e5f08ce8b0a616ecaf11729ebd99ad72d510431878a6f4R60-R63

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move Transports to the resource package, that's something we can do during the merge.


private static final AddressResolverGroup<?> ADDRESS_RESOLVER_GROUP;

static {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be unit-tested, but I could not come up with a good way to write unit tests regarding this in lettuce's test repository, because we need unittest jars with and without netty-dns-resolver. We might be able do it by dirty-hacking class loader, but I am not quite familiar with it.

I wrote a unit test code outside of this repo:
https://github.com/yueki1993/lettuce-test-PR1517/blob/main/lettuce-test-PR1517-without-netty-dns-resolver/src/test/java/com/github/yueki1993/lettuce_test_PR1517/without_netty_dns_resolver/AddressResolverGroupTests.java
https://github.com/yueki1993/lettuce-test-PR1517/blob/main/lettuce-test-PR1517-with-netty-dns-resolver/src/test/java/com/github/yueki1993/lettuce_test_PR1517/with_netty_dns_resolver/AddressResolverGroupTests.java

@yueki1993
Copy link
Contributor Author

yueki1993 commented Nov 22, 2020

[Question]
ec85577#diff-360c3d0885efdc414995fb4c00c9c30ff0fd3ea7f978f8029fe3fb31e67a35c7R140

        /**
         * Sets the {@link DnsResolver} that is used to resolve hostnames to {@link java.net.InetAddress}. Defaults to
         * {@link DnsResolvers#JVM_DEFAULT}
         *
         * @param dnsResolver the DNS resolver, must not be {@code null}.
         * @return {@code this} {@link Builder}.
         * @since 4.3
         */
        Builder dnsResolver(DnsResolver dnsResolver);

I think DnsResolver currently defaults to DnsResolvers#UNRESOLVED. Can I fix it in this PR?

dnsResolverAvailable = false;
}

// create addressResolverGroup instance via Supplier to avoid NoClassDefFoundError.
Copy link
Contributor Author

@yueki1993 yueki1993 Nov 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Supplemental Comments]
Actually I am not so familiar with jvm's classloader.
I found developer docs of jenkins, and they say using generics types will not raise NoClassDefFoundError.
Thus I wrapped this by Supplier<AddressResolverGroup<?>> and created AddressResolverGroup<?> instance via supplier.
https://www.jenkins.io/doc/developer/plugin-development/optional-dependencies/


/**
* Wraps and provides {@link AddressResolverGroup} classes. This is to protect the user from {@link ClassNotFoundException}'s
* caused by the absence of the {@literal netty-dns-resolver} library during runtime. This class will be deleted when
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Supplemental Comments]
This class can be deleted when netty-dns-resolver becomes mandatory: simply replace here with
https://github.com/lettuce-io/lettuce-core/pull/1517/files#diff-596af0e5ee58e7de4607c1c26b0ab4bfad308c0f2a713d52caa5a68a2aa1cf9cR112-R113

    public static final AddressResolverGroup<?> DEFAULT_ADDRESS_RESOLVER_GROUP = new DnsAddressResolverGroup(new DnsNameResolverBuilder().channelType(Transports.datagramChannelClass())
                .socketChannelType(Transports.socketChannelClass().asSubclass(SocketChannel.class)));

@yueki1993 yueki1993 changed the title [WIP] Resolve hostnames using netty's non-blocking DNS resolver Resolve hostnames using netty's non-blocking DNS resolver Nov 22, 2020
@yueki1993
Copy link
Contributor Author

yueki1993 commented Nov 22, 2020

Hi @mp911de,
I pushed these:

  • add AddressResolverGroup to ClientResources (for lettuce 6.1)
  • make netty-resolver-dns optional (for lettuce 6.1)

Could you review my commits?
I will squash up my commits after this PR gets approved.

Making netty-resolver-dns mandatory has still a lot of time. So filing another ticket for task3 is sufficient for now.

Raised an issue: #1527

@yueki1993 yueki1993 force-pushed the resolve-hostnames-by-nonblocking-netty-dns-resolver branch from d0bbf2c to 13cd028 Compare November 22, 2020 08:19
Copy link
Collaborator

@mp911de mp911de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I left a few minor comments. We can sort out the remaining issues during the merge. Care to squash your commits?

Comment on lines 325 to +326
channelType(connectionBuilder, redisURI);
resolver(connectionBuilder, redisURI);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to refactor the connect code a bit to pull things more together. I'll take care of this during the merge.

@@ -34,7 +34,7 @@
* @author Yohei Ueki
* @since 4.4
*/
class Transports {
public class Transports {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move Transports to the resource package, that's something we can do during the merge.

static {
boolean dnsResolverAvailable;
try {
Class.forName("io.netty.resolver.dns.DnsAddressResolverGroup");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use LettuceClassUtils.isPresent(…).

return ADDRESS_RESOLVER_GROUP;
}

private static DnsAddressResolverGroup defaultDnsAddressResolverGroup() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method signature creates a dependency to DnsAddressResolverGroup that the class loader tries to resolve when the class gets loaded. Please inline this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I tried with my UT and jenkins' doc, this did not produce ClassNotFoundError or something, because it is declared as private and it is not upcasted.
But i don't have strong reason to stick with this, so this also leave to you.

*
* @param addressResolverGroup the {@link AddressResolverGroup} instance, must not be {@code null}.
* @return {@code this} {@link Builder}
* @since xxx
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since 6.1

@@ -70,9 +71,11 @@
* <li>a {@code socketAddressResolver} which is a provided instance of {@link SocketAddressResolver}.</li>
* <li>a {@code timer} that is a provided instance of {@link io.netty.util.HashedWheelTimer}.</li>
* <li>a {@code tracing} that is a provided instance of {@link Tracing}.</li>
* <li>a {@code addressResolverGroup} that is a provided instance of {@link AddressResolverGroup}.</li>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We try to sort elements alphabetically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nits) also i noticed it should be an {@code addressResolverGroup} ...

*
* @param addressResolverGroup the {@link AddressResolverGroup} instance, must not be {@code null}.
* @return {@code this} {@link ClientResources.Builder}
* @since xxx
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since 6.1

@mp911de mp911de added this to the 6.1 M1 milestone Nov 30, 2020
@yueki1993
Copy link
Contributor Author

We can sort out the remaining issues during the merge. Care to squash your commits?

Thanks, I leave remaining issues to you if ok (because it seems quicker than reviewing again).
I don't have strong wish of squashing commits, so this also leaves to you.
Thanks for your reviewing 🙇

mp911de pushed a commit that referenced this pull request Dec 2, 2020
We now use netty's AddressResolverGroup and configure DnsResolverGroup if netty-dns-resolver is on the classpath.

Original pull request: #1517.
mp911de added a commit that referenced this pull request Dec 2, 2020
Move Transports to resource package. Move Bootstrap initialization from Transports to ConnectionBuilder.

Rebase and adopt code to refactorings.

Reorder methods and fields.

Fix KeepAliveOptions.interval(…) builder method.

Original pull request: #1517.
@mp911de
Copy link
Collaborator

mp911de commented Dec 2, 2020

Thank you for your contribution. That's merged, squashed, and polished now.

@mp911de mp911de closed this Dec 2, 2020
@yueki1993 yueki1993 deleted the resolve-hostnames-by-nonblocking-netty-dns-resolver branch December 23, 2020 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Netty's asynchronous DNS resolver
2 participants