Update Cluster Bus Port out of range error message for Cluster Meet command #240
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hwware
force-pushed
the
port-and-cport-range-check
branch
from
f838260 to
1cf1cef
Compare
hwware
added a commit
to valkey-io/valkey
that referenced
this pull request
Feb 27, 2025
…and (#1686) Now for the cluster meet command, we first check the port and cport number, and then the function clusterStartHandshake() is called. In the function clusterStartHandshake(), ip address is checked first and then port and cport range are checked. Even port or cport range is out of bound, only error message "Invalid node address specified" is reported. This is not correct. In this PR, I just make the port and cport check together before clusterStartHandshake() function. Thus the port number and range can be checked together. One example: Current behavior: 127.0.0.1:7000> cluster meet 10.21.96.98 65000 (error) ERR Invalid node address specified: 10.21.96.98:65000 127.0.0.1:7000> cluster meet 1928.2292.2983.3884.26622 6379 (error) ERR Invalid node address specified: 1928.2292.2983.3884.26622:6379 Whatever the wrong ip address or incorrect bus port number, user get the same error message. New behavior: 127.0.0.1:7000> cluster meet 10.21.96.98 65000 (error) ERR Cluster bus port number is out of range 127.0.0.1:7000> cluster meet 1928.2292.2983.3884.26622 6379 (error) ERR Invalid node address specified: 1928.2292.2983.3884.26622:6379 User can get much clearer error message. Related note pr: valkey-io/valkey-doc#240 --------- Signed-off-by: hwware <[email protected]> Signed-off-by: Wen Hui <[email protected]> Co-authored-by: Viktor Söderqvist <[email protected]>
zuiderkwast
reviewed
Feb 27, 2025
resp2_replies.json
Outdated
| @@ -259,7 +259,7 @@ | |||
| "[Array reply](../topics/protocol.md#arrays): an array of maps where each map contains various attributes and their values of a cluster link." | |||
| ], | |||
| "CLUSTER MEET": [ | |||
| "[Simple string reply](../topics/protocol.md#simple-strings): `OK` if the command was successful. If the address or port specified are invalid an error is returned." | |||
| "[Simple string reply](../topics/protocol.md#simple-strings): `OK` if the command was successful. If the port or cluster bus port number is out of range, an error is returned. If the ip address is invalid, Invalid node address message is returned." | |||
There was a problem hiding this comment.
"Invalid node address message" is an error too.
Do we need to specify document the exact error message here? I think we usually don't document it.
What do you think about this?
Suggested change
| "[Simple string reply](../topics/protocol.md#simple-strings): `OK` if the command was successful. If the port or cluster bus port number is out of range, an error is returned. If the ip address is invalid, Invalid node address message is returned." | |
| "[Simple string reply](../topics/protocol.md#simple-strings): `OK` if the command was successful. If the port or cluster bus port number is out of range, or if an invalid address is specified, an error is returned." |
There was a problem hiding this comment.
Address your comments in 2 files. Please take a look, Thanks
Signed-off-by: hwware <[email protected]>
Signed-off-by: hwware <[email protected]>
hwware
force-pushed
the
port-and-cport-range-check
branch
from
4485b7b to
4d07e16
Compare
zuiderkwast
reviewed
Feb 28, 2025
| @@ -259,7 +259,7 @@ | |||
| "[Array reply](../topics/protocol.md#arrays): an array of [Map reply](../topics/protocol.md#maps) where each map contains various attributes and their values of a cluster link." | |||
| ], | |||
| "CLUSTER MEET": [ | |||
| "[Simple string reply](../topics/protocol.md#simple-strings): `OK` if the command was successful. If the address or port specified are invalid an error is returned." | |||
| "[Simple string reply](../topics/protocol.md#simple-strings): `OK` if the command was successful.If the port or cluster bus port number is out of range, or if an invalid address is specified, an error is returned." | |||
There was a problem hiding this comment.
Looks good, but a space was removed by mistake.
Suggested change
| "[Simple string reply](../topics/protocol.md#simple-strings): `OK` if the command was successful.If the port or cluster bus port number is out of range, or if an invalid address is specified, an error is returned." | |
| "[Simple string reply](../topics/protocol.md#simple-strings): `OK` if the command was successful. If the port or cluster bus port number is out of range, or if an invalid address is specified, an error is returned." |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is related to PR valkey-io/valkey#1686