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

Added handling to "?" and NULL hostnames in CLUSTER SLOTS #104

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

barshaul
Copy link

Based on the description of CLUSTER SLOTS (https://redis.io/commands/cluster-slots/):

The preferred endpoint, along with the port, defines the location that clients should use to send requests for a given slot. A NULL value for the endpoint indicates the node has an unknown endpoint and the client should connect to the same endpoint it used to send the CLUSTER SLOTS command but with the port returned from the command. This unknown endpoint configuration is useful when the Redis nodes are behind a load balancer that Redis doesn't know the endpoint of. Which endpoint is set as preferred is determined by the cluster-preferred-endpoint-type config. An empty string "" is another abnormal value of the endpoint field, as well as for the ip field, which is returned if the node doesn't know its own IP address. This can happen in a cluster that consists of only one node or the node has not yet been joined with the rest of the cluster. The value ? is displayed if the node is incorrectly configured to use announced hostnames but no hostname is configured using cluster-announce-hostname. Clients may treat the empty string in the same way as NULL, that is the same endpoint it used to send the current command to, while "?" should be treated as an unknown node, not necessarily the same node as the one serving the current command.

Copy link

@shachlanAmazon shachlanAmazon left a comment

Choose a reason for hiding this comment

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

minor: if we add tests only to one cluster implementation, it's better to add to the async connection, not the sync.

@barshaul barshaul force-pushed the fix_cluster_slots branch 2 times, most recently from 25e725f to ae43cff Compare January 25, 2024 07:26
@barshaul
Copy link
Author

barshaul commented Jan 25, 2024

minor: if we add tests only to one cluster implementation, it's better to add to the async connection, not the sync.

@shachlanAmazon
Added tests for async_cluster too, and moved the test that is the exactly the same for sync and async (test_cluster_async_cannot_connect_to_server_with_unknown_host_name) to the test_cluster_async file.

@barshaul barshaul merged commit 9e51226 into amazon-contributing:main Jan 25, 2024
8 of 10 checks passed
@barshaul barshaul deleted the fix_cluster_slots branch January 25, 2024 08:42
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