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

Add Resolver interface and implementations #332

Merged
merged 2 commits into from
Aug 31, 2024

Conversation

Bouncheck
Copy link
Collaborator

@Bouncheck Bouncheck commented Aug 28, 2024

Requested modification to allow for more control over hostname resolution when testing.
Adds a set of Resolver classes and factories and hooks them into necessary places.
See commit messages and javadoc for more details.

@Bouncheck
Copy link
Collaborator Author

I think this might be a little overzealous in replacing all found calls to InetAddress in core module, I'll probably revert codecs, clients etc. to direct calls. For our use case it's probably most important to plug it in ContactPoints class and metadata subdirectory

Copy link
Collaborator

@dkropachev dkropachev left a comment

Choose a reason for hiding this comment

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

Let's hook into ChannelFactory:

    ChannelFuture connectFuture = bootstrap.connect(RESOLVER.resolve(endPoint.resolve()));

Adds tiny utility method to CcmBridge.
@Bouncheck Bouncheck force-pushed the scylla-4.x-resolver-class branch 6 times, most recently from a7f8918 to 8fc4afa Compare August 30, 2024 15:54
"node-1.cluster.fake",
new ValidResponse(new InetAddress[] {getNodeInetAddress(ccmBridge, 1)}));
MockResolverFactory resolverFactory = new MockResolverFactory();
resolverFactory.addResponses(map);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use addResponse here

@dkropachev dkropachev marked this pull request as ready for review August 30, 2024 17:15
@Bouncheck
Copy link
Collaborator Author

Applied all suggestions with minor fixes.

@Bouncheck
Copy link
Collaborator Author

Changed logic of checking for "unresolvedness". Turns out Java 8 has different toString implementation than the version I used.

Adds a middleman between driver and all direct calls to InetAddress.
This is meant to help with testing in case there is a need to substitute
name resolution with resolution by specific ip address without modifying
system environment or providing custom DNS server.

Default implementation redirects all calls to InetAddress class which
should not introduce change in behaviour.
One introduced downside is that if the methods are to be customizable
then the interface cannot have them `static`.

Adds also implementation of MockResolver that has its own mapping of hostnames
to addresses.

Adds MockResolverIT.
Copy link
Collaborator

@dkropachev dkropachev left a comment

Choose a reason for hiding this comment

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

@Bouncheck , thanks, great job, it looks amazing and we going to get a lot out of it.

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.

2 participants