From 175335abc6253f79fd97fbca137b65c90ffd691a Mon Sep 17 00:00:00 2001 From: git-hulk Date: Tue, 11 Jun 2024 22:42:05 +0800 Subject: [PATCH 01/11] chore(error): explicitly assign the type in Redis error --- src/cluster/sync_migrate_context.cc | 2 +- src/commands/cmd_bloom_filter.cc | 8 +++--- src/commands/cmd_key.cc | 10 ++++---- src/commands/cmd_list.cc | 6 ++--- src/commands/cmd_replication.cc | 4 +-- src/commands/cmd_script.cc | 2 +- src/commands/cmd_server.cc | 14 +++++------ src/commands/cmd_stream.cc | 4 +-- src/commands/cmd_txn.cc | 2 +- src/commands/cmd_zset.cc | 4 +-- src/commands/error_constants.h | 4 +-- src/server/redis_connection.cc | 28 ++++++++++----------- src/server/redis_reply.cc | 39 ++++++++++++++++++++++++++++- src/server/redis_reply.h | 15 ++++++++++- src/server/worker.cc | 4 +-- src/storage/scripting.cc | 8 +++--- 16 files changed, 102 insertions(+), 52 deletions(-) diff --git a/src/cluster/sync_migrate_context.cc b/src/cluster/sync_migrate_context.cc index 3ba2806ca45..c530a323889 100644 --- a/src/cluster/sync_migrate_context.cc +++ b/src/cluster/sync_migrate_context.cc @@ -68,7 +68,7 @@ void SyncMigrateContext::OnWrite(bufferevent *bev) { if (migrate_result_) { conn_->Reply(redis::SimpleString("OK")); } else { - conn_->Reply(redis::Error("ERR " + migrate_result_.Msg())); + conn_->Reply(redis::Error(redis::ErrorType::Err, migrate_result_.Msg())); } timer_.reset(); diff --git a/src/commands/cmd_bloom_filter.cc b/src/commands/cmd_bloom_filter.cc index f33979e3d50..bd229dffd93 100644 --- a/src/commands/cmd_bloom_filter.cc +++ b/src/commands/cmd_bloom_filter.cc @@ -33,7 +33,7 @@ constexpr const char *errInvalidErrorRate = "error rate should be between 0 and constexpr const char *errInvalidCapacity = "capacity should be larger than 0"; constexpr const char *errInvalidExpansion = "expansion should be greater or equal to 1"; constexpr const char *errNonscalingButExpand = "nonscaling filters cannot expand"; -constexpr const char *errFilterFull = "ERR nonscaling filter is full"; +constexpr const char *errFilterFull = "nonscaling filter is full"; } // namespace namespace redis { @@ -119,7 +119,7 @@ class CommandBFAdd : public Commander { *output = redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output = redis::Error(errFilterFull); + *output = redis::Error(ErrorType::Err, errFilterFull); break; } return Status::OK(); @@ -152,7 +152,7 @@ class CommandBFMAdd : public Commander { *output += redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output += redis::Error(errFilterFull); + *output += redis::Error(ErrorType::Err, errFilterFull); break; } } @@ -248,7 +248,7 @@ class CommandBFInsert : public Commander { *output += redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output += redis::Error(errFilterFull); + *output += redis::Error(ErrorType::Err, errFilterFull); break; } } diff --git a/src/commands/cmd_key.cc b/src/commands/cmd_key.cc index 24d8fe29c4d..63a1ef90130 100644 --- a/src/commands/cmd_key.cc +++ b/src/commands/cmd_key.cc @@ -480,7 +480,7 @@ class CommandSort : public Commander { } if (type != RedisType::kRedisList && type != RedisType::kRedisSet && type != RedisType::kRedisZSet) { - *output = Error("WRONGTYPE Operation against a key holding the wrong kind of value"); + *output = Error(ErrorType::WrongType, "Operation against a key holding the wrong kind of value"); return Status::OK(); } @@ -508,14 +508,14 @@ class CommandSort : public Commander { switch (res) { case Database::SortResult::UNKNOWN_TYPE: - *output = redis::Error("Unknown Type"); + *output = redis::Error(ErrorType::Err, "Unknown Type"); break; case Database::SortResult::DOUBLE_CONVERT_ERROR: - *output = redis::Error("One or more scores can't be converted into double"); + *output = redis::Error(ErrorType::None, "One or more scores can't be converted into double"); break; case Database::SortResult::LIMIT_EXCEEDED: - *output = redis::Error("The number of elements to be sorted exceeds SORT_LENGTH_LIMIT = " + - std::to_string(SORT_LENGTH_LIMIT)); + *output = redis::Error(ErrorType::None, "The number of elements to be sorted exceeds SORT_LENGTH_LIMIT = " + + std::to_string(SORT_LENGTH_LIMIT)); break; case Database::SortResult::DONE: if (sort_argument_.storekey.empty()) { diff --git a/src/commands/cmd_list.cc b/src/commands/cmd_list.cc index f354d64cc4b..a9ae36fa6c0 100644 --- a/src/commands/cmd_list.cc +++ b/src/commands/cmd_list.cc @@ -304,7 +304,7 @@ class CommandBPop : public BlockingCommander { conn_->Reply(conn_->MultiBulkString({*last_key_ptr, std::move(elem)})); } } else if (!s.IsNotFound()) { - conn_->Reply(redis::Error("ERR " + s.ToString())); + conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); } return s; @@ -414,7 +414,7 @@ class CommandBLMPop : public BlockingCommander { conn_->Reply(redis::Array({redis::BulkString(chosen_key), std::move(elems_bulk)})); } } else if (!s.IsNotFound()) { - conn_->Reply(redis::Error("ERR " + s.ToString())); + conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); } return s; @@ -757,7 +757,7 @@ class CommandBLMove : public BlockingCommander { std::string elem; auto s = list_db.LMove(args_[1], args_[2], src_left_, dst_left_, &elem); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error("ERR " + s.ToString())); + conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); return true; } diff --git a/src/commands/cmd_replication.cc b/src/commands/cmd_replication.cc index 5ec6faa03b6..0c1cf8d2cc8 100644 --- a/src/commands/cmd_replication.cc +++ b/src/commands/cmd_replication.cc @@ -102,7 +102,7 @@ class CommandPSync : public Commander { srv->stats.IncrPSyncOKCount(); s = srv->AddSlave(conn, next_repl_seq_); if (!s.IsOK()) { - std::string err = redis::Error(s.Msg()); + std::string err = redis::Error(ErrorType::None, s.Msg()); s = util::SockSend(conn->GetFD(), err, conn->GetBufferEvent()); if (!s.IsOK()) { LOG(WARNING) << "failed to send error message to the replica: " << s.Msg(); @@ -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(ErrorType::None, "can't create db checkpoint"), bev); if (!s.IsOK()) { LOG(WARNING) << "[replication] Failed to send error response: " << s.Msg(); } diff --git a/src/commands/cmd_script.cc b/src/commands/cmd_script.cc index 34a7183756d..49d85f274dc 100644 --- a/src/commands/cmd_script.cc +++ b/src/commands/cmd_script.cc @@ -31,7 +31,7 @@ class CommandEvalImpl : public Commander { public: Status Execute(Server *srv, Connection *conn, std::string *output) override { if (evalsha && args_[1].size() != 40) { - *output = redis::Error(errNoMatchingScript); + *output = redis::Error(ErrorType::NoScript, errNoMatchingScript); return Status::OK(); } diff --git a/src/commands/cmd_server.cc b/src/commands/cmd_server.cc index 114eee15f27..5b30ff90a0a 100644 --- a/src/commands/cmd_server.cc +++ b/src/commands/cmd_server.cc @@ -95,17 +95,17 @@ class CommandNamespace : public Commander { } } else if (args_.size() == 4 && sub_command == "set") { Status s = srv->GetNamespace()->Set(args_[2], args_[3]); - *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error("ERR " + s.Msg()); + *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(ErrorType::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 = srv->GetNamespace()->Add(args_[2], args_[3]); - *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error("ERR " + s.Msg()); + *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(ErrorType::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 = srv->GetNamespace()->Del(args_[2]); - *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error("ERR " + s.Msg()); + *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(ErrorType::Err, s.Msg()); LOG(WARNING) << "Deleted namespace: " << args_[2] << ", addr: " << conn->GetAddr() << ", result: " << s.Msg(); } else { return {Status::RedisExecErr, "NAMESPACE subcommand must be one of GET, SET, DEL, ADD"}; @@ -630,9 +630,9 @@ class CommandDebug : public Commander { } else if (protocol_type_ == "verbatim") { // verbatim string *output = conn->VerbatimString("txt", "verbatim string"); } else { - *output = redis::Error( - "Wrong protocol type name. Please use one of the following: " - "string|integer|double|array|set|bignum|true|false|null|attrib|verbatim"); + *output = redis::Error(ErrorType::None, + "Wrong protocol type name. Please use one of the following: " + "string|integer|double|array|set|bignum|true|false|null|attrib|verbatim"); } } else if (subcommand_ == "dbsize-limit") { srv->storage->SetDBSizeLimit(dbsize_limit_); @@ -741,7 +741,7 @@ class CommandHello final : public Commander { // kvrocks only supports REPL2 by now, but for supporting some // `hello 3`, it will not report error when using 3. if (protocol < 2 || protocol > 3) { - conn->Reply(redis::Error("NOPROTO unsupported protocol version")); + conn->Reply(redis::Error(ErrorType::NoProto, "unsupported protocol version")); return Status::OK(); } } diff --git a/src/commands/cmd_stream.cc b/src/commands/cmd_stream.cc index 7b3bf54e236..944feee36f8 100644 --- a/src/commands/cmd_stream.cc +++ b/src/commands/cmd_stream.cc @@ -1130,7 +1130,7 @@ class CommandXRead : public Commander, std::vector result; auto s = stream_db.Range(streams_[i], options, &result); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error("ERR " + s.ToString())); + conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); return; } @@ -1423,7 +1423,7 @@ class CommandXReadGroup : public Commander, auto s = stream_db.RangeWithPending(streams_[i], options, &result, group_name_, consumer_name_, noack_, latest_marks_[i]); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error("ERR " + s.ToString())); + conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); return; } diff --git a/src/commands/cmd_txn.cc b/src/commands/cmd_txn.cc index fa1a47aadae..a7c145ce192 100644 --- a/src/commands/cmd_txn.cc +++ b/src/commands/cmd_txn.cc @@ -68,7 +68,7 @@ class CommandExec : public Commander { auto reset_multiexec = MakeScopeExit([conn] { conn->ResetMultiExec(); }); if (conn->IsMultiError()) { - *output = redis::Error("EXECABORT Transaction discarded"); + *output = redis::Error(ErrorType::ExecAbort, "Transaction discarded"); return Status::OK(); } diff --git a/src/commands/cmd_zset.cc b/src/commands/cmd_zset.cc index c32c976a431..3e243afe352 100644 --- a/src/commands/cmd_zset.cc +++ b/src/commands/cmd_zset.cc @@ -369,7 +369,7 @@ class CommandBZPop : public BlockingCommander { redis::ZSet zset_db(srv_->storage, conn_->GetNamespace()); auto s = PopFromMultipleZsets(&zset_db, keys_, min_, 1, &user_key, &member_scores); if (!s.ok()) { - conn_->Reply(redis::Error("ERR " + s.ToString())); + conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); return true; } @@ -548,7 +548,7 @@ class CommandBZMPop : public BlockingCommander { redis::ZSet zset_db(srv_->storage, conn_->GetNamespace()); auto s = PopFromMultipleZsets(&zset_db, keys_, flag_ == ZSET_MIN, count_, &user_key, &member_scores); if (!s.ok()) { - conn_->Reply(redis::Error("ERR " + s.ToString())); + conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); return true; } diff --git a/src/commands/error_constants.h b/src/commands/error_constants.h index 2074a705531..08c107ad6f6 100644 --- a/src/commands/error_constants.h +++ b/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"; -inline constexpr const char *errRestoringBackup = "LOADING kvrocks is restoring the db from backup"; +inline constexpr const char *errRestoringBackup = "kvrocks is restoring the db from backup"; } // namespace redis diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc index f3960a66001..38dfa4b5b81 100644 --- a/src/server/redis_connection.cc +++ b/src/server/redis_connection.cc @@ -87,7 +87,7 @@ void Connection::OnRead(struct bufferevent *bev) { auto s = req_.Tokenize(Input()); if (!s.IsOK()) { EnableFlag(redis::Connection::kCloseAfterReply); - Reply(redis::Error("ERR " + s.Msg())); + Reply(redis::Error(ErrorType::Err, s.Msg())); LOG(INFO) << "[connection] Failed to tokenize the request. Error: " << s.Msg(); return; } @@ -432,7 +432,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { auto cmd_s = Server::LookupAndCreateCommand(cmd_tokens.front()); if (!cmd_s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error("ERR unknown command " + cmd_tokens.front())); + Reply(redis::Error(ErrorType::Err, "unknown command " + cmd_tokens.front())); continue; } auto current_cmd = std::move(*cmd_s); @@ -444,7 +444,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { if (GetNamespace().empty()) { if (!password.empty()) { if (cmd_name != "auth" && cmd_name != "hello") { - Reply(redis::Error("NOAUTH Authentication required.")); + Reply(redis::Error(ErrorType::NoAuth, "Authentication required.")); continue; } } else { @@ -477,7 +477,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (srv_->IsLoading() && !(cmd_flags & kCmdLoading)) { - Reply(redis::Error(errRestoringBackup)); + Reply(redis::Error(ErrorType::Loading, errRestoringBackup)); if (is_multi_exec) multi_error_ = true; continue; } @@ -485,7 +485,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { int tokens = static_cast(cmd_tokens.size()); if (!attributes->CheckArity(tokens)) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error("ERR wrong number of arguments")); + Reply(redis::Error(ErrorType::Err, "wrong number of arguments")); continue; } @@ -493,12 +493,12 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { auto s = current_cmd->Parse(); if (!s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error("ERR " + s.Msg())); + Reply(redis::Error(ErrorType::Err, s.Msg())); continue; } if (is_multi_exec && (cmd_flags & kCmdNoMulti)) { - Reply(redis::Error("ERR Can't execute " + cmd_name + " in MULTI")); + Reply(redis::Error(ErrorType::Err, "Can't execute " + cmd_name + " in MULTI")); multi_error_ = true; continue; } @@ -507,7 +507,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { s = srv_->cluster->CanExecByMySelf(attributes, cmd_tokens, this); if (!s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error(s.Msg())); + Reply(redis::Error(ErrorType::None, s.Msg())); continue; } } @@ -525,20 +525,20 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (config->slave_readonly && srv_->IsSlave() && (cmd_flags & kCmdWrite)) { - Reply(redis::Error("READONLY You can't write against a read only slave.")); + Reply(redis::Error(ErrorType::Readonly, "You can't write against a read only slave.")); continue; } if ((cmd_flags & kCmdWrite) && !(cmd_flags & kCmdNoDBSizeCheck) && srv_->storage->ReachedDBSizeLimit()) { - Reply(redis::Error("ERR write command not allowed when reached max-db-size.")); + Reply(redis::Error(ErrorType::Err, "write command not allowed when reached max-db-size.")); continue; } if (!config->slave_serve_stale_data && srv_->IsSlave() && cmd_name != "info" && cmd_name != "slaveof" && srv_->GetReplicationState() != kReplConnected) { - Reply( - redis::Error("MASTERDOWN Link with MASTER is down " - "and slave-serve-stale-data is set to 'no'.")); + Reply(redis::Error(ErrorType::MasterDown, + "Link with MASTER is down " + "and slave-serve-stale-data is set to 'no'.")); continue; } @@ -585,7 +585,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { // Reply for MULTI if (!s.IsOK()) { - Reply(redis::Error("ERR " + s.Msg())); + Reply(redis::Error(ErrorType::Err, s.Msg())); continue; } diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index d2143b6a787..67a17981ffe 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -28,7 +28,44 @@ void Reply(evbuffer *output, const std::string &data) { evbuffer_add(output, dat std::string SimpleString(const std::string &data) { return "+" + data + CRLF; } -std::string Error(const std::string &err) { return "-" + err + CRLF; } +std::string Error(const ErrorType type, const std::string &message) { + std::string prefix; + switch (type) { + case ErrorType::Loading: + prefix = "LOADING"; + break; + case ErrorType::NoScript: + prefix = "NOSCRIPT"; + break; + case ErrorType::WrongType: + prefix = "WRONGTYPE"; + break; + case ErrorType::NoProto: + prefix = "NOPROTO"; + break; + case ErrorType::NoAuth: + prefix = "NOAUTH"; + break; + case ErrorType::Readonly: + prefix = "READONLY"; + break; + case ErrorType::MasterDown: + prefix = "MASTERDOWN"; + break; + case ErrorType::ExecAbort: + prefix = "EXECABORT"; + break; + case ErrorType::Err: + prefix = "ERR"; + break; + default: + break; + } + if (prefix.empty()) { + return RESP_PREFIX_ERROR + message + CRLF; + } + return RESP_PREFIX_ERROR + prefix + " " + message + CRLF; +} std::string BulkString(const std::string &data) { return "$" + std::to_string(data.length()) + CRLF + data + CRLF; } diff --git a/src/server/redis_reply.h b/src/server/redis_reply.h index c7fd3b2a8e2..b39df386452 100644 --- a/src/server/redis_reply.h +++ b/src/server/redis_reply.h @@ -31,11 +31,24 @@ namespace redis { +enum class ErrorType { + None, + WrongType, + NoScript, + NoProto, + NoAuth, + Loading, + Readonly, + MasterDown, + ExecAbort, + Err, +}; + enum class RESP { v2, v3 }; void Reply(evbuffer *output, const std::string &data); std::string SimpleString(const std::string &data); -std::string Error(const std::string &err); +std::string Error(ErrorType type, const std::string &message); template , int> = 0> std::string Integer(T data) { diff --git a/src/server/worker.cc b/src/server/worker.cc index 1e8fed37441..6552ce5b5ca 100644 --- a/src/server/worker.cc +++ b/src/server/worker.cc @@ -167,7 +167,7 @@ void Worker::newTCPConnection(evconnlistener *listener, evutil_socket_t fd, sock s = AddConnection(conn); if (!s.IsOK()) { - std::string err_msg = redis::Error("ERR " + s.Msg()); + std::string err_msg = redis::Error(redis::ErrorType::Err, s.Msg()); s = util::SockSend(fd, err_msg, ssl); if (!s.IsOK()) { LOG(WARNING) << "Failed to send error response to socket: " << s.Msg(); @@ -200,7 +200,7 @@ void Worker::newUnixSocketConnection(evconnlistener *listener, evutil_socket_t f auto s = AddConnection(conn); if (!s.IsOK()) { - std::string err_msg = redis::Error("ERR " + s.Msg()); + std::string err_msg = redis::Error(redis::ErrorType::Err, s.Msg()); s = util::SockSend(fd, err_msg); if (!s.IsOK()) { LOG(WARNING) << "Failed to send error response to socket: " << s.Msg(); diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc index 409d5e19f92..9767670415e 100644 --- a/src/storage/scripting.cc +++ b/src/storage/scripting.cc @@ -611,7 +611,7 @@ Status EvalGenericCommand(redis::Connection *conn, const std::string &body_or_sh auto s = srv->ScriptGet(funcname + 2, &body); if (!s.IsOK()) { lua_pop(lua, 1); /* remove the error handler from the stack. */ - *output = redis::Error(redis::errNoMatchingScript); + *output = redis::Error(redis::ErrorType::NoScript, redis::errNoMatchingScript); return Status::OK(); } } else { @@ -640,8 +640,8 @@ Status EvalGenericCommand(redis::Connection *conn, const std::string &body_or_sh SetGlobalArray(lua, "ARGV", argv); if (lua_pcall(lua, 0, 1, -2)) { - auto msg = fmt::format("ERR running script (call to {}): {}", funcname, lua_tostring(lua, -1)); - *output = redis::Error(msg); + auto msg = fmt::format("running script (call to {}): {}", funcname, lua_tostring(lua, -1)); + *output = redis::Error(redis::ErrorType::Err, msg); lua_pop(lua, 2); } else { *output = ReplyToRedisReply(conn, lua); @@ -1192,7 +1192,7 @@ std::string ReplyToRedisReply(redis::Connection *conn, lua_State *lua) { lua_rawget(lua, -2); t = lua_type(lua, -1); if (t == LUA_TSTRING) { - output = redis::Error(lua_tostring(lua, -1)); + output = redis::Error(redis::ErrorType::None, lua_tostring(lua, -1)); lua_pop(lua, 1); return output; } From bbdaf19ef74c3d6c3fb02b98c7eeb7b271110989 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Tue, 11 Jun 2024 23:14:22 +0800 Subject: [PATCH 02/11] Allow to use Status in Redis::Error --- src/cluster/sync_migrate_context.cc | 2 +- src/commands/cmd_key.cc | 2 +- src/commands/cmd_replication.cc | 2 +- src/commands/cmd_server.cc | 6 +++--- src/server/redis_connection.cc | 6 +++--- src/server/redis_reply.h | 4 ++++ src/server/worker.cc | 3 +-- 7 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/cluster/sync_migrate_context.cc b/src/cluster/sync_migrate_context.cc index c530a323889..f1de89b6e26 100644 --- a/src/cluster/sync_migrate_context.cc +++ b/src/cluster/sync_migrate_context.cc @@ -68,7 +68,7 @@ void SyncMigrateContext::OnWrite(bufferevent *bev) { if (migrate_result_) { conn_->Reply(redis::SimpleString("OK")); } else { - conn_->Reply(redis::Error(redis::ErrorType::Err, migrate_result_.Msg())); + conn_->Reply(redis::Error(migrate_result_)); } timer_.reset(); diff --git a/src/commands/cmd_key.cc b/src/commands/cmd_key.cc index 63a1ef90130..ee4d01abff3 100644 --- a/src/commands/cmd_key.cc +++ b/src/commands/cmd_key.cc @@ -508,7 +508,7 @@ class CommandSort : public Commander { switch (res) { case Database::SortResult::UNKNOWN_TYPE: - *output = redis::Error(ErrorType::Err, "Unknown Type"); + *output = redis::Error(ErrorType::None, "Unknown Type"); break; case Database::SortResult::DOUBLE_CONVERT_ERROR: *output = redis::Error(ErrorType::None, "One or more scores can't be converted into double"); diff --git a/src/commands/cmd_replication.cc b/src/commands/cmd_replication.cc index 0c1cf8d2cc8..36e9e799a81 100644 --- a/src/commands/cmd_replication.cc +++ b/src/commands/cmd_replication.cc @@ -102,7 +102,7 @@ class CommandPSync : public Commander { srv->stats.IncrPSyncOKCount(); s = srv->AddSlave(conn, next_repl_seq_); if (!s.IsOK()) { - std::string err = redis::Error(ErrorType::None, s.Msg()); + std::string err = redis::Error(s); s = util::SockSend(conn->GetFD(), err, conn->GetBufferEvent()); if (!s.IsOK()) { LOG(WARNING) << "failed to send error message to the replica: " << s.Msg(); diff --git a/src/commands/cmd_server.cc b/src/commands/cmd_server.cc index 5b30ff90a0a..2d50aa19182 100644 --- a/src/commands/cmd_server.cc +++ b/src/commands/cmd_server.cc @@ -95,17 +95,17 @@ class CommandNamespace : public Commander { } } else if (args_.size() == 4 && sub_command == "set") { Status s = srv->GetNamespace()->Set(args_[2], args_[3]); - *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(ErrorType::Err, s.Msg()); + *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(s); 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 = srv->GetNamespace()->Add(args_[2], args_[3]); - *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(ErrorType::Err, s.Msg()); + *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(s); 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 = srv->GetNamespace()->Del(args_[2]); - *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(ErrorType::Err, s.Msg()); + *output = s.IsOK() ? redis::SimpleString("OK") : redis::Error(s); LOG(WARNING) << "Deleted namespace: " << args_[2] << ", addr: " << conn->GetAddr() << ", result: " << s.Msg(); } else { return {Status::RedisExecErr, "NAMESPACE subcommand must be one of GET, SET, DEL, ADD"}; diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc index 38dfa4b5b81..3a68d4af63f 100644 --- a/src/server/redis_connection.cc +++ b/src/server/redis_connection.cc @@ -87,7 +87,7 @@ void Connection::OnRead(struct bufferevent *bev) { auto s = req_.Tokenize(Input()); if (!s.IsOK()) { EnableFlag(redis::Connection::kCloseAfterReply); - Reply(redis::Error(ErrorType::Err, s.Msg())); + Reply(redis::Error(s)); LOG(INFO) << "[connection] Failed to tokenize the request. Error: " << s.Msg(); return; } @@ -493,7 +493,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { auto s = current_cmd->Parse(); if (!s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error(ErrorType::Err, s.Msg())); + Reply(redis::Error(s)); continue; } @@ -585,7 +585,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { // Reply for MULTI if (!s.IsOK()) { - Reply(redis::Error(ErrorType::Err, s.Msg())); + Reply(redis::Error(s)); continue; } diff --git a/src/server/redis_reply.h b/src/server/redis_reply.h index b39df386452..5fac6131d9c 100644 --- a/src/server/redis_reply.h +++ b/src/server/redis_reply.h @@ -25,6 +25,8 @@ #include #include +#include "status.h" + #define CRLF "\r\n" // NOLINT #define RESP_PREFIX_ERROR "-" // NOLINT #define RESP_PREFIX_SIMPLE_STRING "+" // NOLINT @@ -48,7 +50,9 @@ enum class RESP { v2, v3 }; void Reply(evbuffer *output, const std::string &data); std::string SimpleString(const std::string &data); + std::string Error(ErrorType type, const std::string &message); +std::string Error(const Status &s) { return Error(ErrorType::Err, s.Msg()); } template , int> = 0> std::string Integer(T data) { diff --git a/src/server/worker.cc b/src/server/worker.cc index 6552ce5b5ca..9e79322eeeb 100644 --- a/src/server/worker.cc +++ b/src/server/worker.cc @@ -200,8 +200,7 @@ void Worker::newUnixSocketConnection(evconnlistener *listener, evutil_socket_t f auto s = AddConnection(conn); if (!s.IsOK()) { - std::string err_msg = redis::Error(redis::ErrorType::Err, s.Msg()); - s = util::SockSend(fd, err_msg); + s = util::SockSend(fd, redis::Error(s)); if (!s.IsOK()) { LOG(WARNING) << "Failed to send error response to socket: " << s.Msg(); } From ddd125bbdd303c85d3aa45441644c9fecb458f92 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Wed, 12 Jun 2024 20:25:59 +0800 Subject: [PATCH 03/11] Rename ErrorType to ErrorKind --- src/commands/cmd_bloom_filter.cc | 6 +++--- src/commands/cmd_key.cc | 8 ++++---- src/commands/cmd_list.cc | 6 +++--- src/commands/cmd_replication.cc | 2 +- src/commands/cmd_script.cc | 2 +- src/commands/cmd_server.cc | 4 ++-- src/commands/cmd_stream.cc | 4 ++-- src/commands/cmd_txn.cc | 2 +- src/commands/cmd_zset.cc | 4 ++-- src/server/redis_connection.cc | 18 +++++++++--------- src/server/redis_reply.cc | 22 +++++++++++----------- src/server/redis_reply.h | 6 +++--- src/server/worker.cc | 2 +- src/storage/scripting.cc | 6 +++--- 14 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/commands/cmd_bloom_filter.cc b/src/commands/cmd_bloom_filter.cc index bd229dffd93..609f520918d 100644 --- a/src/commands/cmd_bloom_filter.cc +++ b/src/commands/cmd_bloom_filter.cc @@ -119,7 +119,7 @@ class CommandBFAdd : public Commander { *output = redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output = redis::Error(ErrorType::Err, errFilterFull); + *output = redis::Error(ErrorKind::Err, errFilterFull); break; } return Status::OK(); @@ -152,7 +152,7 @@ class CommandBFMAdd : public Commander { *output += redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output += redis::Error(ErrorType::Err, errFilterFull); + *output += redis::Error(ErrorKind::Err, errFilterFull); break; } } @@ -248,7 +248,7 @@ class CommandBFInsert : public Commander { *output += redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output += redis::Error(ErrorType::Err, errFilterFull); + *output += redis::Error(ErrorKind::Err, errFilterFull); break; } } diff --git a/src/commands/cmd_key.cc b/src/commands/cmd_key.cc index ee4d01abff3..1ab99e591aa 100644 --- a/src/commands/cmd_key.cc +++ b/src/commands/cmd_key.cc @@ -480,7 +480,7 @@ class CommandSort : public Commander { } if (type != RedisType::kRedisList && type != RedisType::kRedisSet && type != RedisType::kRedisZSet) { - *output = Error(ErrorType::WrongType, "Operation against a key holding the wrong kind of value"); + *output = Error(ErrorKind::WrongType, "Operation against a key holding the wrong kind of value"); return Status::OK(); } @@ -508,13 +508,13 @@ class CommandSort : public Commander { switch (res) { case Database::SortResult::UNKNOWN_TYPE: - *output = redis::Error(ErrorType::None, "Unknown Type"); + *output = redis::Error(ErrorKind::None, "Unknown Type"); break; case Database::SortResult::DOUBLE_CONVERT_ERROR: - *output = redis::Error(ErrorType::None, "One or more scores can't be converted into double"); + *output = redis::Error(ErrorKind::None, "One or more scores can't be converted into double"); break; case Database::SortResult::LIMIT_EXCEEDED: - *output = redis::Error(ErrorType::None, "The number of elements to be sorted exceeds SORT_LENGTH_LIMIT = " + + *output = redis::Error(ErrorKind::None, "The number of elements to be sorted exceeds SORT_LENGTH_LIMIT = " + std::to_string(SORT_LENGTH_LIMIT)); break; case Database::SortResult::DONE: diff --git a/src/commands/cmd_list.cc b/src/commands/cmd_list.cc index a9ae36fa6c0..68ab6b83416 100644 --- a/src/commands/cmd_list.cc +++ b/src/commands/cmd_list.cc @@ -304,7 +304,7 @@ class CommandBPop : public BlockingCommander { conn_->Reply(conn_->MultiBulkString({*last_key_ptr, std::move(elem)})); } } else if (!s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); + conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); } return s; @@ -414,7 +414,7 @@ class CommandBLMPop : public BlockingCommander { conn_->Reply(redis::Array({redis::BulkString(chosen_key), std::move(elems_bulk)})); } } else if (!s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); + conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); } return s; @@ -757,7 +757,7 @@ class CommandBLMove : public BlockingCommander { std::string elem; auto s = list_db.LMove(args_[1], args_[2], src_left_, dst_left_, &elem); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); + conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); return true; } diff --git a/src/commands/cmd_replication.cc b/src/commands/cmd_replication.cc index 36e9e799a81..899b7fa7eb2 100644 --- a/src/commands/cmd_replication.cc +++ b/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(ErrorType::None, "can't create db checkpoint"), bev); + s = util::SockSend(repl_fd, redis::Error(ErrorKind::None, "can't create db checkpoint"), bev); if (!s.IsOK()) { LOG(WARNING) << "[replication] Failed to send error response: " << s.Msg(); } diff --git a/src/commands/cmd_script.cc b/src/commands/cmd_script.cc index 49d85f274dc..0682262a9d9 100644 --- a/src/commands/cmd_script.cc +++ b/src/commands/cmd_script.cc @@ -31,7 +31,7 @@ class CommandEvalImpl : public Commander { public: Status Execute(Server *srv, Connection *conn, std::string *output) override { if (evalsha && args_[1].size() != 40) { - *output = redis::Error(ErrorType::NoScript, errNoMatchingScript); + *output = redis::Error(ErrorKind::NoScript, errNoMatchingScript); return Status::OK(); } diff --git a/src/commands/cmd_server.cc b/src/commands/cmd_server.cc index 2d50aa19182..cdc28d10330 100644 --- a/src/commands/cmd_server.cc +++ b/src/commands/cmd_server.cc @@ -630,7 +630,7 @@ class CommandDebug : public Commander { } else if (protocol_type_ == "verbatim") { // verbatim string *output = conn->VerbatimString("txt", "verbatim string"); } else { - *output = redis::Error(ErrorType::None, + *output = redis::Error(ErrorKind::None, "Wrong protocol type name. Please use one of the following: " "string|integer|double|array|set|bignum|true|false|null|attrib|verbatim"); } @@ -741,7 +741,7 @@ class CommandHello final : public Commander { // kvrocks only supports REPL2 by now, but for supporting some // `hello 3`, it will not report error when using 3. if (protocol < 2 || protocol > 3) { - conn->Reply(redis::Error(ErrorType::NoProto, "unsupported protocol version")); + conn->Reply(redis::Error(ErrorKind::NoProto, "unsupported protocol version")); return Status::OK(); } } diff --git a/src/commands/cmd_stream.cc b/src/commands/cmd_stream.cc index 944feee36f8..1aeb53d2ad9 100644 --- a/src/commands/cmd_stream.cc +++ b/src/commands/cmd_stream.cc @@ -1130,7 +1130,7 @@ class CommandXRead : public Commander, std::vector result; auto s = stream_db.Range(streams_[i], options, &result); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); + conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); return; } @@ -1423,7 +1423,7 @@ class CommandXReadGroup : public Commander, auto s = stream_db.RangeWithPending(streams_[i], options, &result, group_name_, consumer_name_, noack_, latest_marks_[i]); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); + conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); return; } diff --git a/src/commands/cmd_txn.cc b/src/commands/cmd_txn.cc index a7c145ce192..7833ab49bb2 100644 --- a/src/commands/cmd_txn.cc +++ b/src/commands/cmd_txn.cc @@ -68,7 +68,7 @@ class CommandExec : public Commander { auto reset_multiexec = MakeScopeExit([conn] { conn->ResetMultiExec(); }); if (conn->IsMultiError()) { - *output = redis::Error(ErrorType::ExecAbort, "Transaction discarded"); + *output = redis::Error(ErrorKind::ExecAbort, "Transaction discarded"); return Status::OK(); } diff --git a/src/commands/cmd_zset.cc b/src/commands/cmd_zset.cc index 3e243afe352..6796f318010 100644 --- a/src/commands/cmd_zset.cc +++ b/src/commands/cmd_zset.cc @@ -369,7 +369,7 @@ class CommandBZPop : public BlockingCommander { redis::ZSet zset_db(srv_->storage, conn_->GetNamespace()); auto s = PopFromMultipleZsets(&zset_db, keys_, min_, 1, &user_key, &member_scores); if (!s.ok()) { - conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); + conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); return true; } @@ -548,7 +548,7 @@ class CommandBZMPop : public BlockingCommander { redis::ZSet zset_db(srv_->storage, conn_->GetNamespace()); auto s = PopFromMultipleZsets(&zset_db, keys_, flag_ == ZSET_MIN, count_, &user_key, &member_scores); if (!s.ok()) { - conn_->Reply(redis::Error(ErrorType::Err, s.ToString())); + conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); return true; } diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc index 3a68d4af63f..61d35b6474d 100644 --- a/src/server/redis_connection.cc +++ b/src/server/redis_connection.cc @@ -432,7 +432,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { auto cmd_s = Server::LookupAndCreateCommand(cmd_tokens.front()); if (!cmd_s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error(ErrorType::Err, "unknown command " + cmd_tokens.front())); + Reply(redis::Error(ErrorKind::Err, "unknown command " + cmd_tokens.front())); continue; } auto current_cmd = std::move(*cmd_s); @@ -444,7 +444,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { if (GetNamespace().empty()) { if (!password.empty()) { if (cmd_name != "auth" && cmd_name != "hello") { - Reply(redis::Error(ErrorType::NoAuth, "Authentication required.")); + Reply(redis::Error(ErrorKind::NoAuth, "Authentication required.")); continue; } } else { @@ -477,7 +477,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (srv_->IsLoading() && !(cmd_flags & kCmdLoading)) { - Reply(redis::Error(ErrorType::Loading, errRestoringBackup)); + Reply(redis::Error(ErrorKind::Loading, errRestoringBackup)); if (is_multi_exec) multi_error_ = true; continue; } @@ -485,7 +485,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { int tokens = static_cast(cmd_tokens.size()); if (!attributes->CheckArity(tokens)) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error(ErrorType::Err, "wrong number of arguments")); + Reply(redis::Error(ErrorKind::Err, "wrong number of arguments")); continue; } @@ -498,7 +498,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (is_multi_exec && (cmd_flags & kCmdNoMulti)) { - Reply(redis::Error(ErrorType::Err, "Can't execute " + cmd_name + " in MULTI")); + Reply(redis::Error(ErrorKind::Err, "Can't execute " + cmd_name + " in MULTI")); multi_error_ = true; continue; } @@ -507,7 +507,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { s = srv_->cluster->CanExecByMySelf(attributes, cmd_tokens, this); if (!s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error(ErrorType::None, s.Msg())); + Reply(redis::Error(ErrorKind::None, s.Msg())); continue; } } @@ -525,18 +525,18 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (config->slave_readonly && srv_->IsSlave() && (cmd_flags & kCmdWrite)) { - Reply(redis::Error(ErrorType::Readonly, "You can't write against a read only slave.")); + Reply(redis::Error(ErrorKind::Readonly, "You can't write against a read only slave.")); continue; } if ((cmd_flags & kCmdWrite) && !(cmd_flags & kCmdNoDBSizeCheck) && srv_->storage->ReachedDBSizeLimit()) { - Reply(redis::Error(ErrorType::Err, "write command not allowed when reached max-db-size.")); + Reply(redis::Error(ErrorKind::Err, "write command not allowed when reached max-db-size.")); continue; } if (!config->slave_serve_stale_data && srv_->IsSlave() && cmd_name != "info" && cmd_name != "slaveof" && srv_->GetReplicationState() != kReplConnected) { - Reply(redis::Error(ErrorType::MasterDown, + Reply(redis::Error(ErrorKind::MasterDown, "Link with MASTER is down " "and slave-serve-stale-data is set to 'no'.")); continue; diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index 67a17981ffe..24661185aa4 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -28,34 +28,34 @@ void Reply(evbuffer *output, const std::string &data) { evbuffer_add(output, dat std::string SimpleString(const std::string &data) { return "+" + data + CRLF; } -std::string Error(const ErrorType type, const std::string &message) { +std::string Error(const ErrorKind kind, const std::string &message) { std::string prefix; - switch (type) { - case ErrorType::Loading: + switch (kind) { + case ErrorKind::Loading: prefix = "LOADING"; break; - case ErrorType::NoScript: + case ErrorKind::NoScript: prefix = "NOSCRIPT"; break; - case ErrorType::WrongType: + case ErrorKind::WrongType: prefix = "WRONGTYPE"; break; - case ErrorType::NoProto: + case ErrorKind::NoProto: prefix = "NOPROTO"; break; - case ErrorType::NoAuth: + case ErrorKind::NoAuth: prefix = "NOAUTH"; break; - case ErrorType::Readonly: + case ErrorKind::Readonly: prefix = "READONLY"; break; - case ErrorType::MasterDown: + case ErrorKind::MasterDown: prefix = "MASTERDOWN"; break; - case ErrorType::ExecAbort: + case ErrorKind::ExecAbort: prefix = "EXECABORT"; break; - case ErrorType::Err: + case ErrorKind::Err: prefix = "ERR"; break; default: diff --git a/src/server/redis_reply.h b/src/server/redis_reply.h index 5fac6131d9c..19369559e78 100644 --- a/src/server/redis_reply.h +++ b/src/server/redis_reply.h @@ -33,7 +33,7 @@ namespace redis { -enum class ErrorType { +enum class ErrorKind { None, WrongType, NoScript, @@ -51,8 +51,8 @@ enum class RESP { v2, v3 }; void Reply(evbuffer *output, const std::string &data); std::string SimpleString(const std::string &data); -std::string Error(ErrorType type, const std::string &message); -std::string Error(const Status &s) { return Error(ErrorType::Err, s.Msg()); } +std::string Error(ErrorKind kind, const std::string &message); +std::string Error(const Status &s) { return Error(ErrorKind::Err, s.Msg()); } template , int> = 0> std::string Integer(T data) { diff --git a/src/server/worker.cc b/src/server/worker.cc index 9e79322eeeb..7b798803d80 100644 --- a/src/server/worker.cc +++ b/src/server/worker.cc @@ -167,7 +167,7 @@ void Worker::newTCPConnection(evconnlistener *listener, evutil_socket_t fd, sock s = AddConnection(conn); if (!s.IsOK()) { - std::string err_msg = redis::Error(redis::ErrorType::Err, s.Msg()); + std::string err_msg = redis::Error(redis::ErrorKind::Err, s.Msg()); s = util::SockSend(fd, err_msg, ssl); if (!s.IsOK()) { LOG(WARNING) << "Failed to send error response to socket: " << s.Msg(); diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc index 9767670415e..726afd7dc51 100644 --- a/src/storage/scripting.cc +++ b/src/storage/scripting.cc @@ -611,7 +611,7 @@ Status EvalGenericCommand(redis::Connection *conn, const std::string &body_or_sh auto s = srv->ScriptGet(funcname + 2, &body); if (!s.IsOK()) { lua_pop(lua, 1); /* remove the error handler from the stack. */ - *output = redis::Error(redis::ErrorType::NoScript, redis::errNoMatchingScript); + *output = redis::Error(redis::ErrorKind::NoScript, redis::errNoMatchingScript); return Status::OK(); } } else { @@ -641,7 +641,7 @@ Status EvalGenericCommand(redis::Connection *conn, const std::string &body_or_sh if (lua_pcall(lua, 0, 1, -2)) { auto msg = fmt::format("running script (call to {}): {}", funcname, lua_tostring(lua, -1)); - *output = redis::Error(redis::ErrorType::Err, msg); + *output = redis::Error(redis::ErrorKind::Err, msg); lua_pop(lua, 2); } else { *output = ReplyToRedisReply(conn, lua); @@ -1192,7 +1192,7 @@ std::string ReplyToRedisReply(redis::Connection *conn, lua_State *lua) { lua_rawget(lua, -2); t = lua_type(lua, -1); if (t == LUA_TSTRING) { - output = redis::Error(redis::ErrorType::None, lua_tostring(lua, -1)); + output = redis::Error(redis::ErrorKind::None, lua_tostring(lua, -1)); lua_pop(lua, 1); return output; } From 6c1f173784bb2addacf17122fc4ec1fec0b72cdc Mon Sep 17 00:00:00 2001 From: git-hulk Date: Wed, 12 Jun 2024 21:52:37 +0800 Subject: [PATCH 04/11] Fix build error --- src/server/redis_reply.cc | 2 ++ src/server/redis_reply.h | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index 24661185aa4..55e8da0e8eb 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -67,6 +67,8 @@ std::string Error(const ErrorKind kind, const std::string &message) { return RESP_PREFIX_ERROR + prefix + " " + message + CRLF; } +std::string Error(const Status &s) { return Error(ErrorKind::Err, s.Msg()); } + std::string BulkString(const std::string &data) { return "$" + std::to_string(data.length()) + CRLF + data + CRLF; } std::string Array(const std::vector &list) { diff --git a/src/server/redis_reply.h b/src/server/redis_reply.h index 19369559e78..804e1b6a76f 100644 --- a/src/server/redis_reply.h +++ b/src/server/redis_reply.h @@ -52,7 +52,7 @@ void Reply(evbuffer *output, const std::string &data); std::string SimpleString(const std::string &data); std::string Error(ErrorKind kind, const std::string &message); -std::string Error(const Status &s) { return Error(ErrorKind::Err, s.Msg()); } +std::string Error(const Status &s); template , int> = 0> std::string Integer(T data) { From 91038620f6359a83db83a0eb9cb1d04a0f9edc5e Mon Sep 17 00:00:00 2001 From: git-hulk Date: Thu, 13 Jun 2024 21:28:44 +0800 Subject: [PATCH 05/11] Use the error status to replace the error output --- src/commands/cmd_bloom_filter.cc | 6 +-- src/commands/cmd_key.cc | 14 +++---- src/commands/cmd_list.cc | 6 +-- src/commands/cmd_replication.cc | 2 +- src/commands/cmd_script.cc | 3 +- src/commands/cmd_server.cc | 9 ++--- src/commands/cmd_stream.cc | 4 +- src/commands/cmd_txn.cc | 3 +- src/commands/cmd_zset.cc | 4 +- src/common/redis_error.h | 63 ++++++++++++++++++++++++++++++++ src/common/status.h | 7 ++++ src/server/redis_connection.cc | 22 +++++------ src/server/redis_reply.cc | 43 ++++------------------ src/server/redis_reply.h | 14 ------- src/server/worker.cc | 2 +- src/storage/scripting.cc | 7 ++-- 16 files changed, 114 insertions(+), 95 deletions(-) create mode 100644 src/common/redis_error.h diff --git a/src/commands/cmd_bloom_filter.cc b/src/commands/cmd_bloom_filter.cc index 609f520918d..1975dd0e52f 100644 --- a/src/commands/cmd_bloom_filter.cc +++ b/src/commands/cmd_bloom_filter.cc @@ -119,7 +119,7 @@ class CommandBFAdd : public Commander { *output = redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output = redis::Error(ErrorKind::Err, errFilterFull); + *output = redis::Error({ErrorKind::Err, errFilterFull}); break; } return Status::OK(); @@ -152,7 +152,7 @@ class CommandBFMAdd : public Commander { *output += redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output += redis::Error(ErrorKind::Err, errFilterFull); + *output += redis::Error({ErrorKind::Err, errFilterFull}); break; } } @@ -248,7 +248,7 @@ class CommandBFInsert : public Commander { *output += redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output += redis::Error(ErrorKind::Err, errFilterFull); + *output += redis::Error({ErrorKind::Err, errFilterFull}); break; } } diff --git a/src/commands/cmd_key.cc b/src/commands/cmd_key.cc index 1ab99e591aa..a6400d4a774 100644 --- a/src/commands/cmd_key.cc +++ b/src/commands/cmd_key.cc @@ -480,8 +480,7 @@ class CommandSort : public Commander { } if (type != RedisType::kRedisList && type != RedisType::kRedisSet && type != RedisType::kRedisZSet) { - *output = Error(ErrorKind::WrongType, "Operation against a key holding the wrong kind of value"); - return Status::OK(); + return {ErrorKind::WrongType, "Operation against a key holding the wrong kind of value"}; } /* When sorting a set with no sort specified, we must sort the output @@ -508,15 +507,12 @@ class CommandSort : public Commander { switch (res) { case Database::SortResult::UNKNOWN_TYPE: - *output = redis::Error(ErrorKind::None, "Unknown Type"); - break; + return {ErrorKind::None, "Unknown Type"}; case Database::SortResult::DOUBLE_CONVERT_ERROR: - *output = redis::Error(ErrorKind::None, "One or more scores can't be converted into double"); - break; + return {ErrorKind::None, "One or more scores can't be converted into double"}; case Database::SortResult::LIMIT_EXCEEDED: - *output = redis::Error(ErrorKind::None, "The number of elements to be sorted exceeds SORT_LENGTH_LIMIT = " + - std::to_string(SORT_LENGTH_LIMIT)); - break; + return {ErrorKind::None, + "The number of elements to be sorted exceeds SORT_LENGTH_LIMIT = " + std::to_string(SORT_LENGTH_LIMIT)}; case Database::SortResult::DONE: if (sort_argument_.storekey.empty()) { std::vector output_vec; diff --git a/src/commands/cmd_list.cc b/src/commands/cmd_list.cc index 68ab6b83416..3ddb92eea66 100644 --- a/src/commands/cmd_list.cc +++ b/src/commands/cmd_list.cc @@ -304,7 +304,7 @@ class CommandBPop : public BlockingCommander { conn_->Reply(conn_->MultiBulkString({*last_key_ptr, std::move(elem)})); } } else if (!s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); + conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); } return s; @@ -414,7 +414,7 @@ class CommandBLMPop : public BlockingCommander { conn_->Reply(redis::Array({redis::BulkString(chosen_key), std::move(elems_bulk)})); } } else if (!s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); + conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); } return s; @@ -757,7 +757,7 @@ class CommandBLMove : public BlockingCommander { std::string elem; auto s = list_db.LMove(args_[1], args_[2], src_left_, dst_left_, &elem); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); + conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); return true; } diff --git a/src/commands/cmd_replication.cc b/src/commands/cmd_replication.cc index 899b7fa7eb2..f94d55d1d5b 100644 --- a/src/commands/cmd_replication.cc +++ b/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(ErrorKind::None, "can't create db checkpoint"), bev); + s = util::SockSend(repl_fd, redis::Error({ErrorKind::None, "can't create db checkpoint"}), bev); if (!s.IsOK()) { LOG(WARNING) << "[replication] Failed to send error response: " << s.Msg(); } diff --git a/src/commands/cmd_script.cc b/src/commands/cmd_script.cc index 0682262a9d9..322fe92162e 100644 --- a/src/commands/cmd_script.cc +++ b/src/commands/cmd_script.cc @@ -31,8 +31,7 @@ class CommandEvalImpl : public Commander { public: Status Execute(Server *srv, Connection *conn, std::string *output) override { if (evalsha && args_[1].size() != 40) { - *output = redis::Error(ErrorKind::NoScript, errNoMatchingScript); - return Status::OK(); + return {ErrorKind::NoScript, errNoMatchingScript}; } int64_t numkeys = GET_OR_RET(ParseInt(args_[2], 10)); diff --git a/src/commands/cmd_server.cc b/src/commands/cmd_server.cc index cdc28d10330..a54a2d06e4a 100644 --- a/src/commands/cmd_server.cc +++ b/src/commands/cmd_server.cc @@ -630,9 +630,9 @@ class CommandDebug : public Commander { } else if (protocol_type_ == "verbatim") { // verbatim string *output = conn->VerbatimString("txt", "verbatim string"); } else { - *output = redis::Error(ErrorKind::None, - "Wrong protocol type name. Please use one of the following: " - "string|integer|double|array|set|bignum|true|false|null|attrib|verbatim"); + return {ErrorKind::None, + "Wrong protocol type name. Please use one of the following: " + "string|integer|double|array|set|bignum|true|false|null|attrib|verbatim"}; } } else if (subcommand_ == "dbsize-limit") { srv->storage->SetDBSizeLimit(dbsize_limit_); @@ -741,8 +741,7 @@ class CommandHello final : public Commander { // kvrocks only supports REPL2 by now, but for supporting some // `hello 3`, it will not report error when using 3. if (protocol < 2 || protocol > 3) { - conn->Reply(redis::Error(ErrorKind::NoProto, "unsupported protocol version")); - return Status::OK(); + return {ErrorKind::NoProto, "unsupported protocol version"}; } } diff --git a/src/commands/cmd_stream.cc b/src/commands/cmd_stream.cc index 1aeb53d2ad9..580c6a278ad 100644 --- a/src/commands/cmd_stream.cc +++ b/src/commands/cmd_stream.cc @@ -1130,7 +1130,7 @@ class CommandXRead : public Commander, std::vector result; auto s = stream_db.Range(streams_[i], options, &result); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); + conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); return; } @@ -1423,7 +1423,7 @@ class CommandXReadGroup : public Commander, auto s = stream_db.RangeWithPending(streams_[i], options, &result, group_name_, consumer_name_, noack_, latest_marks_[i]); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); + conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); return; } diff --git a/src/commands/cmd_txn.cc b/src/commands/cmd_txn.cc index 7833ab49bb2..57332dbcff8 100644 --- a/src/commands/cmd_txn.cc +++ b/src/commands/cmd_txn.cc @@ -68,8 +68,7 @@ class CommandExec : public Commander { auto reset_multiexec = MakeScopeExit([conn] { conn->ResetMultiExec(); }); if (conn->IsMultiError()) { - *output = redis::Error(ErrorKind::ExecAbort, "Transaction discarded"); - return Status::OK(); + return {ErrorKind::ExecAbort, "Transaction discarded"}; } if (srv->IsWatchedKeysModified(conn)) { diff --git a/src/commands/cmd_zset.cc b/src/commands/cmd_zset.cc index 6796f318010..631cdd091c1 100644 --- a/src/commands/cmd_zset.cc +++ b/src/commands/cmd_zset.cc @@ -369,7 +369,7 @@ class CommandBZPop : public BlockingCommander { redis::ZSet zset_db(srv_->storage, conn_->GetNamespace()); auto s = PopFromMultipleZsets(&zset_db, keys_, min_, 1, &user_key, &member_scores); if (!s.ok()) { - conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); + conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); return true; } @@ -548,7 +548,7 @@ class CommandBZMPop : public BlockingCommander { redis::ZSet zset_db(srv_->storage, conn_->GetNamespace()); auto s = PopFromMultipleZsets(&zset_db, keys_, flag_ == ZSET_MIN, count_, &user_key, &member_scores); if (!s.ok()) { - conn_->Reply(redis::Error(ErrorKind::Err, s.ToString())); + conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); return true; } diff --git a/src/common/redis_error.h b/src/common/redis_error.h new file mode 100644 index 00000000000..ce440922768 --- /dev/null +++ b/src/common/redis_error.h @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * + */ + +#pragma once + +namespace redis { + +enum class ErrorKind { + None, + WrongType, + NoScript, + NoProto, + NoAuth, + Loading, + Readonly, + MasterDown, + ExecAbort, + Err, +}; + +inline std::string ErrorKindToPrefix(ErrorKind kind) { + switch (kind) { + case ErrorKind::Loading: + return "LOADING"; + case ErrorKind::NoScript: + return "NOSCRIPT"; + case ErrorKind::WrongType: + return "WRONGTYPE"; + case ErrorKind::NoProto: + return "NOPROTO"; + case ErrorKind::NoAuth: + return "NOAUTH"; + case ErrorKind::Readonly: + return "READONLY"; + case ErrorKind::MasterDown: + return "MASTERDOWN"; + case ErrorKind::ExecAbort: + return "EXECABORT"; + case ErrorKind::Err: + return "ERR"; + default: + return {}; + } +} + +} // namespace redis diff --git a/src/common/status.h b/src/common/status.h index b425ea5b238..d528b43ac02 100644 --- a/src/common/status.h +++ b/src/common/status.h @@ -30,6 +30,7 @@ #include #include +#include "redis_error.h" #include "type_util.h" class [[nodiscard]] Status { @@ -46,6 +47,7 @@ class [[nodiscard]] Status { DBGetWALErr, // Redis + RedisError, RedisUnknownCmd, RedisInvalidCmd, RedisParseErr, @@ -68,6 +70,7 @@ class [[nodiscard]] Status { Status(Code code, std::string msg = {}) : impl_{new Impl{code, std::move(msg)}} { // NOLINT CHECK(code != cOK); } + Status(redis::ErrorKind kind, std::string msg) : impl_{new Impl{RedisError, std::move(msg), kind}} {} Status(const Status& s) : impl_{s.impl_ ? new Impl{s.impl_->code, s.impl_->msg} : nullptr} {} Status(Status&&) = default; @@ -89,6 +92,7 @@ class [[nodiscard]] Status { explicit operator bool() const { return IsOK(); } Code GetCode() const { return impl_ ? impl_->code : cOK; } + redis::ErrorKind GetErrorKind() const { return impl_ ? impl_->kind : redis::ErrorKind::None; } std::string Msg() const& { if (*this) return ok_msg; @@ -120,6 +124,9 @@ class [[nodiscard]] Status { struct Impl { Code code; std::string msg; + // Error kind is used to distinguish different types of Redis errors, + // it only takes effect when code is RedisError. + redis::ErrorKind kind = redis::ErrorKind::None; }; std::unique_ptr impl_; diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc index 61d35b6474d..1ca704f4b2a 100644 --- a/src/server/redis_connection.cc +++ b/src/server/redis_connection.cc @@ -432,7 +432,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { auto cmd_s = Server::LookupAndCreateCommand(cmd_tokens.front()); if (!cmd_s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error(ErrorKind::Err, "unknown command " + cmd_tokens.front())); + Reply(redis::Error({ErrorKind::Err, "unknown command " + cmd_tokens.front()})); continue; } auto current_cmd = std::move(*cmd_s); @@ -444,7 +444,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { if (GetNamespace().empty()) { if (!password.empty()) { if (cmd_name != "auth" && cmd_name != "hello") { - Reply(redis::Error(ErrorKind::NoAuth, "Authentication required.")); + Reply(redis::Error({ErrorKind::NoAuth, "Authentication required."})); continue; } } else { @@ -477,7 +477,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (srv_->IsLoading() && !(cmd_flags & kCmdLoading)) { - Reply(redis::Error(ErrorKind::Loading, errRestoringBackup)); + Reply(redis::Error({ErrorKind::Loading, errRestoringBackup})); if (is_multi_exec) multi_error_ = true; continue; } @@ -485,7 +485,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { int tokens = static_cast(cmd_tokens.size()); if (!attributes->CheckArity(tokens)) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error(ErrorKind::Err, "wrong number of arguments")); + Reply(redis::Error({ErrorKind::Err, "wrong number of arguments"})); continue; } @@ -498,7 +498,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (is_multi_exec && (cmd_flags & kCmdNoMulti)) { - Reply(redis::Error(ErrorKind::Err, "Can't execute " + cmd_name + " in MULTI")); + Reply(redis::Error({ErrorKind::Err, "Can't execute " + cmd_name + " in MULTI"})); multi_error_ = true; continue; } @@ -507,7 +507,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { s = srv_->cluster->CanExecByMySelf(attributes, cmd_tokens, this); if (!s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error(ErrorKind::None, s.Msg())); + Reply(redis::Error({ErrorKind::None, s.Msg()})); continue; } } @@ -525,20 +525,20 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (config->slave_readonly && srv_->IsSlave() && (cmd_flags & kCmdWrite)) { - Reply(redis::Error(ErrorKind::Readonly, "You can't write against a read only slave.")); + Reply(redis::Error({ErrorKind::Readonly, "You can't write against a read only slave."})); continue; } if ((cmd_flags & kCmdWrite) && !(cmd_flags & kCmdNoDBSizeCheck) && srv_->storage->ReachedDBSizeLimit()) { - Reply(redis::Error(ErrorKind::Err, "write command not allowed when reached max-db-size.")); + Reply(redis::Error({ErrorKind::Err, "write command not allowed when reached max-db-size."})); continue; } if (!config->slave_serve_stale_data && srv_->IsSlave() && cmd_name != "info" && cmd_name != "slaveof" && srv_->GetReplicationState() != kReplConnected) { - Reply(redis::Error(ErrorKind::MasterDown, - "Link with MASTER is down " - "and slave-serve-stale-data is set to 'no'.")); + Reply(redis::Error({ErrorKind::MasterDown, + "Link with MASTER is down " + "and slave-serve-stale-data is set to 'no'."})); continue; } diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index 55e8da0e8eb..e57df7222c4 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -28,47 +28,18 @@ void Reply(evbuffer *output, const std::string &data) { evbuffer_add(output, dat std::string SimpleString(const std::string &data) { return "+" + data + CRLF; } -std::string Error(const ErrorKind kind, const std::string &message) { - std::string prefix; - switch (kind) { - case ErrorKind::Loading: - prefix = "LOADING"; - break; - case ErrorKind::NoScript: - prefix = "NOSCRIPT"; - break; - case ErrorKind::WrongType: - prefix = "WRONGTYPE"; - break; - case ErrorKind::NoProto: - prefix = "NOPROTO"; - break; - case ErrorKind::NoAuth: - prefix = "NOAUTH"; - break; - case ErrorKind::Readonly: - prefix = "READONLY"; - break; - case ErrorKind::MasterDown: - prefix = "MASTERDOWN"; - break; - case ErrorKind::ExecAbort: - prefix = "EXECABORT"; - break; - case ErrorKind::Err: - prefix = "ERR"; - break; - default: - break; +std::string Error(const Status &s) { + CHECK(!s.IsOK()); + if (!s.Is()) { + return "-ERR " + s.Msg() + CRLF; } + auto prefix = ErrorKindToPrefix(s.GetErrorKind()); if (prefix.empty()) { - return RESP_PREFIX_ERROR + message + CRLF; + return "-" + s.Msg() + CRLF; } - return RESP_PREFIX_ERROR + prefix + " " + message + CRLF; + return "-" + prefix + " " + s.Msg() + CRLF; } -std::string Error(const Status &s) { return Error(ErrorKind::Err, s.Msg()); } - std::string BulkString(const std::string &data) { return "$" + std::to_string(data.length()) + CRLF + data + CRLF; } std::string Array(const std::vector &list) { diff --git a/src/server/redis_reply.h b/src/server/redis_reply.h index 804e1b6a76f..c68790931e0 100644 --- a/src/server/redis_reply.h +++ b/src/server/redis_reply.h @@ -33,25 +33,11 @@ namespace redis { -enum class ErrorKind { - None, - WrongType, - NoScript, - NoProto, - NoAuth, - Loading, - Readonly, - MasterDown, - ExecAbort, - Err, -}; - enum class RESP { v2, v3 }; void Reply(evbuffer *output, const std::string &data); std::string SimpleString(const std::string &data); -std::string Error(ErrorKind kind, const std::string &message); std::string Error(const Status &s); template , int> = 0> diff --git a/src/server/worker.cc b/src/server/worker.cc index 7b798803d80..15d2b5ed044 100644 --- a/src/server/worker.cc +++ b/src/server/worker.cc @@ -167,7 +167,7 @@ void Worker::newTCPConnection(evconnlistener *listener, evutil_socket_t fd, sock s = AddConnection(conn); if (!s.IsOK()) { - std::string err_msg = redis::Error(redis::ErrorKind::Err, s.Msg()); + std::string err_msg = redis::Error({redis::ErrorKind::Err, s.Msg()}); s = util::SockSend(fd, err_msg, ssl); if (!s.IsOK()) { LOG(WARNING) << "Failed to send error response to socket: " << s.Msg(); diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc index 726afd7dc51..fcb923345e8 100644 --- a/src/storage/scripting.cc +++ b/src/storage/scripting.cc @@ -611,8 +611,7 @@ Status EvalGenericCommand(redis::Connection *conn, const std::string &body_or_sh auto s = srv->ScriptGet(funcname + 2, &body); if (!s.IsOK()) { lua_pop(lua, 1); /* remove the error handler from the stack. */ - *output = redis::Error(redis::ErrorKind::NoScript, redis::errNoMatchingScript); - return Status::OK(); + return {redis::ErrorKind::NoScript, redis::errNoMatchingScript}; } } else { body = body_or_sha; @@ -641,7 +640,7 @@ Status EvalGenericCommand(redis::Connection *conn, const std::string &body_or_sh if (lua_pcall(lua, 0, 1, -2)) { auto msg = fmt::format("running script (call to {}): {}", funcname, lua_tostring(lua, -1)); - *output = redis::Error(redis::ErrorKind::Err, msg); + *output = redis::Error({redis::ErrorKind::Err, msg}); lua_pop(lua, 2); } else { *output = ReplyToRedisReply(conn, lua); @@ -1192,7 +1191,7 @@ std::string ReplyToRedisReply(redis::Connection *conn, lua_State *lua) { lua_rawget(lua, -2); t = lua_type(lua, -1); if (t == LUA_TSTRING) { - output = redis::Error(redis::ErrorKind::None, lua_tostring(lua, -1)); + output = redis::Error({redis::ErrorKind::None, lua_tostring(lua, -1)}); lua_pop(lua, 1); return output; } From fb342c9e9aca7894d4f68ffa294291b26d76b22d Mon Sep 17 00:00:00 2001 From: git-hulk Date: Thu, 13 Jun 2024 22:15:28 +0800 Subject: [PATCH 06/11] Use ErrorKind::* to replace the error string --- src/cluster/cluster.cc | 11 +++++----- src/common/redis_error.h | 45 +++++++++++++++++---------------------- src/server/redis_reply.cc | 7 +++--- 3 files changed, 28 insertions(+), 35 deletions(-) diff --git a/src/cluster/cluster.cc b/src/cluster/cluster.cc index d080ed34452..6a76bfe36b3 100644 --- a/src/cluster/cluster.cc +++ b/src/cluster/cluster.cc @@ -821,13 +821,13 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons int cur_slot = GetSlotIdFromKey(cmd_tokens[i]); if (slot == -1) slot = cur_slot; if (slot != cur_slot) { - return {Status::RedisExecErr, "CROSSSLOT Attempted to access keys that don't hash to the same slot"}; + return {redis::ErrorKind::CrossSlot, "Attempted to access keys that don't hash to the same slot"}; } } if (slot == -1) return Status::OK(); if (slots_nodes_[slot] == nullptr) { - return {Status::ClusterDown, "CLUSTERDOWN Hash slot not served"}; + return {redis::ErrorKind::ClusterDown, "Hash slot not served"}; } if (myself_ && myself_ == slots_nodes_[slot]) { @@ -835,12 +835,12 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons // Server can't change the topology directly, so we record the migrated slots // to move the requests of the migrated slots to the destination node. if (migrated_slots_.count(slot) > 0) { // I'm not serving the migrated slot - return {Status::RedisExecErr, fmt::format("MOVED {} {}", slot, migrated_slots_[slot])}; + return {redis::ErrorKind::Moved, fmt::format("{} {}", slot, migrated_slots_[slot])}; } // To keep data consistency, slot will be forbidden write while sending the last incremental data. // During this phase, the requests of the migrating slot has to be rejected. if ((attributes->flags & redis::kCmdWrite) && IsWriteForbiddenSlot(slot)) { - return {Status::RedisExecErr, "TRYAGAIN Can't write to slot being migrated which is in write forbidden phase"}; + return {redis::ErrorKind::TryAgain, "Can't write to slot being migrated which is in write forbidden phase"}; } return Status::OK(); // I'm serving this slot @@ -868,8 +868,7 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons return Status::OK(); // My master is serving this slot } - return {Status::RedisExecErr, - fmt::format("MOVED {} {}:{}", slot, slots_nodes_[slot]->host, slots_nodes_[slot]->port)}; + return {redis::ErrorKind::Moved, fmt::format("{} {}:{}", slot, slots_nodes_[slot]->host, slots_nodes_[slot]->port)}; } // Only HARD mode is meaningful to the Kvrocks cluster, diff --git a/src/common/redis_error.h b/src/common/redis_error.h index ce440922768..6def31be006 100644 --- a/src/common/redis_error.h +++ b/src/common/redis_error.h @@ -20,10 +20,13 @@ #pragma once +#include + namespace redis { enum class ErrorKind { None, + Err, WrongType, NoScript, NoProto, @@ -32,32 +35,24 @@ enum class ErrorKind { Readonly, MasterDown, ExecAbort, - Err, + Moved, + TryAgain, + ClusterDown, + CrossSlot, }; -inline std::string ErrorKindToPrefix(ErrorKind kind) { - switch (kind) { - case ErrorKind::Loading: - return "LOADING"; - case ErrorKind::NoScript: - return "NOSCRIPT"; - case ErrorKind::WrongType: - return "WRONGTYPE"; - case ErrorKind::NoProto: - return "NOPROTO"; - case ErrorKind::NoAuth: - return "NOAUTH"; - case ErrorKind::Readonly: - return "READONLY"; - case ErrorKind::MasterDown: - return "MASTERDOWN"; - case ErrorKind::ExecAbort: - return "EXECABORT"; - case ErrorKind::Err: - return "ERR"; - default: - return {}; - } -} +const std::map ErrorKindMap = {{ErrorKind::Err, "ERR"}, + {ErrorKind::WrongType, "WRONGTYPE"}, + {ErrorKind::NoScript, "NOSCRIPT"}, + {ErrorKind::NoProto, "NOPROTO"}, + {ErrorKind::NoAuth, "NOAUTH"}, + {ErrorKind::Loading, "LOADING"}, + {ErrorKind::Readonly, "READONLY"}, + {ErrorKind::MasterDown, "MASTERDOWN"}, + {ErrorKind::ExecAbort, "EXECABORT"}, + {ErrorKind::Moved, "MOVED"}, + {ErrorKind::TryAgain, "TRYAGAIN"}, + {ErrorKind::ClusterDown, "CLUSTERDOWN"}, + {ErrorKind::CrossSlot, "CROSSSLOT"}}; } // namespace redis diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index e57df7222c4..d8645950f69 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -33,11 +33,10 @@ std::string Error(const Status &s) { if (!s.Is()) { return "-ERR " + s.Msg() + CRLF; } - auto prefix = ErrorKindToPrefix(s.GetErrorKind()); - if (prefix.empty()) { - return "-" + s.Msg() + CRLF; + if (auto iter = ErrorKindMap.find(s.GetErrorKind()); iter != ErrorKindMap.end()) { + return "-" + iter->second + " " + s.Msg() + CRLF; } - return "-" + prefix + " " + s.Msg() + CRLF; + return "-" + s.Msg() + CRLF; } std::string BulkString(const std::string &data) { return "$" + std::to_string(data.length()) + CRLF + data + CRLF; } From 4d1020e8251b871549a713921920d72fc417ccce Mon Sep 17 00:00:00 2001 From: git-hulk Date: Thu, 13 Jun 2024 22:41:15 +0800 Subject: [PATCH 07/11] Fix wrong message in Lua --- src/server/redis_connection.cc | 2 +- src/server/redis_reply.cc | 10 ++++++---- src/server/redis_reply.h | 1 + src/storage/scripting.cc | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc index 1ca704f4b2a..fe4f8fe43bd 100644 --- a/src/server/redis_connection.cc +++ b/src/server/redis_connection.cc @@ -507,7 +507,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { s = srv_->cluster->CanExecByMySelf(attributes, cmd_tokens, this); if (!s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error({ErrorKind::None, s.Msg()})); + Reply(redis::Error(s)); continue; } } diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index d8645950f69..8e5cbaf2054 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -28,15 +28,17 @@ void Reply(evbuffer *output, const std::string &data) { evbuffer_add(output, dat std::string SimpleString(const std::string &data) { return "+" + data + CRLF; } -std::string Error(const Status &s) { +std::string Error(const Status &s) { return RESP_PREFIX_ERROR + StatusToRedisError(s); } + +std::string StatusToRedisError(const Status &s) { CHECK(!s.IsOK()); if (!s.Is()) { - return "-ERR " + s.Msg() + CRLF; + return "ERR " + s.Msg() + CRLF; } if (auto iter = ErrorKindMap.find(s.GetErrorKind()); iter != ErrorKindMap.end()) { - return "-" + iter->second + " " + s.Msg() + CRLF; + return iter->second + " " + s.Msg() + CRLF; } - return "-" + s.Msg() + CRLF; + return s.Msg() + CRLF; } std::string BulkString(const std::string &data) { return "$" + std::to_string(data.length()) + CRLF + data + CRLF; } diff --git a/src/server/redis_reply.h b/src/server/redis_reply.h index c68790931e0..6e40fd39c10 100644 --- a/src/server/redis_reply.h +++ b/src/server/redis_reply.h @@ -39,6 +39,7 @@ void Reply(evbuffer *output, const std::string &data); std::string SimpleString(const std::string &data); std::string Error(const Status &s); +std::string StatusToRedisError(const Status &s); template , int> = 0> std::string Integer(T data) { diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc index fcb923345e8..3bc1af9867e 100644 --- a/src/storage/scripting.cc +++ b/src/storage/scripting.cc @@ -754,7 +754,7 @@ int RedisGenericCommand(lua_State *lua, int raise_error) { if (config->cluster_enabled) { auto s = srv->cluster->CanExecByMySelf(attributes, args, conn); if (!s.IsOK()) { - PushError(lua, s.Msg().c_str()); + PushError(lua, redis::StatusToRedisError(s).c_str()); return raise_error ? RaiseError(lua) : 1; } } From 045b087679885b9779cd8134e1f74f720b2079d1 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Fri, 14 Jun 2024 16:35:34 +0800 Subject: [PATCH 08/11] Move the error kind inside status --- src/cluster/cluster.cc | 18 ++--- src/cluster/cluster_defs.h | 2 +- src/commands/cmd_bloom_filter.cc | 6 +- src/commands/cmd_cluster.cc | 20 +++--- src/commands/cmd_hash.cc | 9 +-- src/commands/cmd_key.cc | 8 +-- src/commands/cmd_list.cc | 6 +- src/commands/cmd_replication.cc | 2 +- src/commands/cmd_script.cc | 2 +- src/commands/cmd_server.cc | 21 +++--- src/commands/cmd_stream.cc | 24 ++----- src/commands/cmd_txn.cc | 2 +- src/commands/cmd_zset.cc | 20 ++---- src/common/redis_error.h | 58 ---------------- src/common/status.h | 21 +++--- src/server/redis_connection.cc | 16 ++--- src/server/redis_reply.cc | 21 ++++-- src/server/worker.cc | 2 +- src/storage/rdb.cc | 109 +++++++------------------------ src/storage/scripting.cc | 6 +- src/types/json.h | 2 +- 21 files changed, 123 insertions(+), 252 deletions(-) delete mode 100644 src/common/redis_error.h diff --git a/src/cluster/cluster.cc b/src/cluster/cluster.cc index 6a76bfe36b3..8d373e0dfc1 100644 --- a/src/cluster/cluster.cc +++ b/src/cluster/cluster.cc @@ -363,7 +363,7 @@ Status Cluster::ImportSlot(redis::Connection *conn, int slot, int state) { Status Cluster::GetClusterInfo(std::string *cluster_infos) { if (version_ < 0) { - return {Status::ClusterDown, errClusterNoInitialized}; + return {Status::RedisClusterDown, errClusterNoInitialized}; } cluster_infos->clear(); @@ -421,7 +421,7 @@ Status Cluster::GetClusterInfo(std::string *cluster_infos) { // ... continued until done Status Cluster::GetSlotsInfo(std::vector *slots_infos) { if (version_ < 0) { - return {Status::ClusterDown, errClusterNoInitialized}; + return {Status::RedisClusterDown, errClusterNoInitialized}; } slots_infos->clear(); @@ -464,7 +464,7 @@ SlotInfo Cluster::genSlotNodeInfo(int start, int end, const std::shared_ptr Cluster::GetReplicas(const std::string &node_id) { if (version_ < 0) { - return {Status::ClusterDown, errClusterNoInitialized}; + return {Status::RedisClusterDown, errClusterNoInitialized}; } auto item = nodes_.find(node_id); @@ -821,13 +821,13 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons int cur_slot = GetSlotIdFromKey(cmd_tokens[i]); if (slot == -1) slot = cur_slot; if (slot != cur_slot) { - return {redis::ErrorKind::CrossSlot, "Attempted to access keys that don't hash to the same slot"}; + return {Status::RedisCrossSlot, "Attempted to access keys that don't hash to the same slot"}; } } if (slot == -1) return Status::OK(); if (slots_nodes_[slot] == nullptr) { - return {redis::ErrorKind::ClusterDown, "Hash slot not served"}; + return {Status::RedisClusterDown, "Hash slot not served"}; } if (myself_ && myself_ == slots_nodes_[slot]) { @@ -835,12 +835,12 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons // Server can't change the topology directly, so we record the migrated slots // to move the requests of the migrated slots to the destination node. if (migrated_slots_.count(slot) > 0) { // I'm not serving the migrated slot - return {redis::ErrorKind::Moved, fmt::format("{} {}", slot, migrated_slots_[slot])}; + return {Status::RedisMoved, fmt::format("{} {}", slot, migrated_slots_[slot])}; } // To keep data consistency, slot will be forbidden write while sending the last incremental data. // During this phase, the requests of the migrating slot has to be rejected. if ((attributes->flags & redis::kCmdWrite) && IsWriteForbiddenSlot(slot)) { - return {redis::ErrorKind::TryAgain, "Can't write to slot being migrated which is in write forbidden phase"}; + return {Status::RedisTryAgain, "Can't write to slot being migrated which is in write forbidden phase"}; } return Status::OK(); // I'm serving this slot @@ -868,7 +868,7 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons return Status::OK(); // My master is serving this slot } - return {redis::ErrorKind::Moved, fmt::format("{} {}:{}", slot, slots_nodes_[slot]->host, slots_nodes_[slot]->port)}; + return {Status::RedisMoved, fmt::format("{} {}:{}", slot, slots_nodes_[slot]->host, slots_nodes_[slot]->port)}; } // Only HARD mode is meaningful to the Kvrocks cluster, diff --git a/src/cluster/cluster_defs.h b/src/cluster/cluster_defs.h index 6638db8807e..0178ac75884 100644 --- a/src/cluster/cluster_defs.h +++ b/src/cluster/cluster_defs.h @@ -36,7 +36,7 @@ inline constexpr const char *errSlotOutOfRange = "Slot is out of range"; inline constexpr const char *errInvalidClusterVersion = "Invalid cluster version"; inline constexpr const char *errSlotOverlapped = "Slot distribution is overlapped"; inline constexpr const char *errNoMasterNode = "The node isn't a master"; -inline constexpr const char *errClusterNoInitialized = "CLUSTERDOWN The cluster is not initialized"; +inline constexpr const char *errClusterNoInitialized = "The cluster is not initialized"; inline constexpr const char *errInvalidClusterNodeInfo = "Invalid cluster nodes info"; inline constexpr const char *errInvalidImportState = "Invalid import state"; diff --git a/src/commands/cmd_bloom_filter.cc b/src/commands/cmd_bloom_filter.cc index 1975dd0e52f..bb2df94e2f5 100644 --- a/src/commands/cmd_bloom_filter.cc +++ b/src/commands/cmd_bloom_filter.cc @@ -119,7 +119,7 @@ class CommandBFAdd : public Commander { *output = redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output = redis::Error({ErrorKind::Err, errFilterFull}); + *output = redis::Error({Status::NotOK, errFilterFull}); break; } return Status::OK(); @@ -152,7 +152,7 @@ class CommandBFMAdd : public Commander { *output += redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output += redis::Error({ErrorKind::Err, errFilterFull}); + *output += redis::Error({Status::NotOK, errFilterFull}); break; } } @@ -248,7 +248,7 @@ class CommandBFInsert : public Commander { *output += redis::Integer(0); break; case BloomFilterAddResult::kFull: - *output += redis::Error({ErrorKind::Err, errFilterFull}); + *output += redis::Error({Status::NotOK, errFilterFull}); break; } } diff --git a/src/commands/cmd_cluster.cc b/src/commands/cmd_cluster.cc index 8ada4e8eec0..0f9b17603fc 100644 --- a/src/commands/cmd_cluster.cc +++ b/src/commands/cmd_cluster.cc @@ -90,7 +90,7 @@ class CommandCluster : public Commander { } } } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else if (subcommand_ == "nodes") { std::string nodes_desc; @@ -98,7 +98,7 @@ class CommandCluster : public Commander { if (s.IsOK()) { *output = conn->VerbatimString("txt", nodes_desc); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else if (subcommand_ == "info") { std::string cluster_info; @@ -106,21 +106,21 @@ class CommandCluster : public Commander { if (s.IsOK()) { *output = conn->VerbatimString("txt", cluster_info); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else if (subcommand_ == "import") { Status s = srv->cluster->ImportSlot(conn, static_cast(slot_), state_); if (s.IsOK()) { *output = redis::SimpleString("OK"); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else if (subcommand_ == "reset") { Status s = srv->cluster->Reset(); if (s.IsOK()) { *output = redis::SimpleString("OK"); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else if (subcommand_ == "replicas") { auto node_id = args_[2]; @@ -128,7 +128,7 @@ class CommandCluster : public Commander { if (s.IsOK()) { *output = conn->VerbatimString("txt", s.GetValue()); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else { return {Status::RedisExecErr, "Invalid cluster command options"}; @@ -252,7 +252,7 @@ class CommandClusterX : public Commander { need_persist_nodes_info = true; *output = redis::SimpleString("OK"); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else if (subcommand_ == "setnodeid") { Status s = srv->cluster->SetNodeId(args_[2]); @@ -260,7 +260,7 @@ class CommandClusterX : public Commander { need_persist_nodes_info = true; *output = redis::SimpleString("OK"); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else if (subcommand_ == "setslot") { Status s = srv->cluster->SetSlotRanges(slot_ranges_, args_[4], set_version_); @@ -268,7 +268,7 @@ class CommandClusterX : public Commander { need_persist_nodes_info = true; *output = redis::SimpleString("OK"); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else if (subcommand_ == "version") { int64_t v = srv->cluster->GetVersion(); @@ -287,7 +287,7 @@ class CommandClusterX : public Commander { } *output = redis::SimpleString("OK"); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else { return {Status::RedisExecErr, "Invalid cluster command options"}; diff --git a/src/commands/cmd_hash.cc b/src/commands/cmd_hash.cc index 6db97f89025..677f131eb98 100644 --- a/src/commands/cmd_hash.cc +++ b/src/commands/cmd_hash.cc @@ -326,16 +326,11 @@ class CommandHRangeByLex : public Commander { return parser.InvalidSyntax(); } } - Status s; if (spec_.reversed) { - s = ParseRangeLexSpec(args[3], args[2], &spec_); + return ParseRangeLexSpec(args[3], args[2], &spec_); } else { - s = ParseRangeLexSpec(args[2], args[3], &spec_); + return ParseRangeLexSpec(args[2], args[3], &spec_); } - if (!s.IsOK()) { - return {Status::RedisParseErr, s.Msg()}; - } - return Status::OK(); } Status Execute(Server *srv, Connection *conn, std::string *output) override { diff --git a/src/commands/cmd_key.cc b/src/commands/cmd_key.cc index a6400d4a774..d7219841a80 100644 --- a/src/commands/cmd_key.cc +++ b/src/commands/cmd_key.cc @@ -480,7 +480,7 @@ class CommandSort : public Commander { } if (type != RedisType::kRedisList && type != RedisType::kRedisSet && type != RedisType::kRedisZSet) { - return {ErrorKind::WrongType, "Operation against a key holding the wrong kind of value"}; + return {Status::RedisWrongType, "Operation against a key holding the wrong kind of value"}; } /* When sorting a set with no sort specified, we must sort the output @@ -507,11 +507,11 @@ class CommandSort : public Commander { switch (res) { case Database::SortResult::UNKNOWN_TYPE: - return {ErrorKind::None, "Unknown Type"}; + return {Status::RedisErrorNoPrefix, "Unknown Type"}; case Database::SortResult::DOUBLE_CONVERT_ERROR: - return {ErrorKind::None, "One or more scores can't be converted into double"}; + return {Status::RedisErrorNoPrefix, "One or more scores can't be converted into double"}; case Database::SortResult::LIMIT_EXCEEDED: - return {ErrorKind::None, + return {Status::RedisErrorNoPrefix, "The number of elements to be sorted exceeds SORT_LENGTH_LIMIT = " + std::to_string(SORT_LENGTH_LIMIT)}; case Database::SortResult::DONE: if (sort_argument_.storekey.empty()) { diff --git a/src/commands/cmd_list.cc b/src/commands/cmd_list.cc index 3ddb92eea66..e9e17266625 100644 --- a/src/commands/cmd_list.cc +++ b/src/commands/cmd_list.cc @@ -304,7 +304,7 @@ class CommandBPop : public BlockingCommander { conn_->Reply(conn_->MultiBulkString({*last_key_ptr, std::move(elem)})); } } else if (!s.IsNotFound()) { - conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); + conn_->Reply(redis::Error({Status::NotOK, s.ToString()})); } return s; @@ -414,7 +414,7 @@ class CommandBLMPop : public BlockingCommander { conn_->Reply(redis::Array({redis::BulkString(chosen_key), std::move(elems_bulk)})); } } else if (!s.IsNotFound()) { - conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); + conn_->Reply(redis::Error({Status::NotOK, s.ToString()})); } return s; @@ -757,7 +757,7 @@ class CommandBLMove : public BlockingCommander { std::string elem; auto s = list_db.LMove(args_[1], args_[2], src_left_, dst_left_, &elem); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); + conn_->Reply(redis::Error({Status::NotOK, s.ToString()})); return true; } diff --git a/src/commands/cmd_replication.cc b/src/commands/cmd_replication.cc index f94d55d1d5b..d3f3c0f25c5 100644 --- a/src/commands/cmd_replication.cc +++ b/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({ErrorKind::None, "can't create db checkpoint"}), bev); + s = util::SockSend(repl_fd, redis::Error({Status::RedisErrorNoPrefix, "can't create db checkpoint"}), bev); if (!s.IsOK()) { LOG(WARNING) << "[replication] Failed to send error response: " << s.Msg(); } diff --git a/src/commands/cmd_script.cc b/src/commands/cmd_script.cc index 322fe92162e..2547c289fcf 100644 --- a/src/commands/cmd_script.cc +++ b/src/commands/cmd_script.cc @@ -31,7 +31,7 @@ class CommandEvalImpl : public Commander { public: Status Execute(Server *srv, Connection *conn, std::string *output) override { if (evalsha && args_[1].size() != 40) { - return {ErrorKind::NoScript, errNoMatchingScript}; + return {Status::RedisNoScript, errNoMatchingScript}; } int64_t numkeys = GET_OR_RET(ParseInt(args_[2], 10)); diff --git a/src/commands/cmd_server.cc b/src/commands/cmd_server.cc index a54a2d06e4a..d595875d7bb 100644 --- a/src/commands/cmd_server.cc +++ b/src/commands/cmd_server.cc @@ -224,7 +224,7 @@ class CommandConfig : public Commander { if (args_.size() == 2 && sub_command == "rewrite") { Status s = config->Rewrite(srv->GetNamespace()->List()); - if (!s.IsOK()) return {Status::RedisExecErr, s.Msg()}; + if (!s.IsOK()) return s; *output = redis::SimpleString("OK"); LOG(INFO) << "# CONFIG REWRITE executed with success"; @@ -315,7 +315,7 @@ class CommandDBSize : public Commander { if (s.IsOK()) { *output = redis::SimpleString("OK"); } else { - return {Status::RedisExecErr, s.Msg()}; + return s; } } else { return {Status::RedisExecErr, "DBSIZE subcommand only supports scan"}; @@ -630,7 +630,7 @@ class CommandDebug : public Commander { } else if (protocol_type_ == "verbatim") { // verbatim string *output = conn->VerbatimString("txt", "verbatim string"); } else { - return {ErrorKind::None, + return {Status::RedisErrorNoPrefix, "Wrong protocol type name. Please use one of the following: " "string|integer|double|array|set|bignum|true|false|null|attrib|verbatim"}; } @@ -741,7 +741,7 @@ class CommandHello final : public Commander { // kvrocks only supports REPL2 by now, but for supporting some // `hello 3`, it will not report error when using 3. if (protocol < 2 || protocol > 3) { - return {ErrorKind::NoProto, "unsupported protocol version"}; + return {Status::RedisNoProto, "unsupported protocol version"}; } } @@ -985,9 +985,7 @@ class CommandSlaveOf : public Commander { } auto s = IsTryingToReplicateItself(srv, host_, port_); - if (!s.IsOK()) { - return {Status::RedisExecErr, s.Msg()}; - } + if (!s.IsOK()) return s; s = srv->AddMaster(host_, port_, false); if (s.IsOK()) { *output = redis::SimpleString("OK"); @@ -1100,7 +1098,7 @@ class CommandRestore : public Commander { auto stream_ptr = std::make_unique(args_[3]); RDB rdb(srv->storage, conn->GetNamespace(), std::move(stream_ptr)); auto s = rdb.Restore(args_[1], args_[3], ttl_ms_); - if (!s.IsOK()) return {Status::RedisExecErr, s.Msg()}; + if (!s.IsOK()) return s; *output = redis::SimpleString("OK"); return Status::OK(); } @@ -1214,9 +1212,8 @@ class CommandApplyBatch : public Commander { auto options = svr->storage->DefaultWriteOptions(); options.low_pri = low_pri_; auto s = svr->storage->ApplyWriteBatch(options, std::move(raw_batch_)); - if (!s.IsOK()) { - return {Status::RedisExecErr, s.Msg()}; - } + if (!s.IsOK()) return s; + *output = redis::Integer(size); return Status::OK(); } @@ -1261,7 +1258,7 @@ class CommandDump : public Commander { auto stream_ptr = std::make_unique(result); RDB rdb(srv->storage, conn->GetNamespace(), std::move(stream_ptr)); auto s = rdb.Dump(key, type); - if (!s.IsOK()) return {Status::RedisExecErr, s.Msg()}; + if (!s.IsOK()) return s; CHECK(dynamic_cast(rdb.GetStream().get()) != nullptr); *output = redis::BulkString(static_cast(rdb.GetStream().get())->GetInput()); return Status::OK(); diff --git a/src/commands/cmd_stream.cc b/src/commands/cmd_stream.cc index 580c6a278ad..ba4f7d9870c 100644 --- a/src/commands/cmd_stream.cc +++ b/src/commands/cmd_stream.cc @@ -39,9 +39,7 @@ class CommandXAck : public Commander { StreamEntryID tmp_id; for (size_t i = 3; i < args.size(); ++i) { auto s = ParseStreamEntryID(args[i], &tmp_id); - if (!s.IsOK()) { - return {Status::RedisParseErr, s.Msg()}; - } + if (!s.IsOK()) return s; entry_ids_.emplace_back(tmp_id); } @@ -130,9 +128,7 @@ class CommandXAdd : public Commander { } auto s = ParseStreamEntryID(args[min_id_idx], &min_id_); - if (!s.IsOK()) { - return {Status::RedisParseErr, s.Msg()}; - } + if (!s.IsOK()) return s; with_min_id_ = true; i += eq_sign_found ? 3 : 2; @@ -145,9 +141,7 @@ class CommandXAdd : public Commander { if (!entry_id_found) { auto result = ParseNextStreamEntryIDStrategy(val); - if (!result.IsOK()) { - return {Status::RedisParseErr, result.Msg()}; - } + if (!result.IsOK()) return result; next_id_strategy_ = std::move(*result); @@ -1130,7 +1124,7 @@ class CommandXRead : public Commander, std::vector result; auto s = stream_db.Range(streams_[i], options, &result); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); + conn_->Reply(redis::Error({Status::NotOK, s.ToString()})); return; } @@ -1423,7 +1417,7 @@ class CommandXReadGroup : public Commander, auto s = stream_db.RangeWithPending(streams_[i], options, &result, group_name_, consumer_name_, noack_, latest_marks_[i]); if (!s.ok() && !s.IsNotFound()) { - conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); + conn_->Reply(redis::Error({Status::NotOK, s.ToString()})); return; } @@ -1599,9 +1593,7 @@ class CommandXSetId : public Commander { stream_name_ = args[1]; auto s = redis::ParseStreamEntryID(args[2], &last_id_); - if (!s.IsOK()) { - return {Status::RedisParseErr, s.Msg()}; - } + if (!s.IsOK()) return s; if (args.size() == 3) { return Status::OK(); @@ -1619,9 +1611,7 @@ class CommandXSetId : public Commander { } else if (util::EqualICase(args[i], "maxdeletedid") && i + 1 < args.size()) { StreamEntryID id; s = redis::ParseStreamEntryID(args[i + 1], &id); - if (!s.IsOK()) { - return {Status::RedisParseErr, s.Msg()}; - } + if (!s.IsOK()) return s; max_deleted_id_ = std::make_optional(id.ms, id.seq); i += 2; diff --git a/src/commands/cmd_txn.cc b/src/commands/cmd_txn.cc index 57332dbcff8..130533fbc9b 100644 --- a/src/commands/cmd_txn.cc +++ b/src/commands/cmd_txn.cc @@ -68,7 +68,7 @@ class CommandExec : public Commander { auto reset_multiexec = MakeScopeExit([conn] { conn->ResetMultiExec(); }); if (conn->IsMultiError()) { - return {ErrorKind::ExecAbort, "Transaction discarded"}; + return {Status::RedisExecAbort, "Transaction discarded"}; } if (srv->IsWatchedKeysModified(conn)) { diff --git a/src/commands/cmd_zset.cc b/src/commands/cmd_zset.cc index 631cdd091c1..715747ab8a4 100644 --- a/src/commands/cmd_zset.cc +++ b/src/commands/cmd_zset.cc @@ -136,9 +136,7 @@ class CommandZCount : public Commander { public: Status Parse(const std::vector &args) override { Status s = ParseRangeScoreSpec(args[2], args[3], &spec_); - if (!s.IsOK()) { - return {Status::RedisParseErr, s.Msg()}; - } + if (!s.IsOK()) return s; return Commander::Parse(args); } @@ -204,9 +202,7 @@ class CommandZLexCount : public Commander { public: Status Parse(const std::vector &args) override { Status s = ParseRangeLexSpec(args[2], args[3], &spec_); - if (!s.IsOK()) { - return {Status::RedisParseErr, s.Msg()}; - } + if (!s.IsOK()) return s; return Commander::Parse(args); } @@ -369,7 +365,7 @@ class CommandBZPop : public BlockingCommander { redis::ZSet zset_db(srv_->storage, conn_->GetNamespace()); auto s = PopFromMultipleZsets(&zset_db, keys_, min_, 1, &user_key, &member_scores); if (!s.ok()) { - conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); + conn_->Reply(redis::Error({Status::NotOK, s.ToString()})); return true; } @@ -548,7 +544,7 @@ class CommandBZMPop : public BlockingCommander { redis::ZSet zset_db(srv_->storage, conn_->GetNamespace()); auto s = PopFromMultipleZsets(&zset_db, keys_, flag_ == ZSET_MIN, count_, &user_key, &member_scores); if (!s.ok()) { - conn_->Reply(redis::Error({ErrorKind::Err, s.ToString()})); + conn_->Reply(redis::Error({Status::NotOK, s.ToString()})); return true; } @@ -985,9 +981,7 @@ class CommandZRemRangeByScore : public Commander { public: Status Parse(const std::vector &args) override { Status s = ParseRangeScoreSpec(args[2], args[3], &spec_); - if (!s.IsOK()) { - return {Status::RedisParseErr, s.Msg()}; - } + if (!s.IsOK()) return s; return Commander::Parse(args); } @@ -1014,9 +1008,7 @@ class CommandZRemRangeByLex : public Commander { public: Status Parse(const std::vector &args) override { Status s = ParseRangeLexSpec(args[2], args[3], &spec_); - if (!s.IsOK()) { - return {Status::RedisParseErr, s.Msg()}; - } + if (!s.IsOK()) return s; return Commander::Parse(args); } diff --git a/src/common/redis_error.h b/src/common/redis_error.h deleted file mode 100644 index 6def31be006..00000000000 --- a/src/common/redis_error.h +++ /dev/null @@ -1,58 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - * - */ - -#pragma once - -#include - -namespace redis { - -enum class ErrorKind { - None, - Err, - WrongType, - NoScript, - NoProto, - NoAuth, - Loading, - Readonly, - MasterDown, - ExecAbort, - Moved, - TryAgain, - ClusterDown, - CrossSlot, -}; - -const std::map ErrorKindMap = {{ErrorKind::Err, "ERR"}, - {ErrorKind::WrongType, "WRONGTYPE"}, - {ErrorKind::NoScript, "NOSCRIPT"}, - {ErrorKind::NoProto, "NOPROTO"}, - {ErrorKind::NoAuth, "NOAUTH"}, - {ErrorKind::Loading, "LOADING"}, - {ErrorKind::Readonly, "READONLY"}, - {ErrorKind::MasterDown, "MASTERDOWN"}, - {ErrorKind::ExecAbort, "EXECABORT"}, - {ErrorKind::Moved, "MOVED"}, - {ErrorKind::TryAgain, "TRYAGAIN"}, - {ErrorKind::ClusterDown, "CLUSTERDOWN"}, - {ErrorKind::CrossSlot, "CROSSSLOT"}}; - -} // namespace redis diff --git a/src/common/status.h b/src/common/status.h index d528b43ac02..d06c2f06014 100644 --- a/src/common/status.h +++ b/src/common/status.h @@ -30,7 +30,6 @@ #include #include -#include "redis_error.h" #include "type_util.h" class [[nodiscard]] Status { @@ -47,14 +46,25 @@ class [[nodiscard]] Status { DBGetWALErr, // Redis - RedisError, RedisUnknownCmd, RedisInvalidCmd, RedisParseErr, RedisExecErr, + RedisErrorNoPrefix, + RedisNoProto, + RedisLoading, + RedisMasterDown, + RedisNoScript, + RedisNoAuth, + RedisWrongType, + RedisReadOnly, + RedisExecAbort, + RedisMoved, + RedisCrossSlot, + RedisTryAgain, + RedisClusterDown, // Cluster - ClusterDown, ClusterInvalidInfo, // Blocking @@ -70,7 +80,6 @@ class [[nodiscard]] Status { Status(Code code, std::string msg = {}) : impl_{new Impl{code, std::move(msg)}} { // NOLINT CHECK(code != cOK); } - Status(redis::ErrorKind kind, std::string msg) : impl_{new Impl{RedisError, std::move(msg), kind}} {} Status(const Status& s) : impl_{s.impl_ ? new Impl{s.impl_->code, s.impl_->msg} : nullptr} {} Status(Status&&) = default; @@ -92,7 +101,6 @@ class [[nodiscard]] Status { explicit operator bool() const { return IsOK(); } Code GetCode() const { return impl_ ? impl_->code : cOK; } - redis::ErrorKind GetErrorKind() const { return impl_ ? impl_->kind : redis::ErrorKind::None; } std::string Msg() const& { if (*this) return ok_msg; @@ -124,9 +132,6 @@ class [[nodiscard]] Status { struct Impl { Code code; std::string msg; - // Error kind is used to distinguish different types of Redis errors, - // it only takes effect when code is RedisError. - redis::ErrorKind kind = redis::ErrorKind::None; }; std::unique_ptr impl_; diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc index fe4f8fe43bd..0457c71d57e 100644 --- a/src/server/redis_connection.cc +++ b/src/server/redis_connection.cc @@ -432,7 +432,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { auto cmd_s = Server::LookupAndCreateCommand(cmd_tokens.front()); if (!cmd_s.IsOK()) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error({ErrorKind::Err, "unknown command " + cmd_tokens.front()})); + Reply(redis::Error({Status::NotOK, "unknown command " + cmd_tokens.front()})); continue; } auto current_cmd = std::move(*cmd_s); @@ -444,7 +444,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { if (GetNamespace().empty()) { if (!password.empty()) { if (cmd_name != "auth" && cmd_name != "hello") { - Reply(redis::Error({ErrorKind::NoAuth, "Authentication required."})); + Reply(redis::Error({Status::RedisNoAuth, "Authentication required."})); continue; } } else { @@ -477,7 +477,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (srv_->IsLoading() && !(cmd_flags & kCmdLoading)) { - Reply(redis::Error({ErrorKind::Loading, errRestoringBackup})); + Reply(redis::Error({Status::RedisLoading, errRestoringBackup})); if (is_multi_exec) multi_error_ = true; continue; } @@ -485,7 +485,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { int tokens = static_cast(cmd_tokens.size()); if (!attributes->CheckArity(tokens)) { if (is_multi_exec) multi_error_ = true; - Reply(redis::Error({ErrorKind::Err, "wrong number of arguments"})); + Reply(redis::Error({Status::NotOK, "wrong number of arguments"})); continue; } @@ -498,7 +498,7 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (is_multi_exec && (cmd_flags & kCmdNoMulti)) { - Reply(redis::Error({ErrorKind::Err, "Can't execute " + cmd_name + " in MULTI"})); + Reply(redis::Error({Status::NotOK, "Can't execute " + cmd_name + " in MULTI"})); multi_error_ = true; continue; } @@ -525,18 +525,18 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } if (config->slave_readonly && srv_->IsSlave() && (cmd_flags & kCmdWrite)) { - Reply(redis::Error({ErrorKind::Readonly, "You can't write against a read only slave."})); + Reply(redis::Error({Status::RedisReadOnly, "You can't write against a read only slave."})); continue; } if ((cmd_flags & kCmdWrite) && !(cmd_flags & kCmdNoDBSizeCheck) && srv_->storage->ReachedDBSizeLimit()) { - Reply(redis::Error({ErrorKind::Err, "write command not allowed when reached max-db-size."})); + Reply(redis::Error({Status::NotOK, "write command not allowed when reached max-db-size."})); continue; } if (!config->slave_serve_stale_data && srv_->IsSlave() && cmd_name != "info" && cmd_name != "slaveof" && srv_->GetReplicationState() != kReplConnected) { - Reply(redis::Error({ErrorKind::MasterDown, + Reply(redis::Error({Status::RedisMasterDown, "Link with MASTER is down " "and slave-serve-stale-data is set to 'no'."})); continue; diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index 8e5cbaf2054..0a64ae8bf50 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -20,8 +20,18 @@ #include "redis_reply.h" +#include #include +const std::map redisErrorPrefixMapping = { + {Status::RedisErrorNoPrefix, ""}, {Status::RedisNoProto, "NOPROTO"}, + {Status::RedisLoading, "LOADING"}, {Status::RedisMasterDown, "MASTERDOWN"}, + {Status::RedisNoScript, "NOSCRIPT"}, {Status::RedisNoAuth, "NOAUTH"}, + {Status::RedisWrongType, "WRONGTYPE"}, {Status::RedisReadOnly, "READONLY"}, + {Status::RedisExecAbort, "EXECABORT"}, {Status::RedisMoved, "MOVED"}, + {Status::RedisCrossSlot, "CROSSSLOT"}, {Status::RedisTryAgain, "TRYAGAIN"}, + {Status::RedisClusterDown, "CLUSTERDOWN"}}; + namespace redis { void Reply(evbuffer *output, const std::string &data) { evbuffer_add(output, data.c_str(), data.length()); } @@ -32,13 +42,14 @@ std::string Error(const Status &s) { return RESP_PREFIX_ERROR + StatusToRedisErr std::string StatusToRedisError(const Status &s) { CHECK(!s.IsOK()); - if (!s.Is()) { - return "ERR " + s.Msg() + CRLF; + std::string prefix = "ERR"; + if (auto it = redisErrorPrefixMapping.find(s.GetCode()); it != redisErrorPrefixMapping.end()) { + prefix = it->second; } - if (auto iter = ErrorKindMap.find(s.GetErrorKind()); iter != ErrorKindMap.end()) { - return iter->second + " " + s.Msg() + CRLF; + if (!prefix.empty()) { + prefix = prefix + " "; } - return s.Msg() + CRLF; + return prefix + s.Msg() + CRLF; } std::string BulkString(const std::string &data) { return "$" + std::to_string(data.length()) + CRLF + data + CRLF; } diff --git a/src/server/worker.cc b/src/server/worker.cc index 15d2b5ed044..22054e1faf8 100644 --- a/src/server/worker.cc +++ b/src/server/worker.cc @@ -167,7 +167,7 @@ void Worker::newTCPConnection(evconnlistener *listener, evutil_socket_t fd, sock s = AddConnection(conn); if (!s.IsOK()) { - std::string err_msg = redis::Error({redis::ErrorKind::Err, s.Msg()}); + std::string err_msg = redis::Error({Status::NotOK, s.Msg()}); s = util::SockSend(fd, err_msg, ssl); if (!s.IsOK()) { LOG(WARNING) << "Failed to send error response to socket: " << s.Msg(); diff --git a/src/storage/rdb.cc b/src/storage/rdb.cc index f513caca698..31c56f00159 100644 --- a/src/storage/rdb.cc +++ b/src/storage/rdb.cc @@ -688,9 +688,9 @@ Status RDB::Dump(const std::string &key, const RedisType type) { /* Serialize the object in an RDB-like format. It consist of an object type * byte followed by the serialized object. This is understood by RESTORE. */ auto s = SaveObjectType(type); - if (!s.IsOK()) return {Status::RedisExecErr, s.Msg()}; + if (!s.IsOK()) return s; s = SaveObject(key, type); - if (!s.IsOK()) return {Status::RedisExecErr, s.Msg()}; + if (!s.IsOK()) return s; /* Write the footer, this is how it looks like: * ----------------+---------------------+---------------+ @@ -705,21 +705,14 @@ Status RDB::Dump(const std::string &key, const RedisType type) { buf[0] = MinRDBVersion & 0xff; buf[1] = (MinRDBVersion >> 8) & 0xff; s = stream_->Write((const char *)buf, 2); - if (!s.IsOK()) { - return {Status::RedisExecErr, s.Msg()}; - } + if (!s.IsOK()) return s; /* CRC64 */ CHECK(dynamic_cast(stream_.get()) != nullptr); std::string &output = static_cast(stream_.get())->GetInput(); uint64_t crc = crc64(0, (unsigned char *)(output.c_str()), output.length()); memrev64ifbe(&crc); - s = stream_->Write((const char *)(&crc), 8); - if (!s.IsOK()) { - return {Status::RedisExecErr, s.Msg()}; - } - - return Status::OK(); + return stream_->Write((const char *)(&crc), 8); } Status RDB::SaveObjectType(const RedisType type) { @@ -797,48 +790,28 @@ Status RDB::RdbSaveLen(uint64_t len) { if (len < (1 << 6)) { /* Save a 6 bit len */ buf[0] = (len & 0xFF) | (RDB6BitLen << 6); - auto status = stream_->Write((const char *)buf, 1); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } - return Status::OK(); + return stream_->Write((const char *)buf, 1); } else if (len < (1 << 14)) { /* Save a 14 bit len */ buf[0] = ((len >> 8) & 0xFF) | (RDB14BitLen << 6); buf[1] = len & 0xFF; - auto status = stream_->Write((const char *)buf, 2); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } - return Status::OK(); + return stream_->Write((const char *)buf, 2); } else if (len <= UINT32_MAX) { /* Save a 32 bit len */ buf[0] = RDB32BitLen; auto status = stream_->Write((const char *)buf, 1); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; uint32_t len32 = htonl(len); - status = stream_->Write((const char *)(&len32), 4); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } - return Status::OK(); + return stream_->Write((const char *)(&len32), 4); } else { /* Save a 64 bit len */ buf[0] = RDB64BitLen; auto status = stream_->Write((const char *)buf, 1); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; len = htonu64(len); - status = stream_->Write((const char *)(&len), 8); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } - return Status::OK(); + return stream_->Write((const char *)(&len), 8); } } @@ -857,11 +830,7 @@ Status RDB::SaveStringObject(const std::string &value) { // encode integer enclen = rdbEncodeInteger(integer_value, buf); if (enclen > 0) { - auto status = stream_->Write((const char *)buf, enclen); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } - return Status::OK(); + return stream_->Write((const char *)buf, enclen); } } } @@ -871,14 +840,9 @@ Status RDB::SaveStringObject(const std::string &value) { /* Store verbatim */ auto status = RdbSaveLen(value.length()); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; if (value.length() > 0) { - status = stream_->Write(value.c_str(), value.length()); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + return stream_->Write(value.c_str(), value.length()); } return Status::OK(); } @@ -886,15 +850,11 @@ Status RDB::SaveStringObject(const std::string &value) { Status RDB::SaveListObject(const std::vector &elems) { if (elems.size() > 0) { auto status = RdbSaveLen(elems.size()); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; for (const auto &elem : elems) { auto status = rdbSaveZipListObject(elem); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; } } else { LOG(WARNING) << "the size of elems is zero"; @@ -906,15 +866,11 @@ Status RDB::SaveListObject(const std::vector &elems) { Status RDB::SaveSetObject(const std::vector &members) { if (members.size() > 0) { auto status = RdbSaveLen(members.size()); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; for (const auto &elem : members) { status = SaveStringObject(elem); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; } } else { LOG(WARNING) << "the size of elems is zero"; @@ -926,20 +882,14 @@ Status RDB::SaveSetObject(const std::vector &members) { Status RDB::SaveZSetObject(const std::vector &member_scores) { if (member_scores.size() > 0) { auto status = RdbSaveLen(member_scores.size()); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; for (const auto &elem : member_scores) { status = SaveStringObject(elem.member); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; status = rdbSaveBinaryDoubleValue(elem.score); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; } } else { LOG(WARNING) << "the size of member_scores is zero"; @@ -951,20 +901,14 @@ Status RDB::SaveZSetObject(const std::vector &member_scores) { Status RDB::SaveHashObject(const std::vector &field_values) { if (field_values.size() > 0) { auto status = RdbSaveLen(field_values.size()); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; for (const auto &p : field_values) { status = SaveStringObject(p.field); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; status = SaveStringObject(p.value); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } + if (!status.IsOK()) return status; } } else { LOG(WARNING) << "the size of field_values is zero"; @@ -1024,10 +968,5 @@ Status RDB::rdbSaveZipListObject(const std::string &elem) { ZipList::SetZipListLength(zl_ptr, ziplist_size, 1); zl_ptr[ziplist_size - 1] = zlEnd; - auto status = SaveStringObject(zl_string); - if (!status.IsOK()) { - return {Status::RedisExecErr, status.Msg()}; - } - - return Status::OK(); + return SaveStringObject(zl_string); } diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc index 3bc1af9867e..4bc68d73e6b 100644 --- a/src/storage/scripting.cc +++ b/src/storage/scripting.cc @@ -611,7 +611,7 @@ Status EvalGenericCommand(redis::Connection *conn, const std::string &body_or_sh auto s = srv->ScriptGet(funcname + 2, &body); if (!s.IsOK()) { lua_pop(lua, 1); /* remove the error handler from the stack. */ - return {redis::ErrorKind::NoScript, redis::errNoMatchingScript}; + return {Status::RedisNoScript, redis::errNoMatchingScript}; } } else { body = body_or_sha; @@ -640,7 +640,7 @@ Status EvalGenericCommand(redis::Connection *conn, const std::string &body_or_sh if (lua_pcall(lua, 0, 1, -2)) { auto msg = fmt::format("running script (call to {}): {}", funcname, lua_tostring(lua, -1)); - *output = redis::Error({redis::ErrorKind::Err, msg}); + *output = redis::Error({Status::NotOK, msg}); lua_pop(lua, 2); } else { *output = ReplyToRedisReply(conn, lua); @@ -1191,7 +1191,7 @@ std::string ReplyToRedisReply(redis::Connection *conn, lua_State *lua) { lua_rawget(lua, -2); t = lua_type(lua, -1); if (t == LUA_TSTRING) { - output = redis::Error({redis::ErrorKind::None, lua_tostring(lua, -1)}); + output = redis::Error({Status::RedisErrorNoPrefix, lua_tostring(lua, -1)}); lua_pop(lua, 1); return output; } diff --git a/src/types/json.h b/src/types/json.h index 0ce88797295..13abb5cc954 100644 --- a/src/types/json.h +++ b/src/types/json.h @@ -238,7 +238,7 @@ struct JsonValue { } catch (const jsoncons::jsonpath::jsonpath_error &e) { return {Status::NotOK, e.what()}; } - if (!s) return {Status::NotOK, s.Msg()}; + if (!s) return s; return results; } From 36849503d14c7fb2ce41f437b8b3b338b81c1947 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Sat, 15 Jun 2024 10:00:02 +0800 Subject: [PATCH 09/11] Fix error message --- src/cluster/replication.cc | 6 +++--- src/commands/error_constants.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cluster/replication.cc b/src/cluster/replication.cc index a3567284091..9db926b3c8b 100644 --- a/src/cluster/replication.cc +++ b/src/cluster/replication.cc @@ -1000,15 +1000,15 @@ Status ReplicationThread::parseWriteBatch(const std::string &batch_string) { } bool ReplicationThread::isRestoringError(std::string_view err) { - return err == std::string(RESP_PREFIX_ERROR) + redis::errRestoringBackup; + return err == redis::Error({Status::RedisLoading, redis::errRestoringBackup}); } bool ReplicationThread::isWrongPsyncNum(std::string_view err) { - return err == std::string(RESP_PREFIX_ERROR) + redis::errWrongNumArguments; + return err == redis::Error({Status::NotOK, redis::errWrongNumArguments}); } bool ReplicationThread::isUnknownOption(std::string_view err) { - return err == fmt::format("{}ERR {}", RESP_PREFIX_ERROR, redis::errUnknownOption); + return err == redis::Error({Status::NotOK, redis::errUnknownOption}); } rocksdb::Status WriteBatchHandler::PutCF(uint32_t column_family_id, const rocksdb::Slice &key, diff --git a/src/commands/error_constants.h b/src/commands/error_constants.h index 08c107ad6f6..ea2c38b72c0 100644 --- a/src/commands/error_constants.h +++ b/src/commands/error_constants.h @@ -43,7 +43,7 @@ inline constexpr const char *errValueIsNotFloat = "value is not a valid float"; 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"; +inline constexpr const char *errWrongNumArguments = "wrong number of arguments"; inline constexpr const char *errRestoringBackup = "kvrocks is restoring the db from backup"; } // namespace redis From 60cd76bec485d2ca3cd497e89d24abfaa5ee81e4 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Sat, 15 Jun 2024 10:28:40 +0800 Subject: [PATCH 10/11] Fix wrong error message --- src/cluster/replication.cc | 9 ++++++--- src/server/redis_reply.cc | 8 ++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/cluster/replication.cc b/src/cluster/replication.cc index 9db926b3c8b..5dd1fedb15e 100644 --- a/src/cluster/replication.cc +++ b/src/cluster/replication.cc @@ -1000,15 +1000,18 @@ Status ReplicationThread::parseWriteBatch(const std::string &batch_string) { } bool ReplicationThread::isRestoringError(std::string_view err) { - return err == redis::Error({Status::RedisLoading, redis::errRestoringBackup}); + // err doesn't contain the CRLF, so cannot use redis::Error here. + return err == RESP_PREFIX_ERROR + redis::StatusToRedisError({Status::RedisLoading, redis::errRestoringBackup}); } bool ReplicationThread::isWrongPsyncNum(std::string_view err) { - return err == redis::Error({Status::NotOK, redis::errWrongNumArguments}); + // err doesn't contain the CRLF, so cannot use redis::Error here. + return err == RESP_PREFIX_ERROR + redis::StatusToRedisError({Status::NotOK, redis::errWrongNumArguments}); } bool ReplicationThread::isUnknownOption(std::string_view err) { - return err == redis::Error({Status::NotOK, redis::errUnknownOption}); + // err doesn't contain the CRLF, so cannot use redis::Error here. + return err == RESP_PREFIX_ERROR + redis::StatusToRedisError({Status::NotOK, redis::errUnknownOption}); } rocksdb::Status WriteBatchHandler::PutCF(uint32_t column_family_id, const rocksdb::Slice &key, diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index 0a64ae8bf50..eb1317593ee 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -38,7 +38,7 @@ void Reply(evbuffer *output, const std::string &data) { evbuffer_add(output, dat std::string SimpleString(const std::string &data) { return "+" + data + CRLF; } -std::string Error(const Status &s) { return RESP_PREFIX_ERROR + StatusToRedisError(s); } +std::string Error(const Status &s) { return RESP_PREFIX_ERROR + StatusToRedisError(s) + CRLF; } std::string StatusToRedisError(const Status &s) { CHECK(!s.IsOK()); @@ -46,10 +46,10 @@ std::string StatusToRedisError(const Status &s) { if (auto it = redisErrorPrefixMapping.find(s.GetCode()); it != redisErrorPrefixMapping.end()) { prefix = it->second; } - if (!prefix.empty()) { - prefix = prefix + " "; + if (prefix.empty()) { + return s.Msg(); } - return prefix + s.Msg() + CRLF; + return prefix + " " + s.Msg(); } std::string BulkString(const std::string &data) { return "$" + std::to_string(data.length()) + CRLF + data + CRLF; } From 89a61d8cfe38fb877625b313c33e22f083348332 Mon Sep 17 00:00:00 2001 From: git-hulk Date: Sat, 15 Jun 2024 10:31:14 +0800 Subject: [PATCH 11/11] Rename --- src/cluster/replication.cc | 6 +++--- src/server/redis_reply.cc | 4 ++-- src/server/redis_reply.h | 2 +- src/storage/scripting.cc | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/cluster/replication.cc b/src/cluster/replication.cc index 5dd1fedb15e..3de51a94047 100644 --- a/src/cluster/replication.cc +++ b/src/cluster/replication.cc @@ -1001,17 +1001,17 @@ Status ReplicationThread::parseWriteBatch(const std::string &batch_string) { bool ReplicationThread::isRestoringError(std::string_view err) { // err doesn't contain the CRLF, so cannot use redis::Error here. - return err == RESP_PREFIX_ERROR + redis::StatusToRedisError({Status::RedisLoading, redis::errRestoringBackup}); + return err == RESP_PREFIX_ERROR + redis::StatusToRedisErrorMsg({Status::RedisLoading, redis::errRestoringBackup}); } bool ReplicationThread::isWrongPsyncNum(std::string_view err) { // err doesn't contain the CRLF, so cannot use redis::Error here. - return err == RESP_PREFIX_ERROR + redis::StatusToRedisError({Status::NotOK, redis::errWrongNumArguments}); + return err == RESP_PREFIX_ERROR + redis::StatusToRedisErrorMsg({Status::NotOK, redis::errWrongNumArguments}); } bool ReplicationThread::isUnknownOption(std::string_view err) { // err doesn't contain the CRLF, so cannot use redis::Error here. - return err == RESP_PREFIX_ERROR + redis::StatusToRedisError({Status::NotOK, redis::errUnknownOption}); + return err == RESP_PREFIX_ERROR + redis::StatusToRedisErrorMsg({Status::NotOK, redis::errUnknownOption}); } rocksdb::Status WriteBatchHandler::PutCF(uint32_t column_family_id, const rocksdb::Slice &key, diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index eb1317593ee..20e15e512f8 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -38,9 +38,9 @@ void Reply(evbuffer *output, const std::string &data) { evbuffer_add(output, dat std::string SimpleString(const std::string &data) { return "+" + data + CRLF; } -std::string Error(const Status &s) { return RESP_PREFIX_ERROR + StatusToRedisError(s) + CRLF; } +std::string Error(const Status &s) { return RESP_PREFIX_ERROR + StatusToRedisErrorMsg(s) + CRLF; } -std::string StatusToRedisError(const Status &s) { +std::string StatusToRedisErrorMsg(const Status &s) { CHECK(!s.IsOK()); std::string prefix = "ERR"; if (auto it = redisErrorPrefixMapping.find(s.GetCode()); it != redisErrorPrefixMapping.end()) { diff --git a/src/server/redis_reply.h b/src/server/redis_reply.h index 6e40fd39c10..f50c559d704 100644 --- a/src/server/redis_reply.h +++ b/src/server/redis_reply.h @@ -39,7 +39,7 @@ void Reply(evbuffer *output, const std::string &data); std::string SimpleString(const std::string &data); std::string Error(const Status &s); -std::string StatusToRedisError(const Status &s); +std::string StatusToRedisErrorMsg(const Status &s); template , int> = 0> std::string Integer(T data) { diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc index 4bc68d73e6b..5197eb5c1b1 100644 --- a/src/storage/scripting.cc +++ b/src/storage/scripting.cc @@ -754,7 +754,7 @@ int RedisGenericCommand(lua_State *lua, int raise_error) { if (config->cluster_enabled) { auto s = srv->cluster->CanExecByMySelf(attributes, args, conn); if (!s.IsOK()) { - PushError(lua, redis::StatusToRedisError(s).c_str()); + PushError(lua, redis::StatusToRedisErrorMsg(s).c_str()); return raise_error ? RaiseError(lua) : 1; } }