search

scylladb / java-driver

ScyllaDB Java Driver for ScyllaDB and Apache Cassandra, based on the DataStax Java Driver
Apache License 2.0
63 stars 37 forks source link

4.x: Add optional fallback for `ControlConnection#reconnect() ` #341

Closed Bouncheck closed 1 month ago

Bouncheck commented 2 months ago

Adds additional config option that changes behavior of ControlConnection's reconnection. In case LBP returns an empty query plan we will attempt to connect to the original endpoints regardless of their last observed status.

The option is by default disabled and driver without enabling it should keep behaving the same way as before. If enabled it should be possible to reconnect to the cluster if all endpoints containing hostnames were lost during normal operation and then cluster was replaced.

dkropachev commented 2 months ago

@Bouncheck , I have been thinking about this solution, more I think less I like it. This solution to feed hosts to controll connection when it reconnects looks hacky to me. I think driver should sync initial contact points to the list of nodes on node events (UP/DOWN/ADD/DEL). Let's discuss it.

dkropachev commented 1 month ago

@Bouncheck , I see that you still use newQueryPlan at one particular place, can you please switch to newControlReconnectionQueryPlan

        Queue<Node> nodes = context.getLoadBalancingPolicyWrapper().newQueryPlan();
dkropachev commented 1 month ago

@Bouncheck , I have been thinking about this solution, more I think less I like it. This solution to feed hosts to controll connection when it reconnects looks hacky to me. I think driver should sync initial contact points to the list of nodes on node events (UP/DOWN/ADD/DEL). Let's discuss it.

Regarding this, I have tried to do it, unfortunately due to the API this fix will be extremely intrusive, let's go ahead with your solution.

Bouncheck commented 1 month ago

@Bouncheck , I see that you still use newQueryPlan at one particular place, can you please switch to newControlReconnectionQueryPlan

        Queue<Node> nodes = context.getLoadBalancingPolicyWrapper().newQueryPlan();

Yes, but it's during the initialization. I believe that part works fine.

dkropachev commented 1 month ago

@Bouncheck , I see that you still use newQueryPlan at one particular place, can you please switch to newControlReconnectionQueryPlan

        Queue<Node> nodes = context.getLoadBalancingPolicyWrapper().newQueryPlan();

Yes, but it's during the initialization. I believe that part works fine.

It works fine, but creates unnecessary confusion.

Bouncheck commented 1 month ago

I reverted that 1 call in ControlConnection init back to newQueryPlan(). I forgot that removing it would require changing around 20 test methods which expect that exact api call

Bouncheck commented 1 month ago

I can adjust them, but I don't know if it's worth it

dkropachev commented 1 month ago

I can adjust them, but I don't know if it's worth it

Please check last commit and tell if you are ok with it.