Fix redundant ERR prefix in cluster redirect error message #1928
Conversation
Currently, Kvrocks clsuter mode returns MOVED/TRYAGAIN error message with the 'ERR' prefix, which will cause the redis client can't recognize the redirection message. To make it compatible with Redis client, we should remove this prefix. Before this patch, Kvrocks will return a MOVED error message: ``` > redis-cli -p 30001 -c set a 1 (error) ERR MOVED 15495 127.0.0.1:30005 ``` After applying this patch, it works well with redis-cli redirection: ``` > redis-cli -p 30001 -c set a 1 OK ```
|
Kudos, SonarCloud Quality Gate passed!
|
| Reply(redis::Error("ERR " + s.Msg())); | ||
| Reply(redis::Error(s.Msg())); |
There was a problem hiding this comment.
IMHO it is a huge change. How about just change the redirect command.
There was a problem hiding this comment.
It can affect many many error messages. And some of these may be not expected to be changed.
There was a problem hiding this comment.
@PragmaTwice It only affects the cluster redirect error message which returns by CanExecByMySelf.
There was a problem hiding this comment.
I did checked the error returns in CanExecByMySelf, they both no need the ERR code
There was a problem hiding this comment.
Hmmm. Then no problem to me.
But for the general situation (we cannot control it for nested returned status right? it will cause huge maintanance effort), I recommend to add some custom error status and check it here.
I mean, we cannot check every commit every time whether every status need the ERR code, which is not a neat method to me.
There was a problem hiding this comment.
Yes, @enjoy-binbin also mentioned this before. It's unclear when the "ERR" prefix is needed.
Currently, Kvrocks cluster mode returns a MOVED/TRYAGAIN error message with the 'ERR' prefix, which will cause the Redis client can't recognize the redirection message. To make it compatible with the Redis client, we should remove this prefix. Before this patch, Kvrocks will return a MOVED error message: ``` > redis-cli -p 30001 -c set a 1 (error) ERR MOVED 15495 127.0.0.1:30005 ``` After applying this patch, it works well with redis-cli redirection: ``` > redis-cli -p 30001 -c set a 1 OK ```
Currently, Kvrocks cluster mode returns a MOVED/TRYAGAIN error message with the 'ERR' prefix, which will cause the Redis client can't recognize the redirection message. To make it compatible with the Redis client, we should remove this prefix.
Before this patch, Kvrocks will return a MOVED error message:
After applying this patch, it works well with redis-cli redirection:
This close #1922