Skip to content

4.x: Add advanced shard awareness #517

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

Open
wants to merge 4 commits into
base: scylla-4.x
Choose a base branch
from

Conversation

Bouncheck
Copy link
Collaborator

@Bouncheck Bouncheck commented Apr 14, 2025

Adds advanced shard awareness to the 4.x version of the driver.
Added integration test that checks if reconnections are sufficiently rare with advanced shard awareness.
should_struggle_to_fill_pools and should_not_struggle_to_fill_pools can be run locally to see the difference between the driver with and without this feature.

Fixes #515

@Bouncheck Bouncheck self-assigned this Apr 14, 2025
@Bouncheck Bouncheck force-pushed the scylla-4.x-adv-shard-awareness branch 5 times, most recently from b2aa823 to f3dc5ef Compare April 14, 2025 21:59
@Bouncheck Bouncheck marked this pull request as ready for review April 14, 2025 21:59
@Bouncheck
Copy link
Collaborator Author

Bouncheck commented Apr 14, 2025

I'll probably introduce some small checks to port allocator (for example the range should not be too small) and some unit tests but the gist of the PR will probably look as it is now.

@Bouncheck Bouncheck force-pushed the scylla-4.x-adv-shard-awareness branch 2 times, most recently from 560a1f2 to 33a9063 Compare April 16, 2025 19:07
@Bouncheck Bouncheck requested a review from dkropachev April 16, 2025 19:17
@dkropachev
Copy link
Collaborator

@Bouncheck , I realy don't like idea of PortAllocator, there is a gap between when you allocate port and create a socket, if by then port become busy, then connection attempt fails.
I have pushed one commit to address that, please take a look.

@dkropachev dkropachev force-pushed the scylla-4.x-adv-shard-awareness branch from 16f0fda to 90d63df Compare April 17, 2025 04:30
@Bouncheck
Copy link
Collaborator Author

@Bouncheck , I realy don't like idea of PortAllocator, there is a gap between when you allocate port and create a socket, if by then port become busy, then connection attempt fails. I have pushed one commit to address that, please take a look.

There always is that possibility. PortIterator suffers from the same thing. The only solution is to try again.

From what I see PortIterator does not check if returned port was free at all.
Multiple instances use independent counters, which can result in more situations where connections snatch ports from each other. Note that PortAllocator uses global atomic counter, so subsequent calls will not return the same port number unless a very small range of free ports is given.
I think out of the two I prefer the allocator.

@dkropachev
Copy link
Collaborator

@Bouncheck , I realy don't like idea of PortAllocator, there is a gap between when you allocate port and create a socket, if by then port become busy, then connection attempt fails. I have pushed one commit to address that, please take a look.

There always is that possibility. PortIterator suffers from the same thing. The only solution is to try again.

From what I see PortIterator does not check if returned port was free at all. Multiple instances use independent counters, which can result in more situations where connections snatch ports from each other. Note that PortAllocator uses global atomic counter, so subsequent calls will not return the same port number unless a very small range of free ports is given. I think out of the two I prefer the allocator.

It does that when socket is actually allocated, and no, it doesn't suffer from it, it will retry on next port if port is busy

@dkropachev
Copy link
Collaborator

@Bouncheck , also another reason to stick to PortIterator is that it keeps behavior inline with OS and driver socket configurations:

  1. DefaultDriverOption.SOCKET_REUSE_ADDRESS
  2. UnixChannelOption.SO_REUSEPORT
  3. sysctl net.ipv4.tcp_tw_reuse

@dkropachev dkropachev force-pushed the scylla-4.x-adv-shard-awareness branch from 90d63df to 67348e7 Compare April 17, 2025 17:10
@Bouncheck
Copy link
Collaborator Author

Bouncheck commented Apr 18, 2025

@Bouncheck , I realy don't like idea of PortAllocator, there is a gap between when you allocate port and create a socket, if by then port become busy, then connection attempt fails. I have pushed one commit to address that, please take a look.

There always is that possibility. PortIterator suffers from the same thing. The only solution is to try again.
From what I see PortIterator does not check if returned port was free at all. Multiple instances use independent counters, which can result in more situations where connections snatch ports from each other. Note that PortAllocator uses global atomic counter, so subsequent calls will not return the same port number unless a very small range of free ports is given. I think out of the two I prefer the allocator.

It does that when socket is actually allocated, and no, it doesn't suffer from it, it will retry on next port if port is busy

Both solutions are just black boxes that return an int when asked. In the time between they return an int and bootstrap.connect(...) is called the port with the same number can be stolen. So both do suffer from it and it cannot be really solved other than trying the whole process another time.

The additional bind exception handling you've added is separate from portIterator and can be done with any other port number picking black box as well, but it does not solve this problem (does not prevent stealing the port).

@Bouncheck
Copy link
Collaborator Author

@Bouncheck , also another reason to stick to PortIterator is that it keeps behavior inline with OS and driver socket configurations:

1. `DefaultDriverOption.SOCKET_REUSE_ADDRESS`

2. `UnixChannelOption.SO_REUSEPORT`

3. sysctl net.ipv4.tcp_tw_reuse

I don't understand this part

@dkropachev
Copy link
Collaborator

dkropachev commented Apr 18, 2025

@Bouncheck , also another reason to stick to PortIterator is that it keeps behavior inline with OS and driver socket configurations:

1. `DefaultDriverOption.SOCKET_REUSE_ADDRESS`

2. `UnixChannelOption.SO_REUSEPORT`

3. sysctl net.ipv4.tcp_tw_reuse

I don't understand this part

When you create a socket or bind it, the OS is generally expected to return an error if the specified port and/or address is already in use.
However, there are exceptions to this behavior, and they are influenced by two main factors:

Socket options:

SO_REUSEPORT and SO_REUSEADDR (exposed in Netty via ChannelOption.SO_REUSEADDR and UnixChannelOption.SO_REUSEPORT).

At least one of these is also configurable through the driver using DefaultDriverOption.SOCKET_REUSE_ADDRESS.

OS-level settings:

For example, on Linux, the sysctl setting net.ipv4.tcp_tw_reuse.

The outcome depends on the combination of SO_REUSEPORT, SO_REUSEADDR, net.ipv4.tcp_tw_reuse, and the current state of the address/port.
These rules differ between bind and listen operations.

Since you're calling listen first, the behavior will follow the rules specific to listen.

@dkropachev dkropachev force-pushed the scylla-4.x-adv-shard-awareness branch 4 times, most recently from 4708b33 to 33a9063 Compare April 22, 2025 12:43
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.

LGTM, one more test needed and and couple of doc issues.

# Whether to use advanced shard awareness when trying to open new connections.
#
# Having this enabled makes sense only for ScyllaDB clusters.
# Results in smaller connection storms in multi-client settings.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it ? I don't see any difference in this regard between regular shard awarness and advanced, if anything it will reduce number of reconnections needed to fullfill connection pool.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it decreases the number of reconnections needed then it lessens the load, no? Of course all driver applications will need to use it for best results.
Maybe I'm using a wrong term here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please refrase it so that it does not confuse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, but which part? I don't know which part is the problem

Copy link
Collaborator

@dkropachev dkropachev Apr 23, 2025

Choose a reason for hiding this comment

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

# Results in smaller connection storms in multi-client settings. -> # Enabling it reduces amount of reconnections driver does to get connection pool filled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to Reduces number of reconnections driver needs to fully initialize connection pool.

@Bouncheck Bouncheck force-pushed the scylla-4.x-adv-shard-awareness branch from 956cbd9 to 799de7e Compare April 22, 2025 19:10
@Bouncheck
Copy link
Collaborator Author

Bouncheck commented Apr 22, 2025

Addresed docs comments and added 1 change to port allocator. It will follow reuse address setting from the config.
Reasoning is that in theory port allocator using false all the time could prevent the driver from using some port that would actually work if reuse setting was respsected.

Adds config options for toggling the feature on and defining
port ranges to use.
By default the feature will be enabled.
With advanced shard awareness target shard is a necessary parameter
for connecting. Extends non public methods with new parameters and
provides one additional public method.
Passing `null` as shard id or ShardingInfo will lead to previous non-shard
aware behavior.
Makes `addMissingChannels` use advanced shard awareness.
It will now specify target shard when adding missing channels for
specific shards.
In case returned channels do not match requested shards warnings are logged.
Initial connection to the node works on previous rules, meaning it uses
arbitrary local port for connection to arbitrary shard.

Adds AdvancedShardAwarenessIT that has several methods displaying
the difference between establishing connections with option enabled
and disabled.

Adds ChannelPoolShardAwareInitTest
@Bouncheck Bouncheck force-pushed the scylla-4.x-adv-shard-awareness branch from 1680633 to 1d92051 Compare April 23, 2025 19:52
@Bouncheck
Copy link
Collaborator Author

Squashed fixes into appropriate commits and added missing @RunWith(DataProviderRunner.class)

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.

4.x: Advanced shard awareness
2 participants