Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Resolve hostnames using netty's non-blocking DNS resolver #1517
Changes from 1 commit
bb645d2
ec85577
13cd028
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 theresource
package, that's something we can do during the merge.There was a problem hiding this comment.
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 withhttps://github.com/lettuce-io/lettuce-core/pull/1517/files#diff-596af0e5ee58e7de4607c1c26b0ab4bfad308c0f2a713d52caa5a68a2aa1cf9cR112-R113
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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(…)
.There was a problem hiding this comment.
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 createdAddressResolverGroup<?>
instance via supplier.https://www.jenkins.io/doc/developer/plugin-development/optional-dependencies/
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since 6.1
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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} ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since 6.1