Skip to content
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

Breaking change: reconnect_to_initial_nodes will be called only when AllConnectionsUnavailable #183

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

barshaul
Copy link

@barshaul barshaul commented Aug 14, 2024

  1. Breaking changes: The ClusterConnectionNotFound error has been renamed to AllConnectionsUnavailable, and a new error, ConnectionNotFoundForRoute, has been introduced to handle cases where a specific connection cannot be found. Previously, ClusterConnectionNotFound was incorrectly used for missing specific connections, which triggered reconnect_to_initial_nodes. However, reconnect_to_initial_nodes should only be invoked when all connections are unavailable, as its purpose is to gather more information from the seed nodes. With this update, when ConnectionNotFoundForRoute is encountered, the system will call refresh_slots and retry the request according to the configured retry logic.

  2. Although reconnect_to_initial_nodes is now intended to be called only when no available connections are found, there might still be scenarios where it is triggered even though the connection map isn't empty, due to ongoing tasks refreshing connections. Currently, when reconnect_to_initial_nodes is called, we create a connection map that includes only the seed nodes provided by the user, replacing the existing connection map. This approach introduces a period where the connection map contains only a subset of the cluster nodes (e.g., the seed nodes), and the slots haven't been refreshed yet. During this time, new requests may come in and find connections only to the seed nodes. If the seed nodes are no longer valid, the client will be unable to send requests to random nodes or rely on MOVED responses. This PR updates the function to extend the connection map with connections retrieved from the seed nodes, rather than replacing it entirely.

  3. Reduced number of retries on tests to speed them up

@barshaul barshaul marked this pull request as draft August 14, 2024 15:49
…Unavailable, added new error ConnectionNotFoundForRoute
@barshaul barshaul requested a review from ikolomi August 15, 2024 08:39
@barshaul barshaul marked this pull request as ready for review August 15, 2024 08:39
@barshaul barshaul changed the title Fixed reconnect_to_initial_nodes not to replace the current connection map but to extend it Breaking changes: reconnect_to_initial_nodes will be called only when AllConnectionsUnavailable Aug 15, 2024
@barshaul barshaul changed the title Breaking changes: reconnect_to_initial_nodes will be called only when AllConnectionsUnavailable Breaking change: reconnect_to_initial_nodes will be called only when AllConnectionsUnavailable Aug 15, 2024
Copy link

@ikolomi ikolomi left a comment

Choose a reason for hiding this comment

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

LGTM

@barshaul barshaul merged commit 2d7200f into amazon-contributing:main Aug 15, 2024
10 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.

2 participants