Skip to content

Conversation

@sylwiaszunejko
Copy link

@sylwiaszunejko sylwiaszunejko commented Apr 9, 2025

Added support for shard awareness:

  • open connections to every shard from every host
  • use shard aware info when routing queries.

Fixes: #10 #3

@sylwiaszunejko sylwiaszunejko self-assigned this Apr 9, 2025
@sylwiaszunejko sylwiaszunejko force-pushed the use-sharding-info branch 2 times, most recently from 63fe28d to e639495 Compare April 9, 2025 09:12
@sylwiaszunejko sylwiaszunejko force-pushed the use-sharding-info branch 2 times, most recently from fe8a65e to 0557788 Compare April 9, 2025 12:09
@sylwiaszunejko sylwiaszunejko changed the title Use sharding info Create connections based on sharding info Apr 9, 2025
@sylwiaszunejko sylwiaszunejko changed the title Create connections based on sharding info Add shard awareness Apr 11, 2025
@sylwiaszunejko sylwiaszunejko marked this pull request as ready for review April 16, 2025 08:25
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.

Can you please create a test to check if pool is getting filled with connections when connections are disconnecte or become broken.

@sylwiaszunejko sylwiaszunejko force-pushed the use-sharding-info branch 4 times, most recently from e6cf6b0 to 0bfb27c Compare April 24, 2025 09:39
@sylwiaszunejko
Copy link
Author

sylwiaszunejko commented Apr 25, 2025

@dkropachev the last commit makes the driver update the sharding info every time it's null and a new connection is opened. This way, we get the right _expectedConnectionLength (number of shards x number of connections per shard). The driver naturally maintains the correct number of connections without needing additional logic. We probably need more testing to checks if this is suffisient. WDYT?

EDIT: from the CI you can see that this is flaky, needs some time to establish the right amount of connections

@dkropachev dkropachev force-pushed the use-sharding-info branch 2 times, most recently from bbda5aa to ef9abf8 Compare April 29, 2025 03:54
@sylwiaszunejko
Copy link
Author

I corrected formatting, but still need support with the things I mentioned here: #8 (comment)

@dkropachev
Copy link
Collaborator

@dkropachev the last commit makes the driver update the sharding info every time it's null and a new connection is opened. This way, we get the right _expectedConnectionLength (number of shards x number of connections per shard). The driver naturally maintains the correct number of connections without needing additional logic. We probably need more testing to checks if this is suffisient. WDYT?

EDIT: from the CI you can see that this is flaky, needs some time to establish the right amount of connections

I think it is good enough, as I mentioned on the call there is one possible case is when node is scaled down, we will address it in a separate PR/issue if it is real one.

@sylwiaszunejko
Copy link
Author

@dkropachev I added extra checks in the tests, does it look alright? I would also want to clean up the commits, they got a little bit messy, and then we could merge

@dkropachev dkropachev merged commit 359ca37 into scylladb:master Apr 29, 2025
2 checks passed
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.

Refactor shard-aware query routing

2 participants