chore(error): unify error by adding Redis error codes for Status #2362

git-hulk · 2024-06-11T14:44:22Z

This PR introduces new error codes to identify Redis error prefixes,
and forces using status to unify Redis errors. You MUST convert
it to Status to send a Redis error response.

git-hulk · 2024-06-13T14:25:49Z

By the way, I found some error messages aren't aligned with Redis, but I don't prefer fixing them in this PR. Will submit another PR to resolve this, as well as to reduce some of the redundant status codes.

src/cluster/cluster.cc

src/common/redis_error.h

PragmaTwice · 2024-06-14T03:49:38Z

There are several points for the current changes:

We need to also add ErrorKind to StatusOr, otherwise it's not consistent between Status and StatusOr, which violates our design of StatusOr.
ErrorKind::None is weird and seems useless. Removing useless state is always a good thing to prevent unnecessary complex.
ErrorKind is not always useful in Status, since not all sink of Status goes to RESP error.

So I suggest maybe we can just merge ErrorKind to Status::Code, which seems a solution that can solve these issues.

git-hulk · 2024-06-14T04:13:45Z

We need to also add ErrorKind to StatusOr, otherwise it's not consistent between Status and StatusOr, which violates our design of StatusOr.

Sure, would add this.

ErrorKind::None is weird and seems useless. Removing useless state is always a good thing to prevent unnecessary complex.

None is used to represent the error without the prefix.

ErrorKind is not always useful in Status, since not all sink of Status goes to RESP error.

Yes, not all status errors can be mapped to the RESP error, and I think it's still important to identify the status error via code. That's why I used the RedisError to identify them.

PragmaTwice · 2024-06-14T16:37:28Z

src/commands/cmd_key.cc

-                               std::to_string(SORT_LENGTH_LIMIT));
-        break;
+        return {Status::RedisErrorNoPrefix,
+                "The number of elements to be sorted exceeds SORT_LENGTH_LIMIT = " + std::to_string(SORT_LENGTH_LIMIT)};
      case Database::SortResult::DONE:


There's really no prefix? Or just because we forget to add the prefix?

Not sure about this. I found we have some places that didn't align with Redis, but I suggest revising and fixing them in another PR: #2362 (comment)

PragmaTwice · 2024-06-14T16:38:14Z

src/commands/cmd_replication.cc

@@ -230,7 +230,7 @@ class CommandFetchMeta : public Commander {
      std::string files;
      auto s = engine::Storage::ReplDataManager::GetFullReplDataInfo(srv->storage, &files);
      if (!s.IsOK()) {
-        s = util::SockSend(repl_fd, redis::Error("can't create db checkpoint"), bev);
+        s = util::SockSend(repl_fd, redis::Error({Status::RedisErrorNoPrefix, "can't create db checkpoint"}), bev);


If here it's because we forget to add the prefix, we should fix it.

Same as the above comment.

src/commands/cmd_server.cc

PragmaTwice · 2024-06-14T16:41:05Z

src/commands/error_constants.h

@@ -40,10 +40,10 @@ inline constexpr const char *errLimitOptionNotAllowed =
 inline constexpr const char *errZSetLTGTNX = "GT, LT, and/or NX options at the same time are not compatible";
 inline constexpr const char *errScoreIsNotValidFloat = "score is not a valid float";
 inline constexpr const char *errValueIsNotFloat = "value is not a valid float";
-inline constexpr const char *errNoMatchingScript = "NOSCRIPT No matching script. Please use EVAL";
+inline constexpr const char *errNoMatchingScript = "No matching script. Please use EVAL";
 inline constexpr const char *errUnknownOption = "unknown option";
 inline constexpr const char *errUnknownSubcommandOrWrongArguments = "Unknown subcommand or wrong number of arguments";
 inline constexpr const char *errWrongNumArguments = "ERR wrong number of arguments";


How about this one?

PragmaTwice · 2024-06-14T16:42:49Z

kvrocks/src/cluster/replication.cc

Line 1003 in 84a3559

return err == std::string(RESP_PREFIX_ERROR) + redis::errRestoringBackup;

Forget to change?

git-hulk · 2024-06-15T02:00:36Z

By the way, I found some error messages aren't aligned with Redis, but I don't prefer fixing them in this PR. Will submit another PR to resolve this, as well as to reduce some of the redundant status codes.

errRestoringBackup

Yes, good catch, fixed now.

src/cluster/replication.cc

PragmaTwice · 2024-06-15T02:24:55Z

src/server/redis_reply.cc

-std::string Error(const std::string &err) { return "-" + err + CRLF; }
+std::string Error(const Status &s) { return RESP_PREFIX_ERROR + StatusToRedisError(s); }
+
+std::string StatusToRedisError(const Status &s) {


Suggested change

std::string StatusToRedisError(const Status &s) {

std::string StatusToRedisErrorMsg(const Status &s) {

Maybe this name is better.

Then we can distinguish between redis error message and resp errors.

redis error message -> <PREFIX> <message..>

resp error -> - <redis error message> CRLF

sonarcloud · 2024-06-15T04:00:05Z

Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
53.7% Coverage on New Code
1.3% Duplication on New Code

See analysis details on SonarCloud

git-hulk force-pushed the unify-error-message branch 2 times, most recently from e647f02 to 694f099 Compare June 12, 2024 12:23

git-hulk changed the title ~~chore(error): explicitly assign the type in Redis error~~ chore(error): unify Redis error by explicitly using the error kind Jun 13, 2024

git-hulk marked this pull request as ready for review June 13, 2024 14:58

git-hulk requested review from PragmaTwice, caipengbo and torwig June 13, 2024 16:08

torwig reviewed Jun 13, 2024

View reviewed changes

src/cluster/cluster.cc Outdated Show resolved Hide resolved

caipengbo reviewed Jun 14, 2024

View reviewed changes

src/common/redis_error.h Outdated Show resolved Hide resolved

git-hulk added 8 commits June 14, 2024 22:10

chore(error): explicitly assign the type in Redis error

175335a

Allow to use Status in Redis::Error

bbdaf19

Rename ErrorType to ErrorKind

ddd125b

Fix build error

6c1f173

Use the error status to replace the error output

9103862

Use ErrorKind::* to replace the error string

fb342c9

Fix wrong message in Lua

4d1020e

Move the error kind inside status

045b087

git-hulk force-pushed the unify-error-message branch from e618aee to 045b087 Compare June 14, 2024 14:10

git-hulk changed the title ~~chore(error): unify Redis error by explicitly using the error kind~~ chore(error): unify error by adding Redis error codes for Status Jun 14, 2024

git-hulk requested review from torwig and caipengbo June 14, 2024 14:28

PragmaTwice reviewed Jun 14, 2024

View reviewed changes

src/commands/cmd_server.cc Show resolved Hide resolved

PragmaTwice reviewed Jun 14, 2024

View reviewed changes

Fix error message

3684950

PragmaTwice reviewed Jun 15, 2024

View reviewed changes

src/cluster/replication.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed Jun 15, 2024

View reviewed changes

git-hulk added 2 commits June 15, 2024 10:28

Fix wrong error message

60cd76b

Rename

89a61d8

PragmaTwice approved these changes Jun 15, 2024

View reviewed changes

Merge branch 'unstable' into unify-error-message

ab7a8b9

git-hulk merged commit 8f1d2ad into apache:unstable Jun 15, 2024
32 checks passed

git-hulk mentioned this pull request Jun 15, 2024

Improve and unify the RESP error message #2335

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(error): unify error by adding Redis error codes for Status #2362

chore(error): unify error by adding Redis error codes for Status #2362

git-hulk commented Jun 11, 2024 •

edited

Loading

git-hulk commented Jun 13, 2024

PragmaTwice commented Jun 14, 2024 •

edited

Loading

git-hulk commented Jun 14, 2024

PragmaTwice Jun 14, 2024

git-hulk Jun 15, 2024

PragmaTwice Jun 14, 2024

git-hulk Jun 15, 2024

PragmaTwice Jun 14, 2024

PragmaTwice commented Jun 14, 2024

git-hulk commented Jun 15, 2024

PragmaTwice Jun 15, 2024

PragmaTwice Jun 15, 2024 •

edited

Loading

git-hulk Jun 15, 2024

sonarcloud bot commented Jun 15, 2024

	std::string StatusToRedisError(const Status &s) {
	std::string StatusToRedisErrorMsg(const Status &s) {

chore(error): unify error by adding Redis error codes for Status #2362

chore(error): unify error by adding Redis error codes for Status #2362

Conversation

git-hulk commented Jun 11, 2024 • edited Loading

git-hulk commented Jun 13, 2024

PragmaTwice commented Jun 14, 2024 • edited Loading

git-hulk commented Jun 14, 2024

PragmaTwice Jun 14, 2024

Choose a reason for hiding this comment

git-hulk Jun 15, 2024

Choose a reason for hiding this comment

PragmaTwice Jun 14, 2024

Choose a reason for hiding this comment

git-hulk Jun 15, 2024

Choose a reason for hiding this comment

PragmaTwice Jun 14, 2024

Choose a reason for hiding this comment

PragmaTwice commented Jun 14, 2024

git-hulk commented Jun 15, 2024

PragmaTwice Jun 15, 2024

Choose a reason for hiding this comment

PragmaTwice Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

git-hulk Jun 15, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Jun 15, 2024

Quality Gate passed

git-hulk commented Jun 11, 2024 •

edited

Loading

PragmaTwice commented Jun 14, 2024 •

edited

Loading

PragmaTwice Jun 15, 2024 •

edited

Loading