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

Handle empty addresses in CLUSTER NODES responses #148

Merged
merged 4 commits into from
Jan 15, 2025

Conversation

bjosv
Copy link
Collaborator

@bjosv bjosv commented Jan 15, 2025

When parsing a CLUSTER NODES response we will now treat an empty IP string as it means the same endpoint as the response came from. This due to that the docs mention that an empty string is returned for the IP field when the node doesn't know its own IP address.
The created valkeyClusterNode will get its address from the valkeyContext used when sending the command.

Additional changes are that we will use the noaddr flag instead of the address length when deciding when a node should be skipped; and added validation of the port.

Example of a response snippet with an empty address:
"752d150249c157c7cb312b6b056517bbbecb42d2 :6379@16379 myself,master - 1658754833817 1658754833000 3 connected 5461-10922"

bjosv added 2 commits January 15, 2025 09:20
Use the `noaddr` flag instead of the address length when
checking that a node has an address.
Empty addresses are valid and can be replaced with the
endpoint that was used when sending the command.

Signed-off-by: Björn Svensson <[email protected]>
Treat an empty ip string as it means the same endpoint that
the current command was sent to.

Signed-off-by: Björn Svensson <[email protected]>
@bjosv bjosv requested a review from zuiderkwast January 15, 2025 10:49
Copy link
Collaborator

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Awesome.

We could mention or quote something from the docs that motivate why we do this, in the commit message / PR top comment.

src/cluster.c Show resolved Hide resolved
@bjosv bjosv merged commit c793fa1 into valkey-io:main Jan 15, 2025
43 checks passed
@bjosv bjosv deleted the cluster-nodes branch January 15, 2025 16:32
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