-
Notifications
You must be signed in to change notification settings - Fork 37
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
3.x: Reconnect to original contact points #344
base: scylla-3.x
Are you sure you want to change the base?
3.x: Reconnect to original contact points #344
Conversation
91c198f
to
0723236
Compare
I believe this should resolve #224
then with this change the driver should keep trying to reach |
driver-core/src/test/java/com/datastax/driver/core/HostResolutionReconnectionTest.java
Outdated
Show resolved
Hide resolved
bridgeB.close(); | ||
} | ||
} | ||
} |
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.
could you please also implement version of should_connect_with_mocked_hostname
from scylla-4.x
branch
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.
Addressed in v2
driver-core/src/test/java/com/datastax/driver/core/HostResolutionReconnectionTest.java
Outdated
Show resolved
Hide resolved
driver-core/src/test/java/com/datastax/driver/core/HostResolutionReconnectionTest.java
Outdated
Show resolved
Hide resolved
// Configure host resolution | ||
Map<String, String> hostAliasesA = new LinkedHashMap<>(); | ||
hostAliasesA.put("control.reconnect.test", "127.1.1.1"); | ||
HostResolutionRequestInterceptor.INSTANCE.install( |
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.
Can you please make a singleton that holds MappedHostResolver
that is installed.
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.
Should this be a part of core driver module or just core test module?
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.
Test module of course.
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.
Addressed in v2
Map<String, String> hostAliasesB = new LinkedHashMap<>(); | ||
hostAliasesB.put("control.reconnect.test", "127.2.2.1"); | ||
HostResolutionRequestInterceptor.INSTANCE.install( | ||
new MappedHostResolver(hostAliasesB), DefaultHostResolver.INSTANCE); |
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.
I am pretty sure you can manipulate dns responses by accessing instance of MappedHostResolver
that is INSTALLED
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.
Addressed in v2
3bf9b5b
to
26ed1b4
Compare
When control connection tries to reconnect usually it considers only nodes provided by load balancing policy. Usually those do not include what was initially passed to the driver but the recently seen alive nodes. In some setups the IPs can keep changing so it may be useful to have an option to try initial contact points as one of the options during reconnection. Mainly if the contact point is a hostname. This change adds the option to the `QueryOptions` to control that behaviour and adds necessary logic to `ControlConnection` class. It is disabled by default, meaning that default behaviour remains unchanged. Additionally adds org.burningwave tools dependency. This dependency has features that allow for easier host resolution mocking. Adds MappedHostResolverProvider class for testing as a single entry point for controlling hostname resolution. Adds an option to CcmBridge Builder to specify cluster name. Driver checks the cluster name when reconnecting so it will refuse to reconnect to a different CcmBridge auto-generated name.
26ed1b4
to
8970735
Compare
Pushed v2 |
Copied from commit message:
Add option to consider initial contact points during reconnection
When control connection tries to reconnect usually it considers only nodes
provided by load balancing policy. Usually those do not include what was
initially passed to the driver but the recently seen alive nodes. In some
setups the IPs can keep changing so it may be useful to have an option to
try initial contact points as one of the options during reconnection.
Mainly if the contact point is a hostname.
This change adds the option to the
QueryOptions
to control that behaviourand adds necessary logic to
ControlConnection
class. It is disabledby default, meaning that default behaviour remains unchanged.
Additionally adds org.burningwave tools dependency.
This dependency has features that allow for easier host resolution mocking.
Adds MappedHostResolverProvider class for testing as a single entry point
for controlling hostname resolution.
Adds an option to CcmBridge Builder to specify cluster name. Driver checks the
cluster name when reconnecting so it will refuse to reconnect to a different
CcmBridge auto-generated name.