-
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
fix: 4.x can't connect to the cluster when first node is non responsive #357
base: scylla-4.x
Are you sure you want to change the base?
fix: 4.x can't connect to the cluster when first node is non responsive #357
Conversation
7905ae0
to
23ce185
Compare
79f95ee
to
e0cfb07
Compare
e7155f9
to
b6edf18
Compare
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 seems to be working and resolving the issue. Current version replaces the unresolved InetSocketAddress with DefaultEndPoints with resolved addresses - this is not in line with current description of "advanced.resolve-contact-points" option.
I am not sure if modifying EndPoint interface which is a part of public API is a good idea or necessary.
* | ||
* <p>This will be called each time the driver opens a new connection to the node. The returned | ||
* address cannot be null. | ||
*/ |
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 believe this is not intended documentation for this method. It's the same as for resolve()
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.
thanks, fixed.
return resolvedContactPoints; | ||
} | ||
|
||
@Synchronized |
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.
What does it do? Is it only an indicator it should be synchronized or does it provide the synchronization too?
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.
It does the same, but on the field Lock
, instead of this
, but what a trip, in my head all the code uses @Synchronized
, while it is not. switched to method modifier instead.
@@ -237,4 +235,40 @@ public void run_replace_test_20_times() { | |||
replace_cluster_test(); | |||
} | |||
} | |||
|
|||
@Test | |||
public void cannot_connect_if_first_node_is_unavailable() { |
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 ran it locally and it passes. The name suggests the driver would be unable to connect though.
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 think something along the lines of should_connect_despite_incorrect_first_A_record() would be fine.
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.
fixed.
d3b7fb7
to
9ac58ad
Compare
netty bootstrap.connect uses only first address of unresolved InetSocketAddress. That causes 4.x to not even try to connect to other when it first one fails. This PR makes driver to resolve unresolved endpoint, instead of leaving to to netty. Making it possible to connect to any ip address from DNS contact endpoint.
9ac58ad
to
0dfd84c
Compare
Closes #356
Since netty
bootstrap.connect
uses only first address of unresolved InetSocketAddress 4.x does not even try to connect to other when it fails.This PR makes driver resolve unresolved endpoint itself and only then handing it over
bootstrap.connect