Update error message and add the ERR error code #1597
Merged
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
We now have some error messages that do not return an ERR error code: ``` 127.0.0.1:6666> cluster nodes Cluster mode is not enabled ``` As far as i know this doesn't affect the client libs, and doesn't affect the users. But I think it's right to add the error code. Note that Redis will actually count error messages and standardize the error codes, like: ``` 127.0.0.1:6379> info Errorstats # Errorstats errorstat_ERR:count=2 errorstat_EXECABORT:count=1 errorstat_WRONGTYPE:count=1 ``` We mixed using the status message and error output in some places, this PR checks all redis::Error calls and use error status to replace it. (Only the ERR case is handled, others are not handled yet.)
|
It's not a direct objective of this PR, but I'd like to have the |
git-hulk
approved these changes
Jul 20, 2023
@torwig thanks for the review. agree, that is a good suggestion. |
xiaobiaozhao
approved these changes
Jul 20, 2023
p1u3o
pushed a commit
to p1u3o/incubator-kvrocks
that referenced
this pull request
Aug 1, 2023
We now have some error messages that do not return an ERR error code: ``` 127.0.0.1:6666> cluster nodes Cluster mode is not enabled ``` As far as i know this doesn't affect the client libs, and doesn't affect the users. But I think it's right to add the error code. Note that Redis will actually count error messages and standardize the error codes, like: ``` 127.0.0.1:6379> info Errorstats # Errorstats errorstat_ERR:count=2 errorstat_EXECABORT:count=1 errorstat_WRONGTYPE:count=1 ``` We mixed using the status message and error output in some places, this PR checks all redis::Error calls and use error status to replace it. (Only the ERR case is handled, others are not handled yet.) We like to have the "ERR" + some_error_message construction only in one place as an implementation detail of the Redis protocol. Maybe, inside redis::Error or so. We will leave it for a future PR as a cleanup. (pull out the error code and error message)
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.
We now have some error messages that do not return
an ERR error code:
As far as i know this doesn't affect the client libs,
and doesn't affect the users. But I think it's right
to add the error code.
Note that Redis will actually count error messages and
standardize the error codes, like:
We mixed using the status message and error output in
some places, this PR checks all redis::Error calls and
use error status to replace it. (Only the ERR case is
handled, others are not handled yet.)
We like to have the "ERR" + some_error_message construction
only in one place as an implementation detail of the Redis
protocol. Maybe, inside redis::Error or so.
We will leave it for a future PR as a cleanup. (pull out
the error code and error message)