This is an automated email from the ASF dual-hosted git repository.
binbin pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new f9fc3402 Update error message and add the ERR error code (#1597)
f9fc3402 is described below
commit f9fc3402d8a6d012d92ca90ca9854654e509bcf1
Author: Binbin <[email protected]>
AuthorDate: Sat Jul 22 16:57:47 2023 +0800
Update error message and add the ERR error code (#1597)
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)
---
src/commands/cmd_cluster.cc | 32 ++++++++++------------
src/commands/cmd_key.cc | 2 +-
src/commands/cmd_server.cc | 62 ++++++++++++++++--------------------------
src/commands/cmd_txn.cc | 12 +++-----
src/server/redis_connection.cc | 6 ++--
5 files changed, 46 insertions(+), 68 deletions(-)
diff --git a/src/commands/cmd_cluster.cc b/src/commands/cmd_cluster.cc
index fc55f914..c4026879 100644
--- a/src/commands/cmd_cluster.cc
+++ b/src/commands/cmd_cluster.cc
@@ -52,13 +52,11 @@ class CommandCluster : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (!svr->GetConfig()->cluster_enabled) {
- *output = redis::Error("Cluster mode is not enabled");
- return Status::OK();
+ return {Status::RedisExecErr, "Cluster mode is not enabled"};
}
if (!conn->IsAdmin()) {
- *output = redis::Error(errAdministorPermissionRequired);
- return Status::OK();
+ return {Status::RedisExecErr, errAdministorPermissionRequired};
}
if (subcommand_ == "keyslot") {
@@ -81,7 +79,7 @@ class CommandCluster : public Commander {
}
}
} else {
- *output = redis::Error(s.Msg());
+ return {Status::RedisExecErr, s.Msg()};
}
} else if (subcommand_ == "nodes") {
std::string nodes_desc;
@@ -89,7 +87,7 @@ class CommandCluster : public Commander {
if (s.IsOK()) {
*output = redis::BulkString(nodes_desc);
} else {
- *output = redis::Error(s.Msg());
+ return {Status::RedisExecErr, s.Msg()};
}
} else if (subcommand_ == "info") {
std::string cluster_info;
@@ -97,17 +95,17 @@ class CommandCluster : public Commander {
if (s.IsOK()) {
*output = redis::BulkString(cluster_info);
} else {
- *output = redis::Error(s.Msg());
+ return {Status::RedisExecErr, s.Msg()};
}
} else if (subcommand_ == "import") {
Status s = svr->cluster->ImportSlot(conn, static_cast<int>(slot_),
state_);
if (s.IsOK()) {
*output = redis::SimpleString("OK");
} else {
- *output = redis::Error(s.Msg());
+ return {Status::RedisExecErr, s.Msg()};
}
} else {
- *output = redis::Error("Invalid cluster command options");
+ return {Status::RedisExecErr, "Invalid cluster command options"};
}
return Status::OK();
}
@@ -214,13 +212,11 @@ class CommandClusterX : public Commander {
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (!svr->GetConfig()->cluster_enabled) {
- *output = redis::Error("Cluster mode is not enabled");
- return Status::OK();
+ return {Status::RedisExecErr, "Cluster mode is not enabled"};
}
if (!conn->IsAdmin()) {
- *output = redis::Error(errAdministorPermissionRequired);
- return Status::OK();
+ return {Status::RedisExecErr, errAdministorPermissionRequired};
}
bool need_persist_nodes_info = false;
@@ -230,7 +226,7 @@ class CommandClusterX : public Commander {
need_persist_nodes_info = true;
*output = redis::SimpleString("OK");
} else {
- *output = redis::Error(s.Msg());
+ return {Status::RedisExecErr, s.Msg()};
}
} else if (subcommand_ == "setnodeid") {
Status s = svr->cluster->SetNodeId(args_[2]);
@@ -238,7 +234,7 @@ class CommandClusterX : public Commander {
need_persist_nodes_info = true;
*output = redis::SimpleString("OK");
} else {
- *output = redis::Error(s.Msg());
+ return {Status::RedisExecErr, s.Msg()};
}
} else if (subcommand_ == "setslot") {
Status s = svr->cluster->SetSlotRanges(slot_ranges_, args_[4],
set_version_);
@@ -246,7 +242,7 @@ class CommandClusterX : public Commander {
need_persist_nodes_info = true;
*output = redis::SimpleString("OK");
} else {
- *output = redis::Error(s.Msg());
+ return {Status::RedisExecErr, s.Msg()};
}
} else if (subcommand_ == "version") {
int64_t v = svr->cluster->GetVersion();
@@ -263,10 +259,10 @@ class CommandClusterX : public Commander {
}
*output = redis::SimpleString("OK");
} else {
- *output = redis::Error(s.Msg());
+ return {Status::RedisExecErr, s.Msg()};
}
} else {
- *output = redis::Error("Invalid cluster command options");
+ return {Status::RedisExecErr, "Invalid cluster command options"};
}
if (need_persist_nodes_info &&
svr->GetConfig()->persist_cluster_nodes_enabled) {
return svr->cluster->DumpClusterNodes(svr->GetConfig()->NodesFilePath());
diff --git a/src/commands/cmd_key.cc b/src/commands/cmd_key.cc
index 1f2e77ae..703a0cdf 100644
--- a/src/commands/cmd_key.cc
+++ b/src/commands/cmd_key.cc
@@ -61,7 +61,7 @@ class CommandObject : public Commander {
output->append(redis::BulkString(info));
}
} else {
- *output = redis::Error("object subcommand must be dump");
+ return {Status::RedisExecErr, "object subcommand must be dump"};
}
return Status::OK();
}
diff --git a/src/commands/cmd_server.cc b/src/commands/cmd_server.cc
index e6f6f33b..d64a7897 100644
--- a/src/commands/cmd_server.cc
+++ b/src/commands/cmd_server.cc
@@ -69,11 +69,9 @@ class CommandAuth : public Commander {
*output = redis::SimpleString("OK");
break;
case AuthResult::INVALID_PASSWORD:
- *output = redis::Error("ERR invalid password");
- break;
+ return {Status::RedisExecErr, "invalid password"};
case AuthResult::NO_REQUIRE_PASS:
- *output = redis::Error("ERR Client sent AUTH, but no password is set");
- break;
+ return {Status::RedisExecErr, "Client sent AUTH, but no password is
set"};
}
return Status::OK();
}
@@ -83,8 +81,7 @@ class CommandNamespace : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (!conn->IsAdmin()) {
- *output = redis::Error(errAdministorPermissionRequired);
- return Status::OK();
+ return {Status::RedisExecErr, errAdministorPermissionRequired};
}
Config *config = svr->GetConfig();
@@ -111,20 +108,20 @@ class CommandNamespace : public Commander {
}
} else if (args_.size() == 4 && sub_command == "set") {
Status s = config->SetNamespace(args_[2], args_[3]);
- *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(s.Msg());
+ *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error("ERR " +
s.Msg());
LOG(WARNING) << "Updated namespace: " << args_[2] << " with token: " <<
args_[3] << ", addr: " << conn->GetAddr()
<< ", result: " << s.Msg();
} else if (args_.size() == 4 && sub_command == "add") {
Status s = config->AddNamespace(args_[2], args_[3]);
- *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(s.Msg());
+ *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error("ERR " +
s.Msg());
LOG(WARNING) << "New namespace: " << args_[2] << " with token: " <<
args_[3] << ", addr: " << conn->GetAddr()
<< ", result: " << s.Msg();
} else if (args_.size() == 3 && sub_command == "del") {
Status s = config->DelNamespace(args_[2]);
- *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(s.Msg());
+ *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error("ERR " +
s.Msg());
LOG(WARNING) << "Deleted namespace: " << args_[2] << ", addr: " <<
conn->GetAddr() << ", result: " << s.Msg();
} else {
- *output = redis::Error("NAMESPACE subcommand must be one of GET, SET,
DEL, ADD");
+ return {Status::RedisExecErr, "NAMESPACE subcommand must be one of GET,
SET, DEL, ADD"};
}
return Status::OK();
}
@@ -140,8 +137,7 @@ class CommandKeys : public Commander {
redis.Keys(std::string(), &keys);
} else {
if (prefix[prefix.size() - 1] != '*') {
- *output = redis::Error("ERR only keys prefix match was supported");
- return Status::OK();
+ return {Status::RedisExecErr, "only keys prefix match was supported"};
}
redis.Keys(prefix.substr(0, prefix.size() - 1), &keys);
@@ -176,8 +172,7 @@ class CommandFlushAll : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (!conn->IsAdmin()) {
- *output = redis::Error(errAdministorPermissionRequired);
- return Status::OK();
+ return {Status::RedisExecErr, errAdministorPermissionRequired};
}
if (svr->GetConfig()->cluster_enabled) {
@@ -225,16 +220,14 @@ class CommandConfig : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (!conn->IsAdmin()) {
- *output = redis::Error(errAdministorPermissionRequired);
- return Status::OK();
+ return {Status::RedisExecErr, errAdministorPermissionRequired};
}
Config *config = svr->GetConfig();
std::string sub_command = util::ToLower(args_[1]);
if ((sub_command == "rewrite" && args_.size() != 2) || (sub_command ==
"get" && args_.size() != 3) ||
(sub_command == "set" && args_.size() != 4)) {
- *output = redis::Error(errWrongNumOfArguments);
- return Status::OK();
+ return {Status::RedisExecErr, errWrongNumOfArguments};
}
if (args_.size() == 2 && sub_command == "rewrite") {
@@ -250,12 +243,12 @@ class CommandConfig : public Commander {
} else if (args_.size() == 4 && sub_command == "set") {
Status s = config->Set(svr, args_[2], args_[3]);
if (!s.IsOK()) {
- *output = redis::Error("CONFIG SET '" + args_[2] + "' error: " +
s.Msg());
+ return {Status::RedisExecErr, "CONFIG SET '" + args_[2] + "' error: "
+ s.Msg()};
} else {
*output = redis::SimpleString("OK");
}
} else {
- *output = redis::Error("CONFIG subcommand must be one of GET, SET,
REWRITE");
+ return {Status::RedisExecErr, "CONFIG subcommand must be one of GET,
SET, REWRITE"};
}
return Status::OK();
}
@@ -330,10 +323,10 @@ class CommandDBSize : public Commander {
if (s.IsOK()) {
*output = redis::SimpleString("OK");
} else {
- *output = redis::Error(s.Msg());
+ return {Status::RedisExecErr, s.Msg()};
}
} else {
- *output = redis::Error("DBSIZE subcommand only supports scan");
+ return {Status::RedisExecErr, "DBSIZE subcommand only supports scan"};
}
return Status::OK();
}
@@ -520,7 +513,7 @@ class CommandClient : public Commander {
*output = redis::Integer(killed);
} else {
if (killed == 0)
- *output = redis::Error("No such client");
+ return {Status::RedisExecErr, "No such client"};
else
*output = redis::SimpleString("OK");
}
@@ -553,8 +546,7 @@ class CommandShutdown : public Commander {
public:
Status Execute(Server *srv, Connection *conn, std::string *output) override {
if (!conn->IsAdmin()) {
- *output = redis::Error(errAdministorPermissionRequired);
- return Status::OK();
+ return {Status::RedisExecErr, errAdministorPermissionRequired};
}
if (!srv->IsStopped()) {
@@ -612,8 +604,7 @@ class CommandCommand : public Commander {
std::string sub_command = util::ToLower(args_[1]);
if ((sub_command == "count" && args_.size() != 2) || (sub_command ==
"getkeys" && args_.size() < 3) ||
(sub_command == "info" && args_.size() < 3)) {
- *output = redis::Error(errWrongNumOfArguments);
- return Status::OK();
+ return {Status::RedisExecErr, errWrongNumOfArguments};
}
if (sub_command == "count") {
@@ -626,8 +617,7 @@ class CommandCommand : public Commander {
if (!s.IsOK()) return s;
if (keys_indexes.size() == 0) {
- *output = redis::Error("Invalid arguments specified for command");
- return Status::OK();
+ return {Status::RedisExecErr, "Invalid arguments specified for
command"};
}
std::vector<std::string> keys;
@@ -637,7 +627,7 @@ class CommandCommand : public Commander {
}
*output = redis::MultiBulkString(keys);
} else {
- *output = redis::Error("Command subcommand must be one of COUNT,
GETKEYS, INFO");
+ return {Status::RedisExecErr, "Command subcommand must be one of
COUNT, GETKEYS, INFO"};
}
}
return Status::OK();
@@ -720,8 +710,7 @@ class CommandHello final : public Commander {
conn->SetName(name);
next_arg += 1;
} else {
- *output = redis::Error("Syntax error in HELLO option " + opt);
- return Status::OK();
+ return {Status::RedisExecErr, "Syntax error in HELLO option " + opt};
}
}
@@ -846,8 +835,7 @@ class CommandBGSave : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (!conn->IsAdmin()) {
- *output = redis::Error(errAdministorPermissionRequired);
- return Status::OK();
+ return {Status::RedisExecErr, errAdministorPermissionRequired};
}
Status s = svr->AsyncBgSaveDB();
@@ -863,8 +851,7 @@ class CommandFlushBackup : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (!conn->IsAdmin()) {
- *output = redis::Error(errAdministorPermissionRequired);
- return Status::OK();
+ return {Status::RedisExecErr, errAdministorPermissionRequired};
}
Status s = svr->AsyncPurgeOldBackups(0, 0);
@@ -923,8 +910,7 @@ class CommandSlaveOf : public Commander {
}
if (!conn->IsAdmin()) {
- *output = redis::Error(errAdministorPermissionRequired);
- return Status::OK();
+ return {Status::RedisExecErr, errAdministorPermissionRequired};
}
if (host_.empty()) {
diff --git a/src/commands/cmd_txn.cc b/src/commands/cmd_txn.cc
index 8dbaa24d..3138c364 100644
--- a/src/commands/cmd_txn.cc
+++ b/src/commands/cmd_txn.cc
@@ -31,8 +31,7 @@ class CommandMulti : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (conn->IsFlagEnabled(Connection::kMultiExec)) {
- *output = redis::Error("ERR MULTI calls can not be nested");
- return Status::OK();
+ return {Status::RedisExecErr, "MULTI calls can not be nested"};
}
conn->ResetMultiExec();
// Client starts into MULTI-EXEC
@@ -46,8 +45,7 @@ class CommandDiscard : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (!conn->IsFlagEnabled(Connection::kMultiExec)) {
- *output = redis::Error("ERR DISCARD without MULTI");
- return Status::OK();
+ return {Status::RedisExecErr, "DISCARD without MULTI"};
}
auto reset_watch = MakeScopeExit([svr, conn] {
svr->ResetWatchedKeys(conn); });
@@ -63,8 +61,7 @@ class CommandExec : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (!conn->IsFlagEnabled(Connection::kMultiExec)) {
- *output = redis::Error("ERR EXEC without MULTI");
- return Status::OK();
+ return {Status::RedisExecErr, "EXEC without MULTI"};
}
auto reset_watch = MakeScopeExit([svr, conn] {
svr->ResetWatchedKeys(conn); });
@@ -98,8 +95,7 @@ class CommandWatch : public Commander {
public:
Status Execute(Server *svr, Connection *conn, std::string *output) override {
if (conn->IsFlagEnabled(Connection::kMultiExec)) {
- *output = redis::Error("ERR WATCH inside MULTI is not allowed");
- return Status::OK();
+ return {Status::RedisExecErr, "WATCH inside MULTI is not allowed"};
}
svr->WatchKey(conn, std::vector<std::string>(args_.begin() + 1,
args_.end()));
diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc
index 819c8006..6540b5b3 100644
--- a/src/server/redis_connection.cc
+++ b/src/server/redis_connection.cc
@@ -80,7 +80,7 @@ void Connection::OnRead(struct bufferevent *bev) {
auto s = req_.Tokenize(Input());
if (!s.IsOK()) {
EnableFlag(redis::Connection::kCloseAfterReply);
- Reply(redis::Error(s.Msg()));
+ Reply(redis::Error("ERR " + s.Msg()));
LOG(INFO) << "[connection] Failed to tokenize the request. Error: " <<
s.Msg();
return;
}
@@ -371,7 +371,7 @@ void Connection::ExecuteCommands(std::deque<CommandTokens>
*to_process_cmds) {
}
if (IsFlagEnabled(Connection::kMultiExec) && attributes->IsNoMulti()) {
- std::string no_multi_err = "Err Can't execute " + attributes->name + "
in MULTI";
+ std::string no_multi_err = "ERR Can't execute " + attributes->name + "
in MULTI";
Reply(redis::Error(no_multi_err));
multi_error_ = true;
continue;
@@ -381,7 +381,7 @@ void Connection::ExecuteCommands(std::deque<CommandTokens>
*to_process_cmds) {
s = svr_->cluster->CanExecByMySelf(attributes, cmd_tokens, this);
if (!s.IsOK()) {
if (IsFlagEnabled(Connection::kMultiExec)) multi_error_ = true;
- Reply(redis::Error(s.Msg()));
+ Reply(redis::Error("ERR " + s.Msg()));
continue;
}
}
