apache / kvrocks

Apache Kvrocks is a distributed key value NoSQL database that uses RocksDB as storage engine and is compatible with Redis protocol.
https://kvrocks.apache.org/
Apache License 2.0
3.47k stars 452 forks source link

Improve and unify the RESP error message #2335

Closed git-hulk closed 3 months ago

git-hulk commented 3 months ago

Search before asking

Motivation

As mentioned in the comment: https://github.com/apache/kvrocks/blob/unstable/tests/gocase/util/server.go#L142, we should take care of different RESP error kinds to avoid introducing the protocol compatibility issues.

Solution

Use an unify function to return the RESP error message, like the following:

const std::string redis::Error(ErrorKind kind, const std::string &message) {
  switch(kind) {
    case ErrorKind::Loading: 
    case ErrorKind::NoProto:
    ...
    default:
  }
}

Are you willing to submit a PR?

PragmaTwice commented 3 months ago

The problem is that we add ERR prefix for all msg returned by Status.

One solution is to add a new status code for no ERR prefix. (Or, add many status code for LOADING, NOPROTO...)

git-hulk commented 3 months ago

The problem is that we add ERR prefix for all msg returned by Status.

Yes

One solution is to add a new status code for no ERR prefix. (Or, add many status code for LOADING, NOPROTO...)

This also sounds good

git-hulk commented 3 months ago

closes by #2362