diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile new file mode 100644 index 00000000000..6b5b486e12a --- /dev/null +++ b/.devcontainer/Dockerfile @@ -0,0 +1,9 @@ +FROM ubuntu:noble +RUN apt update \ + && apt install -y \ + git build-essential cmake libtool python3 libssl-dev python3-pip \ + wget curl clang-format-14 clang-tidy-14 golang-go ninja-build \ + redis-tools vim python3-redis redis-server clang lld mold gdb fish +RUN BUILD_DIR=$(pwd) && git clone https://github.com/jsha/minica /opt/minica \ + && cd /opt/minica && git checkout 96a5c93723cf3d34b50b3e723a9f05cd3765bc67 && go build && cd $BUILD_DIR \ + && echo 'export PATH=/opt/minica:$PATH' >> $HOME/.bashrc diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json new file mode 100644 index 00000000000..601cce0dae6 --- /dev/null +++ b/.devcontainer/devcontainer.json @@ -0,0 +1,5 @@ +{ + "build": { + "dockerfile": "Dockerfile" + } +} diff --git a/.github/config/typos.toml b/.github/config/typos.toml index daae57c8137..035185408e1 100644 --- a/.github/config/typos.toml +++ b/.github/config/typos.toml @@ -20,6 +20,11 @@ extend-exclude = [ ".git/", "src/vendor/", "tests/gocase/util/slot.go", + + # Uses short strings for testing glob matching + "tests/cppunit/string_util_test.cc", + "tests/gocase/unit/keyspace/keyspace_test.go", + "tests/gocase/unit/scan/scan_test.go", ] ignore-hidden = false diff --git a/.github/workflows/kvrocks.yaml b/.github/workflows/kvrocks.yaml index a203f892d0d..c338cbfc54c 100644 --- a/.github/workflows/kvrocks.yaml +++ b/.github/workflows/kvrocks.yaml @@ -58,7 +58,7 @@ jobs: steps: - uses: actions/checkout@v4 - name: Check typos - uses: crate-ci/typos@v1.25.0 + uses: crate-ci/typos@v1.27.0 with: config: .github/config/typos.toml @@ -258,7 +258,7 @@ jobs: if: ${{ matrix.sonarcloud }} - name: Install sonar-scanner and build-wrapper - uses: SonarSource/sonarcloud-github-c-cpp@v2 + uses: SonarSource/sonarcloud-github-c-cpp@v3 if: ${{ matrix.sonarcloud }} - name: Build Kvrocks diff --git a/cmake/jsoncons.cmake b/cmake/jsoncons.cmake index 4b9a3982552..7be26255b44 100644 --- a/cmake/jsoncons.cmake +++ b/cmake/jsoncons.cmake @@ -20,8 +20,8 @@ include_guard() include(cmake/utils.cmake) FetchContent_DeclareGitHubWithMirror(jsoncons - danielaparker/jsoncons v0.177.0 - MD5=393062889321f4f715a9302d8f49acf8 + danielaparker/jsoncons v0.178.0 + MD5=397410843b7c540e9dcee9b8b0c797a6 ) FetchContent_MakeAvailableWithArgs(jsoncons diff --git a/cmake/rocksdb.cmake b/cmake/rocksdb.cmake index c5799763be8..393a720423d 100644 --- a/cmake/rocksdb.cmake +++ b/cmake/rocksdb.cmake @@ -26,8 +26,8 @@ endif() include(cmake/utils.cmake) FetchContent_DeclareGitHubWithMirror(rocksdb - facebook/rocksdb v9.3.1 - MD5=129235c789a963c004290d27d09ca48a + facebook/rocksdb v9.7.4 + MD5=9a08feb50e017006146bcff37059096f ) FetchContent_GetProperties(jemalloc) diff --git a/cmake/tbb.cmake b/cmake/tbb.cmake index 7fde5f30c03..622a478306b 100644 --- a/cmake/tbb.cmake +++ b/cmake/tbb.cmake @@ -20,8 +20,8 @@ include_guard() include(cmake/utils.cmake) FetchContent_DeclareGitHubWithMirror(tbb - oneapi-src/oneTBB v2021.13.0 - MD5=2dd9b7cfa5de5bb3add2f7392e0c9bab + oneapi-src/oneTBB v2022.0.0 + MD5=7eaeff0ddec85182afb60f2232fae2af ) FetchContent_MakeAvailableWithArgs(tbb diff --git a/src/cluster/cluster.cc b/src/cluster/cluster.cc index c4dd9b8c697..3850e12fc29 100644 --- a/src/cluster/cluster.cc +++ b/src/cluster/cluster.cc @@ -75,9 +75,7 @@ Status Cluster::SetNodeId(const std::string &node_id) { } // Set replication relationship - if (myself_) return SetMasterSlaveRepl(); - - return Status::OK(); + return SetMasterSlaveRepl(); } // The reason why the new version MUST be +1 of current version is that, @@ -204,11 +202,8 @@ Status Cluster::SetClusterNodes(const std::string &nodes_str, int64_t version, b } // Set replication relationship - if (myself_) { - s = SetMasterSlaveRepl(); - if (!s.IsOK()) { - return s.Prefixed("failed to set master-replica replication"); - } + if (auto s = SetMasterSlaveRepl(); !s.IsOK()) { + return s.Prefixed("failed to set master-replica replication"); } // Clear data of migrated slots @@ -234,7 +229,13 @@ Status Cluster::SetClusterNodes(const std::string &nodes_str, int64_t version, b Status Cluster::SetMasterSlaveRepl() { if (!srv_) return Status::OK(); - if (!myself_) return Status::OK(); + // If the node is not in the cluster topology, remove the master replication if it's a replica. + if (!myself_) { + if (auto s = srv_->RemoveMaster(); !s.IsOK()) { + return s.Prefixed("failed to remove master"); + } + return Status::OK(); + } if (myself_->role == kClusterMaster) { // Master mode diff --git a/src/cluster/replication.cc b/src/cluster/replication.cc index 14575703ddc..dff2d3d7956 100644 --- a/src/cluster/replication.cc +++ b/src/cluster/replication.cc @@ -62,7 +62,7 @@ Status FeedSlaveThread::Start() { sigaddset(&mask, SIGHUP); sigaddset(&mask, SIGPIPE); pthread_sigmask(SIG_BLOCK, &mask, &omask); - auto s = util::SockSend(conn_->GetFD(), redis::SimpleString("OK"), conn_->GetBufferEvent()); + auto s = util::SockSend(conn_->GetFD(), redis::RESP_OK, conn_->GetBufferEvent()); if (!s.IsOK()) { LOG(ERROR) << "failed to send OK response to the replica: " << s.Msg(); return; diff --git a/src/cluster/sync_migrate_context.cc b/src/cluster/sync_migrate_context.cc index a7caf73c853..7b51c982793 100644 --- a/src/cluster/sync_migrate_context.cc +++ b/src/cluster/sync_migrate_context.cc @@ -66,7 +66,7 @@ void SyncMigrateContext::TimerCB(int, [[maybe_unused]] int16_t events) { void SyncMigrateContext::OnWrite(bufferevent *bev) { if (migrate_result_) { - conn_->Reply(redis::SimpleString("OK")); + conn_->Reply(redis::RESP_OK); } else { conn_->Reply(redis::Error(migrate_result_)); } diff --git a/src/commands/blocking_commander.h b/src/commands/blocking_commander.h index 05883a8ac04..2749681984f 100644 --- a/src/commands/blocking_commander.h +++ b/src/commands/blocking_commander.h @@ -21,6 +21,7 @@ #pragma once #include "commander.h" +#include "common/lock_manager.h" #include "event_util.h" #include "server/redis_connection.h" @@ -44,6 +45,10 @@ class BlockingCommander : public Commander, // in other words, returning true indicates ending the blocking virtual bool OnBlockingWrite() = 0; + // GetLocks() locks the keys of the BlockingCommander with MultiLockGuard. + // When OnWrite() is triggered, BlockingCommander needs to relock the keys. + virtual MultiLockGuard GetLocks() = 0; + // to start the blocking process // usually put to the end of the Execute method Status StartBlocking(int64_t timeout, std::string *output) { @@ -63,7 +68,11 @@ class BlockingCommander : public Commander, } void OnWrite(bufferevent *bev) { - bool done = OnBlockingWrite(); + bool done{false}; + { + auto guard = GetLocks(); + done = OnBlockingWrite(); + } if (!done) { // The connection may be waked up but can't pop from the datatype. diff --git a/src/commands/cmd_bloom_filter.cc b/src/commands/cmd_bloom_filter.cc index 6fd422ff8eb..d58a1a46255 100644 --- a/src/commands/cmd_bloom_filter.cc +++ b/src/commands/cmd_bloom_filter.cc @@ -94,7 +94,7 @@ class CommandBFReserve : public Commander { auto s = bloomfilter_db.Reserve(ctx, args_[1], capacity_, error_rate_, expansion_); if (!s.ok()) return {Status::RedisExecErr, s.ToString()}; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } diff --git a/src/commands/cmd_cluster.cc b/src/commands/cmd_cluster.cc index ab23f86d2a5..f8e637281b4 100644 --- a/src/commands/cmd_cluster.cc +++ b/src/commands/cmd_cluster.cc @@ -116,14 +116,14 @@ class CommandCluster : public Commander { // TODO: support multiple slot ranges Status s = srv->cluster->ImportSlotRange(conn, slot_ranges_[0], state_); if (s.IsOK()) { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else { return s; } } else if (subcommand_ == "reset") { Status s = srv->cluster->Reset(); if (s.IsOK()) { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else { return s; } @@ -258,7 +258,7 @@ class CommandClusterX : public Commander { Status s = srv->cluster->SetClusterNodes(nodes_str_, set_version_, force_); if (s.IsOK()) { need_persist_nodes_info = true; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else { return s; } @@ -266,7 +266,7 @@ class CommandClusterX : public Commander { Status s = srv->cluster->SetNodeId(args_[2]); if (s.IsOK()) { need_persist_nodes_info = true; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else { return s; } @@ -274,7 +274,7 @@ class CommandClusterX : public Commander { Status s = srv->cluster->SetSlotRanges(slot_ranges_, args_[4], set_version_); if (s.IsOK()) { need_persist_nodes_info = true; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else { return s; } @@ -293,7 +293,7 @@ class CommandClusterX : public Commander { if (sync_migrate_) { return {Status::BlockingCmd}; } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else { return s; } @@ -331,7 +331,7 @@ class CommandReadOnly : public Commander { public: Status Execute([[maybe_unused]] engine::Context &ctx, [[maybe_unused]] Server *srv, Connection *conn, std::string *output) override { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; conn->EnableFlag(redis::Connection::kReadOnly); return Status::OK(); } @@ -341,7 +341,7 @@ class CommandReadWrite : public Commander { public: Status Execute([[maybe_unused]] engine::Context &ctx, [[maybe_unused]] Server *srv, Connection *conn, std::string *output) override { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; conn->DisableFlag(redis::Connection::kReadOnly); return Status::OK(); } @@ -352,16 +352,15 @@ class CommandAsking : public Commander { Status Execute([[maybe_unused]] engine::Context &ctx, [[maybe_unused]] Server *srv, Connection *conn, std::string *output) override { conn->EnableFlag(redis::Connection::kAsking); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; -REDIS_REGISTER_COMMANDS(Cluster, - MakeCmdAttr("cluster", -2, "cluster no-script", NO_KEY, GenerateClusterFlag), - MakeCmdAttr("clusterx", -2, "cluster no-script", NO_KEY, GenerateClusterFlag), - MakeCmdAttr("readonly", 1, "cluster no-multi", NO_KEY), - MakeCmdAttr("readwrite", 1, "cluster no-multi", NO_KEY), - MakeCmdAttr("asking", 1, "cluster", NO_KEY), ) +REDIS_REGISTER_COMMANDS(Cluster, MakeCmdAttr("cluster", -2, "no-script", NO_KEY, GenerateClusterFlag), + MakeCmdAttr("clusterx", -2, "no-script", NO_KEY, GenerateClusterFlag), + MakeCmdAttr("readonly", 1, "no-multi", NO_KEY), + MakeCmdAttr("readwrite", 1, "no-multi", NO_KEY), + MakeCmdAttr("asking", 1, "", NO_KEY), ) } // namespace redis diff --git a/src/commands/cmd_function.cc b/src/commands/cmd_function.cc index ee5a580d314..c229cdaa24e 100644 --- a/src/commands/cmd_function.cc +++ b/src/commands/cmd_function.cc @@ -75,7 +75,7 @@ struct CommandFunction : Commander { auto s = lua::FunctionDelete(ctx, srv, libname); if (!s) return s; - *output = SimpleString("OK"); + *output = RESP_OK; return Status::OK(); } else { return {Status::NotOK, "no such subcommand"}; @@ -109,9 +109,10 @@ uint64_t GenerateFunctionFlags(uint64_t flags, const std::vector &a return flags; } -REDIS_REGISTER_COMMANDS( - Function, MakeCmdAttr("function", -2, "exclusive no-script", NO_KEY, GenerateFunctionFlags), - MakeCmdAttr>("fcall", -3, "exclusive write no-script", GetScriptEvalKeyRange), - MakeCmdAttr>("fcall_ro", -3, "read-only ro-script no-script", GetScriptEvalKeyRange)); +REDIS_REGISTER_COMMANDS(Function, + MakeCmdAttr("function", -2, "exclusive no-script", NO_KEY, + GenerateFunctionFlags), + MakeCmdAttr>("fcall", -3, "exclusive write no-script", GetScriptEvalKeyRange), + MakeCmdAttr>("fcall_ro", -3, "read-only no-script", GetScriptEvalKeyRange)); } // namespace redis diff --git a/src/commands/cmd_hash.cc b/src/commands/cmd_hash.cc index 9b9847fbbe3..30d6eb28ab0 100644 --- a/src/commands/cmd_hash.cc +++ b/src/commands/cmd_hash.cc @@ -249,7 +249,7 @@ class CommandHMSet : public Commander { if (GetAttributes()->name == "hset") { *output = redis::Integer(ret); } else { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } return Status::OK(); } diff --git a/src/commands/cmd_hll.cc b/src/commands/cmd_hll.cc index 35f6889dd85..3af558cdc57 100644 --- a/src/commands/cmd_hll.cc +++ b/src/commands/cmd_hll.cc @@ -93,7 +93,7 @@ class CommandPfMerge final : public Commander { if (!s.ok() && !s.IsNotFound()) { return {Status::RedisExecErr, s.ToString()}; } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; diff --git a/src/commands/cmd_json.cc b/src/commands/cmd_json.cc index 7f9de307ccd..a18cf903dbf 100644 --- a/src/commands/cmd_json.cc +++ b/src/commands/cmd_json.cc @@ -61,7 +61,7 @@ class CommandJsonSet : public Commander { auto s = json.Set(ctx, args_[1], args_[2], args_[3]); if (!s.ok()) return {Status::RedisExecErr, s.ToString()}; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; @@ -337,7 +337,7 @@ class CommandJsonMerge : public Commander { if (!result) { *output = conn->NilString(); } else { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } return Status::OK(); @@ -648,7 +648,7 @@ class CommandJsonMSet : public Commander { if (auto s = json.MSet(ctx, user_keys, paths, values); !s.ok()) return {Status::RedisExecErr, s.ToString()}; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; diff --git a/src/commands/cmd_key.cc b/src/commands/cmd_key.cc index 023db7ca16c..827f6f52e70 100644 --- a/src/commands/cmd_key.cc +++ b/src/commands/cmd_key.cc @@ -367,7 +367,7 @@ class CommandRename : public Commander { auto s = redis.Copy(ctx, ns_key, new_ns_key, false, true, &res); if (!s.ok()) return {Status::RedisExecErr, s.ToString()}; if (res == Database::CopyResult::KEY_NOT_EXIST) return {Status::RedisExecErr, "no such key"}; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; diff --git a/src/commands/cmd_list.cc b/src/commands/cmd_list.cc index 0cc2f1210fd..1a9f5d03dfc 100644 --- a/src/commands/cmd_list.cc +++ b/src/commands/cmd_list.cc @@ -313,6 +313,16 @@ class CommandBPop : public BlockingCommander { return s; } + MultiLockGuard GetLocks() override { + std::vector lock_keys; + lock_keys.reserve(keys_.size()); + for (const auto &key : keys_) { + auto ns_key = ComposeNamespaceKey(conn_->GetNamespace(), key, srv_->storage->IsSlotIdEncoded()); + lock_keys.emplace_back(std::move(ns_key)); + } + return MultiLockGuard(srv_->storage->GetLockManager(), lock_keys); + } + bool OnBlockingWrite() override { engine::Context ctx(srv_->storage); auto s = TryPopFromList(ctx); @@ -436,6 +446,16 @@ class CommandBLMPop : public BlockingCommander { } } + MultiLockGuard GetLocks() override { + std::vector lock_keys; + lock_keys.reserve(keys_.size()); + for (const auto &key : keys_) { + auto ns_key = ComposeNamespaceKey(conn_->GetNamespace(), key, srv_->storage->IsSlotIdEncoded()); + lock_keys.emplace_back(std::move(ns_key)); + } + return MultiLockGuard(srv_->storage->GetLockManager(), lock_keys); + } + bool OnBlockingWrite() override { engine::Context ctx(srv_->storage); auto s = ExecuteUnblocked(ctx); @@ -625,7 +645,7 @@ class CommandLSet : public Commander { return {Status::RedisExecErr, errNoSuchKey}; } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -656,7 +676,7 @@ class CommandLTrim : public Commander { return {Status::RedisExecErr, s.ToString()}; } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -767,6 +787,15 @@ class CommandBLMove : public BlockingCommander { void UnblockKeys() override { srv_->UnblockOnKey(args_[1], conn_); } + MultiLockGuard GetLocks() override { + std::vector lock_keys{ + ComposeNamespaceKey(conn_->GetNamespace(), args_[1], srv_->storage->IsSlotIdEncoded())}; + if (args_[1] != args_[2]) { + lock_keys.emplace_back(ComposeNamespaceKey(conn_->GetNamespace(), args_[2], srv_->storage->IsSlotIdEncoded())); + } + return MultiLockGuard(srv_->storage->GetLockManager(), lock_keys); + } + bool OnBlockingWrite() override { redis::List list_db(srv_->storage, conn_->GetNamespace()); std::string elem; @@ -865,14 +894,15 @@ class CommandLPos : public Commander { PosSpec spec_; }; -REDIS_REGISTER_COMMANDS(List, MakeCmdAttr("blpop", -3, "write no-script", 1, -2, 1), - MakeCmdAttr("brpop", -3, "write no-script", 1, -2, 1), - MakeCmdAttr("blmpop", -5, "write no-script", CommandBLMPop::keyRangeGen), +REDIS_REGISTER_COMMANDS(List, MakeCmdAttr("blpop", -3, "write no-script blocking", 1, -2, 1), + MakeCmdAttr("brpop", -3, "write no-script blocking", 1, -2, 1), + MakeCmdAttr("blmpop", -5, "write no-script blocking", + CommandBLMPop::keyRangeGen), MakeCmdAttr("lindex", 3, "read-only", 1, 1, 1), MakeCmdAttr("linsert", 5, "write slow", 1, 1, 1), MakeCmdAttr("llen", 2, "read-only", 1, 1, 1), MakeCmdAttr("lmove", 5, "write", 1, 2, 1), - MakeCmdAttr("blmove", 6, "write", 1, 2, 1), + MakeCmdAttr("blmove", 6, "write blocking", 1, 2, 1), MakeCmdAttr("lpop", -2, "write", 1, 1, 1), // MakeCmdAttr("lpos", -3, "read-only", 1, 1, 1), MakeCmdAttr("lpush", -3, "write", 1, 1, 1), diff --git a/src/commands/cmd_pubsub.cc b/src/commands/cmd_pubsub.cc index e7e4dee7826..8aaa7f4bab8 100644 --- a/src/commands/cmd_pubsub.cc +++ b/src/commands/cmd_pubsub.cc @@ -252,15 +252,14 @@ class CommandPubSub : public Commander { std::string subcommand_; }; -REDIS_REGISTER_COMMANDS( - Pubsub, MakeCmdAttr("publish", 3, "read-only pub-sub", NO_KEY), - MakeCmdAttr("mpublish", -3, "read-only pub-sub", NO_KEY), - MakeCmdAttr("subscribe", -2, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("unsubscribe", -1, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("psubscribe", -2, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("punsubscribe", -1, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("ssubscribe", -2, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("sunsubscribe", -1, "read-only pub-sub no-multi no-script", NO_KEY), - MakeCmdAttr("pubsub", -2, "read-only pub-sub no-script", NO_KEY), ) +REDIS_REGISTER_COMMANDS(Pubsub, MakeCmdAttr("publish", 3, "read-only", NO_KEY), + MakeCmdAttr("mpublish", -3, "read-only", NO_KEY), + MakeCmdAttr("subscribe", -2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("unsubscribe", -1, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("psubscribe", -2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("punsubscribe", -1, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("ssubscribe", -2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("sunsubscribe", -1, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("pubsub", -2, "read-only no-script", NO_KEY), ) } // namespace redis diff --git a/src/commands/cmd_replication.cc b/src/commands/cmd_replication.cc index 71fd7fb626d..4d46e11d9ff 100644 --- a/src/commands/cmd_replication.cc +++ b/src/commands/cmd_replication.cc @@ -194,7 +194,7 @@ class CommandReplConf : public Commander { if (!ip_address_.empty()) { conn->SetAnnounceIP(ip_address_); } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -344,11 +344,10 @@ class CommandDBName : public Commander { } }; -REDIS_REGISTER_COMMANDS( - Replication, MakeCmdAttr("replconf", -3, "read-only replication no-script", NO_KEY), - MakeCmdAttr("psync", -2, "read-only replication no-multi no-script", NO_KEY), - MakeCmdAttr("_fetch_meta", 1, "read-only replication no-multi no-script", NO_KEY), - MakeCmdAttr("_fetch_file", 2, "read-only replication no-multi no-script", NO_KEY), - MakeCmdAttr("_db_name", 1, "read-only replication no-multi", NO_KEY), ) +REDIS_REGISTER_COMMANDS(Replication, MakeCmdAttr("replconf", -3, "read-only no-script", NO_KEY), + MakeCmdAttr("psync", -2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("_fetch_meta", 1, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("_fetch_file", 2, "read-only no-multi no-script", NO_KEY), + MakeCmdAttr("_db_name", 1, "read-only no-multi", NO_KEY), ) } // namespace redis diff --git a/src/commands/cmd_script.cc b/src/commands/cmd_script.cc index 40c19a17ecf..b4cf3539f52 100644 --- a/src/commands/cmd_script.cc +++ b/src/commands/cmd_script.cc @@ -83,7 +83,7 @@ class CommandScript : public Commander { LOG(ERROR) << "Failed to propagate script command: " << s.Msg(); return s; } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else if (args_.size() >= 3 && subcommand_ == "exists") { *output = redis::MultiLen(args_.size() - 2); for (size_t j = 2; j < args_.size(); j++) { @@ -125,11 +125,11 @@ uint64_t GenerateScriptFlags(uint64_t flags, const std::vector &arg return flags; } -REDIS_REGISTER_COMMANDS( - Script, MakeCmdAttr("eval", -3, "exclusive write no-script", GetScriptEvalKeyRange), - MakeCmdAttr("evalsha", -3, "exclusive write no-script", GetScriptEvalKeyRange), - MakeCmdAttr("eval_ro", -3, "read-only no-script ro-script", GetScriptEvalKeyRange), - MakeCmdAttr("evalsha_ro", -3, "read-only no-script ro-script", GetScriptEvalKeyRange), - MakeCmdAttr("script", -2, "exclusive no-script", NO_KEY), ) +REDIS_REGISTER_COMMANDS(Script, + MakeCmdAttr("eval", -3, "exclusive write no-script", GetScriptEvalKeyRange), + MakeCmdAttr("evalsha", -3, "exclusive write no-script", GetScriptEvalKeyRange), + MakeCmdAttr("eval_ro", -3, "read-only no-script", GetScriptEvalKeyRange), + MakeCmdAttr("evalsha_ro", -3, "read-only no-script", GetScriptEvalKeyRange), + MakeCmdAttr("script", -2, "exclusive no-script", NO_KEY), ) } // namespace redis diff --git a/src/commands/cmd_search.cc b/src/commands/cmd_search.cc index f6d5ab05e25..bf4e527f27f 100644 --- a/src/commands/cmd_search.cc +++ b/src/commands/cmd_search.cc @@ -185,7 +185,7 @@ class CommandFTCreate : public Commander { GET_OR_RET(srv->index_mgr.Create(ctx, std::move(index_info_))); - output->append(redis::SimpleString("OK")); + output->append(redis::RESP_OK); return Status::OK(); }; @@ -488,7 +488,7 @@ class CommandFTDrop : public Commander { GET_OR_RET(srv->index_mgr.Drop(ctx, index_name, conn->GetNamespace())); - output->append(SimpleString("OK")); + output->append(redis::RESP_OK); return Status::OK(); }; diff --git a/src/commands/cmd_server.cc b/src/commands/cmd_server.cc index 725b28d0cdf..e2938267710 100644 --- a/src/commands/cmd_server.cc +++ b/src/commands/cmd_server.cc @@ -23,6 +23,8 @@ #include "commands/scan_base.h" #include "common/io_util.h" #include "common/rdb_stream.h" +#include "common/string_util.h" +#include "common/time_util.h" #include "config/config.h" #include "error_constants.h" #include "server/redis_connection.h" @@ -30,8 +32,6 @@ #include "server/server.h" #include "stats/disk_stats.h" #include "storage/rdb/rdb.h" -#include "string_util.h" -#include "time_util.h" namespace redis { @@ -54,7 +54,7 @@ class CommandAuth : public Commander { break; } conn->SetNamespace(ns); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; @@ -92,17 +92,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(s); + *output = s.IsOK() ? redis::RESP_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(s); + *output = s.IsOK() ? redis::RESP_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(s); + *output = s.IsOK() ? redis::RESP_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"}; @@ -114,15 +114,15 @@ class CommandNamespace : public Commander { class CommandKeys : public Commander { public: Status Execute(engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override { - const std::string &prefix = args_[1]; + const std::string &glob_pattern = args_[1]; std::vector keys; redis::Database redis(srv->storage, conn->GetNamespace()); - if (prefix.empty() || prefix.find('*') != prefix.size() - 1) { - return {Status::RedisExecErr, "only keys prefix match was supported"}; + if (const Status s = util::ValidateGlob(glob_pattern); !s.IsOK()) { + return {Status::RedisParseErr, "Invalid glob pattern: " + s.Msg()}; } - - const rocksdb::Status s = redis.Keys(ctx, prefix.substr(0, prefix.size() - 1), &keys); + const auto [prefix, suffix_glob] = util::SplitGlob(glob_pattern); + const rocksdb::Status s = redis.Keys(ctx, prefix, suffix_glob, &keys); if (!s.ok()) { return {Status::RedisExecErr, s.ToString()}; } @@ -145,7 +145,7 @@ class CommandFlushDB : public Commander { auto s = redis.FlushDB(ctx); LOG(WARNING) << "DB keys in namespace: " << conn->GetNamespace() << " was flushed, addr: " << conn->GetAddr(); if (s.ok()) { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -172,7 +172,7 @@ class CommandFlushAll : public Commander { auto s = redis.FlushAll(ctx); if (s.ok()) { LOG(WARNING) << "All DB keys was flushed, addr: " << conn->GetAddr(); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -199,7 +199,7 @@ class CommandSelect : public Commander { public: Status Execute([[maybe_unused]] engine::Context &ctx, [[maybe_unused]] Server *srv, [[maybe_unused]] Connection *conn, std::string *output) override { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; @@ -222,7 +222,7 @@ class CommandConfig : public Commander { Status s = config->Rewrite(srv->GetNamespace()->List()); if (!s.IsOK()) return s; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; LOG(INFO) << "# CONFIG REWRITE executed with success"; } else if (args_.size() == 3 && sub_command == "get") { std::vector values; @@ -233,7 +233,7 @@ class CommandConfig : public Commander { if (!s.IsOK()) { return {Status::RedisExecErr, "CONFIG SET '" + args_[2] + "' error: " + s.Msg()}; } else { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } } else { return {Status::RedisExecErr, "CONFIG subcommand must be one of GET, SET, REWRITE"}; @@ -311,7 +311,7 @@ class CommandDBSize : public Commander { } else if (args_.size() == 2 && util::EqualICase(args_[1], "scan")) { Status s = srv->AsyncScanDBSize(ns); if (s.IsOK()) { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else { return s; } @@ -348,7 +348,7 @@ class CommandPerfLog : public Commander { *output = redis::Integer(static_cast(perf_log->Size())); } else if (subcommand_ == "reset") { perf_log->Reset(); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else if (subcommand_ == "get") { *output = perf_log->GetLatestEntries(cnt_); } @@ -384,7 +384,7 @@ class CommandSlowlog : public Commander { auto slowlog = srv->GetSlowLog(); if (subcommand_ == "reset") { slowlog->Reset(); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } else if (subcommand_ == "len") { *output = redis::Integer(static_cast(slowlog->Size())); @@ -489,7 +489,7 @@ class CommandClient : public Commander { return Status::OK(); } else if (subcommand_ == "setname") { conn->SetName(conn_name_); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } else if (subcommand_ == "getname") { std::string name = conn->GetName(); @@ -507,7 +507,7 @@ class CommandClient : public Commander { if (killed == 0) return {Status::RedisExecErr, "No such client"}; else - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } return Status::OK(); } @@ -530,7 +530,7 @@ class CommandMonitor : public Commander { Status Execute([[maybe_unused]] engine::Context &ctx, [[maybe_unused]] Server *srv, Connection *conn, std::string *output) override { conn->Owner()->BecomeMonitorConn(conn); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; @@ -556,7 +556,7 @@ class CommandQuit : public Commander { Status Execute([[maybe_unused]] engine::Context &ctx, [[maybe_unused]] Server *srv, Connection *conn, std::string *output) override { conn->EnableFlag(redis::Connection::kCloseAfterReply); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; @@ -591,7 +591,7 @@ class CommandDebug : public Commander { Status Execute([[maybe_unused]] engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override { if (subcommand_ == "sleep") { usleep(microsecond_); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else if (subcommand_ == "protocol") { // protocol type if (protocol_type_ == "string") { *output = redis::BulkString("Hello World"); @@ -639,7 +639,7 @@ class CommandDebug : public Commander { } } else if (subcommand_ == "dbsize-limit") { srv->storage->SetDBSizeLimit(dbsize_limit_); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else { return {Status::RedisInvalidCmd, "Unknown subcommand, should be SLEEP, PROTOCOL or DBSIZE-LIMIT"}; } @@ -846,7 +846,7 @@ class CommandScan : public CommandScanBase { std::vector keys; std::string end_key; - auto s = redis_db.Scan(ctx, key_name, limit_, prefix_, &keys, &end_key, type_); + const auto s = redis_db.Scan(ctx, key_name, limit_, prefix_, suffix_glob_, &keys, &end_key, type_); if (!s.ok()) { return {Status::RedisExecErr, s.ToString()}; } @@ -885,7 +885,7 @@ class CommandCompact : public Commander { Status s = srv->AsyncCompactDB(begin_key, end_key); if (!s.IsOK()) return s; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; LOG(INFO) << "Compact was triggered by manual with executed success"; return Status::OK(); } @@ -901,7 +901,7 @@ class CommandBGSave : public Commander { Status s = srv->AsyncBgSaveDB(); if (!s.IsOK()) return s; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; LOG(INFO) << "BGSave was triggered by manual with executed success"; return Status::OK(); } @@ -917,7 +917,7 @@ class CommandFlushBackup : public Commander { Status s = srv->AsyncPurgeOldBackups(0, 0); if (!s.IsOK()) return s; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; LOG(INFO) << "flushbackup was triggered by manual with executed success"; return Status::OK(); } @@ -979,7 +979,7 @@ class CommandSlaveOf : public Commander { return s.Prefixed("failed to remove master"); } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; LOG(WARNING) << "MASTER MODE enabled (user request from '" << conn->GetAddr() << "')"; if (srv->GetConfig()->cluster_enabled) { srv->slot_migrator->SetStopMigrationFlag(false); @@ -993,7 +993,7 @@ class CommandSlaveOf : public Commander { if (!s.IsOK()) return s; s = srv->AddMaster(host_, port_, false); if (s.IsOK()) { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; LOG(WARNING) << "SLAVE OF " << host_ << ":" << port_ << " enabled (user request from '" << conn->GetAddr() << "')"; if (srv->GetConfig()->cluster_enabled) { @@ -1096,7 +1096,7 @@ class CommandRestore : public Commander { auto now = util::GetTimeStampMS(); if (ttl_ms_ <= now) { // return ok if the ttl is already expired - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } ttl_ms_ -= now; @@ -1106,7 +1106,7 @@ class CommandRestore : public Commander { RDB rdb(srv->storage, conn->GetNamespace(), std::move(stream_ptr)); auto s = rdb.Restore(ctx, args_[1], args_[3], ttl_ms_); if (!s.IsOK()) return s; - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -1154,7 +1154,7 @@ class CommandRdb : public Commander { RDB rdb(srv->storage, conn->GetNamespace(), std::move(stream_ptr)); GET_OR_RET(rdb.LoadRdb(ctx, db_index_, overwrite_exist_key_)); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -1337,8 +1337,8 @@ REDIS_REGISTER_COMMANDS(Server, MakeCmdAttr("auth", 2, "read-only o MakeCmdAttr("config", -2, "read-only", NO_KEY, GenerateConfigFlag), MakeCmdAttr("namespace", -3, "read-only", NO_KEY), MakeCmdAttr("keys", 2, "read-only slow", NO_KEY), - MakeCmdAttr("flushdb", 1, "write no-dbsize-check", NO_KEY), - MakeCmdAttr("flushall", 1, "write no-dbsize-check", NO_KEY), + MakeCmdAttr("flushdb", 1, "write no-dbsize-check exclusive", NO_KEY), + MakeCmdAttr("flushall", 1, "write no-dbsize-check exclusive", NO_KEY), MakeCmdAttr("dbsize", -1, "read-only", NO_KEY), MakeCmdAttr("slowlog", -2, "read-only", NO_KEY), MakeCmdAttr("perflog", -2, "read-only", NO_KEY), @@ -1352,8 +1352,8 @@ REDIS_REGISTER_COMMANDS(Server, MakeCmdAttr("auth", 2, "read-only o MakeCmdAttr("command", -1, "read-only", NO_KEY), MakeCmdAttr("echo", 2, "read-only", NO_KEY), MakeCmdAttr("time", 1, "read-only ok-loading", NO_KEY), - MakeCmdAttr("disk", 3, "read-only", NO_KEY), - MakeCmdAttr("memory", 3, "read-only", NO_KEY), + MakeCmdAttr("disk", 3, "read-only", 2, 2, 1), + MakeCmdAttr("memory", 3, "read-only", 2, 2, 1), MakeCmdAttr("hello", -1, "read-only ok-loading", NO_KEY), MakeCmdAttr("restore", -4, "write", 1, 1, 1), @@ -1364,8 +1364,8 @@ REDIS_REGISTER_COMMANDS(Server, MakeCmdAttr("auth", 2, "read-only o MakeCmdAttr("slaveof", 3, "read-only exclusive no-script", NO_KEY), MakeCmdAttr("stats", 1, "read-only", NO_KEY), MakeCmdAttr("rdb", -3, "write exclusive", NO_KEY), - MakeCmdAttr("reset", 1, "ok-loading multi no-script pub-sub", NO_KEY), + MakeCmdAttr("reset", 1, "ok-loading multi no-script", NO_KEY), MakeCmdAttr("applybatch", -2, "write no-multi", NO_KEY), - MakeCmdAttr("dump", 2, "read-only", NO_KEY), + MakeCmdAttr("dump", 2, "read-only", 1, 1, 1), MakeCmdAttr("pollupdates", -2, "read-only", NO_KEY), ) } // namespace redis diff --git a/src/commands/cmd_stream.cc b/src/commands/cmd_stream.cc index 903b0031c5d..0fe1cd3bccf 100644 --- a/src/commands/cmd_stream.cc +++ b/src/commands/cmd_stream.cc @@ -547,7 +547,7 @@ class CommandXGroup : public Commander { return {Status::RedisExecErr, s.ToString()}; } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } if (subcommand_ == "destroy") { @@ -599,7 +599,7 @@ class CommandXGroup : public Commander { return {Status::RedisExecErr, s.ToString()}; } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } return Status::OK(); @@ -1635,6 +1635,14 @@ class CommandXReadGroup : public Commander, redis::Stream stream_db(srv_->storage, conn_->GetNamespace()); std::vector results; + + std::vector lock_keys; + lock_keys.reserve(streams_.size()); + for (auto &stream_name : streams_) { + auto ns_key = stream_db.AppendNamespacePrefix(stream_name); + lock_keys.emplace_back(std::move(ns_key)); + } + MultiLockGuard guard(srv_->storage->GetLockManager(), lock_keys); engine::Context ctx(srv_->storage); for (size_t i = 0; i < streams_.size(); ++i) { redis::StreamRangeOptions options; @@ -1865,7 +1873,7 @@ class CommandXSetId : public Commander { return {Status::RedisExecErr, s.ToString()}; } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -1888,8 +1896,8 @@ REDIS_REGISTER_COMMANDS(Stream, MakeCmdAttr("xack", -4, "write no-d MakeCmdAttr("xpending", -3, "read-only", 1, 1, 1), MakeCmdAttr("xrange", -4, "read-only", 1, 1, 1), MakeCmdAttr("xrevrange", -2, "read-only", 1, 1, 1), - MakeCmdAttr("xread", -4, "read-only", NO_KEY), - MakeCmdAttr("xreadgroup", -7, "write", NO_KEY), + MakeCmdAttr("xread", -4, "read-only blocking", NO_KEY), + MakeCmdAttr("xreadgroup", -7, "write blocking", NO_KEY), MakeCmdAttr("xtrim", -4, "write no-dbsize-check", 1, 1, 1), MakeCmdAttr("xsetid", -3, "write", 1, 1, 1)) diff --git a/src/commands/cmd_string.cc b/src/commands/cmd_string.cc index 1322c74b7b9..cfeb10ef7c8 100644 --- a/src/commands/cmd_string.cc +++ b/src/commands/cmd_string.cc @@ -322,7 +322,7 @@ class CommandSet : public Commander { } } else { if (ret.has_value()) { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; } else { *output = conn->NilString(); } @@ -356,7 +356,7 @@ class CommandSetEX : public Commander { redis::String string_db(srv->storage, conn->GetNamespace()); auto s = string_db.SetEX(ctx, args_[1], args_[3], expire_); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -383,7 +383,7 @@ class CommandPSetEX : public Commander { redis::String string_db(srv->storage, conn->GetNamespace()); auto s = string_db.SetEX(ctx, args_[1], args_[3], expire_); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -413,7 +413,7 @@ class CommandMSet : public Commander { return {Status::RedisExecErr, s.ToString()}; } - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; diff --git a/src/commands/cmd_txn.cc b/src/commands/cmd_txn.cc index 92113fee222..4f3cb3a4d4f 100644 --- a/src/commands/cmd_txn.cc +++ b/src/commands/cmd_txn.cc @@ -37,7 +37,7 @@ class CommandMulti : public Commander { conn->ResetMultiExec(); // Client starts into MULTI-EXEC conn->EnableFlag(Connection::kMultiExec); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; @@ -52,7 +52,7 @@ class CommandDiscard : public Commander { auto reset_watch = MakeScopeExit([srv, conn] { srv->ResetWatchedKeys(conn); }); conn->ResetMultiExec(); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } @@ -100,12 +100,12 @@ class CommandWatch : public Commander { // If a conn is already marked as watched_keys_modified, we can skip the watch. if (srv->IsWatchedKeysModified(conn)) { - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } srv->WatchKey(conn, std::vector(args_.begin() + 1, args_.end())); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; @@ -114,7 +114,7 @@ class CommandUnwatch : public Commander { public: Status Execute([[maybe_unused]] engine::Context &ctx, Server *srv, Connection *conn, std::string *output) override { srv->ResetWatchedKeys(conn); - *output = redis::SimpleString("OK"); + *output = redis::RESP_OK; return Status::OK(); } }; diff --git a/src/commands/cmd_zset.cc b/src/commands/cmd_zset.cc index d26abb54b91..99285a625e9 100644 --- a/src/commands/cmd_zset.cc +++ b/src/commands/cmd_zset.cc @@ -366,6 +366,16 @@ class CommandBZPop : public BlockingCommander { conn_->Reply(output); } + MultiLockGuard GetLocks() override { + std::vector lock_keys; + lock_keys.reserve(keys_.size()); + for (const auto &key : keys_) { + auto ns_key = ComposeNamespaceKey(conn_->GetNamespace(), key, srv_->storage->IsSlotIdEncoded()); + lock_keys.emplace_back(std::move(ns_key)); + } + return MultiLockGuard(srv_->storage->GetLockManager(), lock_keys); + } + bool OnBlockingWrite() override { std::string user_key; std::vector member_scores; @@ -548,6 +558,16 @@ class CommandBZMPop : public BlockingCommander { std::string NoopReply(const Connection *conn) override { return conn->NilString(); } + MultiLockGuard GetLocks() override { + std::vector lock_keys; + lock_keys.reserve(keys_.size()); + for (const auto &key : keys_) { + auto ns_key = ComposeNamespaceKey(conn_->GetNamespace(), key, srv_->storage->IsSlotIdEncoded()); + lock_keys.emplace_back(std::move(ns_key)); + } + return MultiLockGuard(srv_->storage->GetLockManager(), lock_keys); + } + bool OnBlockingWrite() override { std::string user_key; std::vector member_scores; @@ -1549,10 +1569,10 @@ REDIS_REGISTER_COMMANDS(ZSet, MakeCmdAttr("zadd", -4, "write", 1, 1 MakeCmdAttr("zlexcount", 4, "read-only", 1, 1, 1), MakeCmdAttr("zpopmax", -2, "write", 1, 1, 1), MakeCmdAttr("zpopmin", -2, "write", 1, 1, 1), - MakeCmdAttr("bzpopmax", -3, "write", 1, -2, 1), - MakeCmdAttr("bzpopmin", -3, "write", 1, -2, 1), + MakeCmdAttr("bzpopmax", -3, "write blocking", 1, -2, 1), + MakeCmdAttr("bzpopmin", -3, "write blocking", 1, -2, 1), MakeCmdAttr("zmpop", -4, "write", CommandZMPop::Range), - MakeCmdAttr("bzmpop", -5, "write", CommandBZMPop::Range), + MakeCmdAttr("bzmpop", -5, "write blocking", CommandBZMPop::Range), MakeCmdAttr("zrangestore", -5, "write", 1, 1, 1), MakeCmdAttr("zrange", -4, "read-only", 1, 1, 1), MakeCmdAttr("zrevrange", -4, "read-only", 1, 1, 1), diff --git a/src/commands/commander.h b/src/commands/commander.h index 87efdb0131e..b1d0a04975f 100644 --- a/src/commands/commander.h +++ b/src/commands/commander.h @@ -56,20 +56,30 @@ class Connection; struct CommandAttributes; enum CommandFlags : uint64_t { - kCmdWrite = 1ULL << 0, // "write" flag - kCmdReadOnly = 1ULL << 1, // "read-only" flag - kCmdReplication = 1ULL << 2, // "replication" flag - kCmdPubSub = 1ULL << 3, // "pub-sub" flag - kCmdScript = 1ULL << 4, // "script" flag - kCmdLoading = 1ULL << 5, // "ok-loading" flag - kCmdMulti = 1ULL << 6, // "multi" flag - kCmdExclusive = 1ULL << 7, // "exclusive" flag - kCmdNoMulti = 1ULL << 8, // "no-multi" flag - kCmdNoScript = 1ULL << 9, // "no-script" flag - kCmdROScript = 1ULL << 10, // "ro-script" flag for read-only script commands - kCmdCluster = 1ULL << 11, // "cluster" flag - kCmdNoDBSizeCheck = 1ULL << 12, // "no-dbsize-check" flag - kCmdSlow = 1ULL << 13, // "slow" flag + // "write" flag, for any command that performs rocksdb writing ops + kCmdWrite = 1ULL << 0, + // "read-only" flag, for any command that performs rocksdb reading ops + // and doesn't perform rocksdb writing ops + kCmdReadOnly = 1ULL << 1, + // "ok-loading" flag, for any command that can be executed while + // the db is in loading phase + kCmdLoading = 1ULL << 5, + // "multi" flag, for commands that can end a MULTI scope + kCmdEndMulti = 1ULL << 6, + // "exclusive" flag, for commands that should be executed execlusive globally + kCmdExclusive = 1ULL << 7, + // "no-multi" flag, for commands that cannot be executed in MULTI scope + kCmdNoMulti = 1ULL << 8, + // "no-script" flag, for commands that cannot be executed in scripting + kCmdNoScript = 1ULL << 9, + // "no-dbsize-check" flag, for commands that can ignore the db size checking + kCmdNoDBSizeCheck = 1ULL << 12, + // "slow" flag, for commands that run slowly, + // usually with a non-constant number of rocksdb ops + kCmdSlow = 1ULL << 13, + // "blocking" flag, for commands that don't perform db ops immediately, + // but block and wait for some event to happen before performing db ops + kCmdBlocking = 1ULL << 14, }; enum class CommandCategory : uint8_t { @@ -302,28 +312,22 @@ inline uint64_t ParseCommandFlags(const std::string &description, const std::str flags |= kCmdWrite; else if (flag == "read-only") flags |= kCmdReadOnly; - else if (flag == "replication") - flags |= kCmdReplication; - else if (flag == "pub-sub") - flags |= kCmdPubSub; else if (flag == "ok-loading") flags |= kCmdLoading; else if (flag == "exclusive") flags |= kCmdExclusive; else if (flag == "multi") - flags |= kCmdMulti; + flags |= kCmdEndMulti; else if (flag == "no-multi") flags |= kCmdNoMulti; else if (flag == "no-script") flags |= kCmdNoScript; - else if (flag == "ro-script") - flags |= kCmdROScript; - else if (flag == "cluster") - flags |= kCmdCluster; else if (flag == "no-dbsize-check") flags |= kCmdNoDBSizeCheck; else if (flag == "slow") flags |= kCmdSlow; + else if (flag == "blocking") + flags |= kCmdBlocking; else { std::cout << fmt::format("Encountered non-existent flag '{}' in command {} in command attribute parsing", flag, cmd_name) diff --git a/src/commands/scan_base.h b/src/commands/scan_base.h index b3773b94ff7..5a0d4ca17c1 100644 --- a/src/commands/scan_base.h +++ b/src/commands/scan_base.h @@ -23,8 +23,10 @@ #include "commander.h" #include "commands/command_parser.h" #include "error_constants.h" +#include "glob.h" #include "parse_util.h" #include "server/server.h" +#include "string_util.h" namespace redis { @@ -44,14 +46,11 @@ class CommandScanBase : public Commander { Status ParseAdditionalFlags(Parser &parser) { while (parser.Good()) { if (parser.EatEqICase("match")) { - prefix_ = GET_OR_RET(parser.TakeStr()); - // The match pattern should contain exactly one '*' at the end; remove the * to - // get the prefix to match. - if (!prefix_.empty() && prefix_.find('*') == prefix_.size() - 1) { - prefix_.pop_back(); - } else { - return {Status::RedisParseErr, "currently only key prefix matching is supported"}; + const std::string glob_pattern = GET_OR_RET(parser.TakeStr()); + if (const Status s = util::ValidateGlob(glob_pattern); !s.IsOK()) { + return {Status::RedisParseErr, "Invalid glob pattern: " + s.Msg()}; } + std::tie(prefix_, suffix_glob_) = util::SplitGlob(glob_pattern); } else if (parser.EatEqICase("count")) { limit_ = GET_OR_RET(parser.TakeInt()); if (limit_ <= 0) { @@ -100,6 +99,7 @@ class CommandScanBase : public Commander { protected: std::string cursor_; std::string prefix_; + std::string suffix_glob_ = "*"; int limit_ = 20; RedisType type_ = kRedisNone; }; diff --git a/src/common/status.h b/src/common/status.h index 2bd610ad904..b4b228a05ef 100644 --- a/src/common/status.h +++ b/src/common/status.h @@ -26,10 +26,10 @@ #include #include #include -#include #include #include +#include "rocksdb/status.h" #include "type_util.h" class [[nodiscard]] Status { @@ -370,10 +370,30 @@ struct [[nodiscard]] StatusOr { friend struct StatusOr; }; +template >::value || std::is_same_v, Status>, int> = 0> +decltype(auto) StatusGetValue(T&& v) { + return std::forward(v).GetValue(); +} + +template , rocksdb::Status>, int> = 0> +void StatusGetValue(T&&) {} + +template >::value || std::is_same_v, Status>, int> = 0> +bool StatusIsOK(const T& v) { + return v.IsOK(); +} + +template , rocksdb::Status>, int> = 0> +bool StatusIsOK(const T& v) { + return v.ok(); +} + // NOLINTNEXTLINE -#define GET_OR_RET(...) \ - ({ \ - auto&& status = (__VA_ARGS__); \ - if (!status) return std::forward(status); \ - std::forward(status); \ - }).GetValue() +#define GET_OR_RET(...) \ + StatusGetValue(({ \ + auto&& status = (__VA_ARGS__); \ + if (!StatusIsOK(status)) return std::forward(status); \ + std::forward(status); \ + })) diff --git a/src/common/string_util.cc b/src/common/string_util.cc index 476e3173bee..cce6440227a 100644 --- a/src/common/string_util.cc +++ b/src/common/string_util.cc @@ -101,118 +101,174 @@ bool HasPrefix(const std::string &str, const std::string &prefix) { return !strncasecmp(str.data(), prefix.data(), prefix.size()); } -int StringMatch(const std::string &pattern, const std::string &in, int nocase) { - return StringMatchLen(pattern.c_str(), pattern.length(), in.c_str(), in.length(), nocase); +Status ValidateGlob(std::string_view glob) { + for (size_t idx = 0; idx < glob.size(); ++idx) { + switch (glob[idx]) { + case '*': + case '?': + break; + case ']': + return {Status::NotOK, "Unmatched unescaped ]"}; + case '\\': + if (idx == glob.size() - 1) { + return {Status::NotOK, "Trailing unescaped backslash"}; + } + // Skip the next character: this is a literal so nothing can go wrong + idx++; + break; + case '[': + idx++; // Skip the opening bracket + while (idx < glob.size() && glob[idx] != ']') { + if (glob[idx] == '\\') { + idx += 2; + continue; + } else if (idx + 1 < glob.size() && glob[idx + 1] == '-') { + if (idx + 2 >= glob.size()) { + return {Status::NotOK, "Unterminated character range"}; + } + // Skip the - and the end of the range + idx += 2; + } + idx++; + } + if (idx == glob.size()) { + return {Status::NotOK, "Unterminated [ group"}; + } + break; + default: + // This is a literal: nothing can go wrong + break; + } + } + return Status::OK(); } -// Glob-style pattern matching. -int StringMatchLen(const char *pattern, size_t pattern_len, const char *string, size_t string_len, int nocase) { - while (pattern_len && string_len) { +constexpr bool StringMatchImpl(std::string_view pattern, std::string_view string, bool ignore_case, + bool *skip_longer_matches, size_t recursion_depth = 0) { + // If we want to ignore case, this is equivalent to converting both the pattern and the string to lowercase + const auto canonicalize = [ignore_case](unsigned char c) -> unsigned char { + return ignore_case ? static_cast(std::tolower(c)) : c; + }; + + if (recursion_depth > 1000) return false; + + while (!pattern.empty() && !string.empty()) { switch (pattern[0]) { case '*': - while (pattern[1] == '*') { - pattern++; - pattern_len--; + // Optimization: collapse multiple * into one + while (pattern.size() >= 2 && pattern[1] == '*') { + pattern.remove_prefix(1); } - - if (pattern_len == 1) return 1; /* match */ - - while (string_len) { - if (StringMatchLen(pattern + 1, pattern_len - 1, string, string_len, nocase)) return 1; /* match */ - string++; - string_len--; + // Optimization: If the '*' is the last character in the pattern, it can match anything + if (pattern.length() == 1) return true; + while (!string.empty()) { + if (StringMatchImpl(pattern.substr(1), string, ignore_case, skip_longer_matches, recursion_depth + 1)) + return true; + if (*skip_longer_matches) return false; + string.remove_prefix(1); } - return 0; /* no match */ + // There was no match for the rest of the pattern starting + // from anywhere in the rest of the string. If there were + // any '*' earlier in the pattern, we can terminate the + // search early without trying to match them to longer + // substrings. This is because a longer match for the + // earlier part of the pattern would require the rest of the + // pattern to match starting later in the string, and we + // have just determined that there is no match for the rest + // of the pattern starting from anywhere in the current + // string. + *skip_longer_matches = true; + return false; case '?': - string++; - string_len--; + if (string.empty()) return false; + string.remove_prefix(1); break; case '[': { - pattern++; - pattern_len--; - int not_symbol = pattern[0] == '^'; - if (not_symbol) { - pattern++; - pattern_len--; - } + pattern.remove_prefix(1); + const bool invert = pattern[0] == '^'; + if (invert) pattern.remove_prefix(1); - int match = 0; + bool match = false; while (true) { - if (pattern[0] == '\\' && pattern_len >= 2) { - pattern++; - pattern_len--; - if (pattern[0] == string[0]) match = 1; + if (pattern.empty()) { + // unterminated [ group: reject invalid pattern + return false; } else if (pattern[0] == ']') { break; - } else if (pattern_len == 0) { - pattern--; - pattern_len++; - break; - } else if (pattern[1] == '-' && pattern_len >= 3) { - int start = pattern[0]; - int end = pattern[2]; - int c = string[0]; - if (start > end) { - int t = start; - start = end; - end = t; - } - if (nocase) { - start = tolower(start); - end = tolower(end); - c = tolower(c); - } - pattern += 2; - pattern_len -= 2; - if (c >= start && c <= end) match = 1; - } else { - if (!nocase) { - if (pattern[0] == string[0]) match = 1; - } else { - if (tolower(static_cast(pattern[0])) == tolower(static_cast(string[0]))) match = 1; - } + } else if (pattern.length() >= 2 && pattern[0] == '\\') { + pattern.remove_prefix(1); + if (pattern[0] == string[0]) match = true; + } else if (pattern.length() >= 3 && pattern[1] == '-') { + unsigned char start = canonicalize(pattern[0]); + unsigned char end = canonicalize(pattern[2]); + if (start > end) std::swap(start, end); + const int c = canonicalize(string[0]); + pattern.remove_prefix(2); + + if (c >= start && c <= end) match = true; + } else if (canonicalize(pattern[0]) == canonicalize(string[0])) { + match = true; } - pattern++; - pattern_len--; + pattern.remove_prefix(1); } - - if (not_symbol) match = !match; - - if (!match) return 0; /* no match */ - - string++; - string_len--; + if (invert) match = !match; + if (!match) return false; + string.remove_prefix(1); break; } case '\\': - if (pattern_len >= 2) { - pattern++; - pattern_len--; + if (pattern.length() >= 2) { + pattern.remove_prefix(1); } - /* fall through */ + [[fallthrough]]; default: - if (!nocase) { - if (pattern[0] != string[0]) return 0; /* no match */ + // Just a normal character + if (!ignore_case) { + if (pattern[0] != string[0]) return false; } else { - if (tolower(static_cast(pattern[0])) != tolower(static_cast(string[0]))) return 0; /* no match */ + if (std::tolower((int)pattern[0]) != std::tolower((int)string[0])) return false; } - string++; - string_len--; + string.remove_prefix(1); break; } - pattern++; - pattern_len--; - if (string_len == 0) { - while (*pattern == '*') { - pattern++; - pattern_len--; - } - break; - } + pattern.remove_prefix(1); } - if (pattern_len == 0 && string_len == 0) return 1; - return 0; + // Now that either the pattern is empty or the string is empty, this is a match iff + // the pattern consists only of '*', and the string is empty. + return string.empty() && std::all_of(pattern.begin(), pattern.end(), [](char c) { return c == '*'; }); +} + +// Given a glob [pattern] and a string [string], return true iff the string matches the glob. +// If [ignore_case] is true, the match is case-insensitive. +bool StringMatch(std::string_view glob, std::string_view str, bool ignore_case) { + bool skip_longer_matches = false; + return StringMatchImpl(glob, str, ignore_case, &skip_longer_matches); +} + +// Split a glob pattern into a literal prefix and a suffix containing wildcards. +// For example, if the user calls [KEYS bla*bla], this function will return {"bla", "*bla"}. +// This allows the caller of this function to optimize this call by performing a +// prefix-scan on "bla" and then filtering the results using the GlobMatches function. +std::pair SplitGlob(std::string_view glob) { + // Stores the prefix of the glob pattern, with backslashes removed + std::string prefix; + // Find the first un-escaped '*', '?' or '[' character in [glob] + for (size_t idx = 0; idx < glob.size(); ++idx) { + if (glob[idx] == '*' || glob[idx] == '?' || glob[idx] == '[') { + // Return a pair of views: the part of the glob before the wildcard, and the part after + return {prefix, std::string(glob.substr(idx))}; + } else if (glob[idx] == '\\') { + // Skip checking whether the next character is a special character + ++idx; + // Append the escaped special character to the prefix + if (idx < glob.size()) prefix.push_back(glob[idx]); + } else { + prefix.push_back(glob[idx]); + } + } + // No wildcard found, return the entire string (without the backslashes) as the prefix + return {prefix, ""}; } std::vector RegexMatch(const std::string &str, const std::string ®ex) { diff --git a/src/common/string_util.h b/src/common/string_util.h index 2dcb10800f5..f86590ad046 100644 --- a/src/common/string_util.h +++ b/src/common/string_util.h @@ -20,7 +20,13 @@ #pragma once -#include "status.h" +#include +#include +#include +#include +#include + +#include "common/status.h" namespace util { @@ -32,8 +38,11 @@ std::string Trim(std::string in, std::string_view chars); std::vector Split(std::string_view in, std::string_view delim); std::vector Split2KV(const std::string &in, const std::string &delim); bool HasPrefix(const std::string &str, const std::string &prefix); -int StringMatch(const std::string &pattern, const std::string &in, int nocase); -int StringMatchLen(const char *p, size_t plen, const char *s, size_t slen, int nocase); + +Status ValidateGlob(std::string_view glob); +bool StringMatch(std::string_view glob, std::string_view str, bool ignore_case = false); +std::pair SplitGlob(std::string_view glob); + std::vector RegexMatch(const std::string &str, const std::string ®ex); std::string StringToHex(std::string_view input); std::vector TokenizeRedisProtocol(const std::string &value); diff --git a/src/config/config.cc b/src/config/config.cc index 922ceb9807a..e15144ed29b 100644 --- a/src/config/config.cc +++ b/src/config/config.cc @@ -904,7 +904,7 @@ Status Config::Load(const CLIOptions &opts) { void Config::Get(const std::string &key, std::vector *values) const { values->clear(); for (const auto &iter : fields_) { - if (util::StringMatch(key, iter.first, 1)) { + if (util::StringMatch(key, iter.first, true)) { if (iter.second->IsMultiConfig()) { for (const auto &p : util::Split(iter.second->ToString(), "\n")) { values->emplace_back(iter.first); diff --git a/src/server/redis_connection.cc b/src/server/redis_connection.cc index 24ea47b9051..54899afe153 100644 --- a/src/server/redis_connection.cc +++ b/src/server/redis_connection.cc @@ -417,19 +417,10 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { // No lock guard, because 'exec' command has acquired 'WorkExclusivityGuard' } else if (cmd_flags & kCmdExclusive) { exclusivity = srv_->WorkExclusivityGuard(); - - // When executing lua script commands that have "exclusive" attribute, we need to know current connection, - // but we should set current connection after acquiring the WorkExclusivityGuard to make it thread-safe - srv_->SetCurrentConnection(this); } else { concurrency = srv_->WorkConcurrencyGuard(); } - if (cmd_flags & kCmdROScript) { - // if executing read only lua script commands, set current connection. - srv_->SetCurrentConnection(this); - } - if (srv_->IsLoading() && !(cmd_flags & kCmdLoading)) { Reply(redis::Error({Status::RedisLoading, errRestoringBackup})); if (is_multi_exec) multi_error_ = true; @@ -471,8 +462,8 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { DisableFlag(kAsking); } - // We don't execute commands, but queue them, ant then execute in EXEC command - if (is_multi_exec && !in_exec_ && !(cmd_flags & kCmdMulti)) { + // We don't execute commands, but queue them, and then execute in EXEC command + if (is_multi_exec && !in_exec_ && !(cmd_flags & kCmdEndMulti)) { multi_cmds_.emplace_back(cmd_tokens); Reply(redis::SimpleString("QUEUED")); continue; @@ -498,9 +489,24 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { SetLastCmd(cmd_name); { + std::optional guard; + if (cmd_flags & kCmdWrite) { + std::vector lock_keys; + attributes->ForEachKeyRange( + [&lock_keys, this](const std::vector &args, const CommandKeyRange &key_range) { + key_range.ForEachKey( + [&, this](const std::string &key) { + auto ns_key = ComposeNamespaceKey(ns_, key, srv_->storage->IsSlotIdEncoded()); + lock_keys.emplace_back(std::move(ns_key)); + }, + args); + }, + cmd_tokens); + + guard.emplace(srv_->storage->GetLockManager(), lock_keys); + } engine::Context ctx(srv_->storage); - // TODO: transaction support for index recording std::vector index_records; if (!srv_->index_mgr.index_map.empty() && IsCmdForIndexing(cmd_flags, attributes->category) && !config->cluster_enabled) { @@ -521,7 +527,6 @@ void Connection::ExecuteCommands(std::deque *to_process_cmds) { } s = ExecuteCommand(ctx, cmd_name, cmd_tokens, current_cmd.get(), &reply); - // TODO: transaction support for index updating for (const auto &record : index_records) { auto s = GlobalIndexer::Update(ctx, record); if (!s.IsOK() && !s.Is()) { diff --git a/src/server/redis_reply.cc b/src/server/redis_reply.cc index d01c823a60e..11a7eb12b4c 100644 --- a/src/server/redis_reply.cc +++ b/src/server/redis_reply.cc @@ -37,7 +37,14 @@ namespace redis { void Reply(evbuffer *output, const std::string &data) { evbuffer_add(output, data.c_str(), data.length()); } -std::string SimpleString(const std::string &data) { return "+" + data + CRLF; } +std::string SimpleString(std::string_view data) { + std::string res; + res.reserve(data.size() + 3); // 1 for '+', 2 for CRLF + res += RESP_PREFIX_SIMPLE_STRING; + res += data; + res += CRLF; + return res; +} std::string Error(const Status &s) { return RESP_PREFIX_ERROR + StatusToRedisErrorMsg(s) + CRLF; } diff --git a/src/server/redis_reply.h b/src/server/redis_reply.h index 9442e8c2056..4a80124b2c4 100644 --- a/src/server/redis_reply.h +++ b/src/server/redis_reply.h @@ -24,6 +24,7 @@ #include #include +#include #include #include "rocksdb/status.h" @@ -39,7 +40,9 @@ namespace redis { enum class RESP { v2, v3 }; void Reply(evbuffer *output, const std::string &data); -std::string SimpleString(const std::string &data); +std::string SimpleString(std::string_view data); + +static const inline std::string RESP_OK = RESP_PREFIX_SIMPLE_STRING "OK" CRLF; std::string Error(const Status &s); std::string StatusToRedisErrorMsg(const Status &s); diff --git a/src/server/server.cc b/src/server/server.cc index 1de5534dc54..5b52eb333fb 100644 --- a/src/server/server.cc +++ b/src/server/server.cc @@ -38,7 +38,7 @@ #include #include "commands/commander.h" -#include "config.h" +#include "common/string_util.h" #include "config/config.h" #include "fmt/format.h" #include "redis_connection.h" @@ -46,7 +46,6 @@ #include "storage/redis_db.h" #include "storage/scripting.h" #include "storage/storage.h" -#include "string_util.h" #include "thread_util.h" #include "time_util.h" #include "version.h" @@ -103,7 +102,7 @@ Server::Server(engine::Storage *storage, Config *config) AdjustOpenFilesLimit(); slow_log_.SetMaxEntries(config->slowlog_max_len); perf_log_.SetMaxEntries(config->profiling_sample_record_max_len); - lua_ = lua::CreateState(this); + lua_ = lua::CreateState(); } Server::~Server() { @@ -160,7 +159,7 @@ Status Server::Start() { if (!config_->cluster_enabled) { engine::Context no_txn_ctx = engine::Context::NoTransactionContext(storage); GET_OR_RET(index_mgr.Load(no_txn_ctx, kDefaultNamespace)); - for (auto [_, ns] : namespace_.List()) { + for (const auto &[_, ns] : namespace_.List()) { GET_OR_RET(index_mgr.Load(no_txn_ctx, ns)); } } @@ -391,7 +390,7 @@ int Server::PublishMessage(const std::string &channel, const std::string &msg) { std::vector patterns; std::vector to_publish_patterns_conn_ctxs; for (const auto &iter : pubsub_patterns_) { - if (util::StringMatch(iter.first, channel, 0)) { + if (util::StringMatch(iter.first, channel, false)) { for (const auto &conn_ctx : iter.second) { to_publish_patterns_conn_ctxs.emplace_back(conn_ctx); patterns.emplace_back(iter.first); @@ -463,7 +462,7 @@ void Server::GetChannelsByPattern(const std::string &pattern, std::vector guard(pubsub_channels_mu_); for (const auto &iter : pubsub_channels_) { - if (pattern.empty() || util::StringMatch(pattern, iter.first, 0)) { + if (pattern.empty() || util::StringMatch(pattern, iter.first, false)) { channels->emplace_back(iter.first); } } @@ -549,7 +548,7 @@ void Server::GetSChannelsByPattern(const std::string &pattern, std::vectoremplace_back(iter.first); } } @@ -1765,7 +1764,7 @@ Status Server::FunctionSetLib(const std::string &func, const std::string &lib) c } void Server::ScriptReset() { - auto lua = lua_.exchange(lua::CreateState(this)); + auto lua = lua_.exchange(lua::CreateState()); lua::DestroyState(lua); } diff --git a/src/server/server.h b/src/server/server.h index 7d8c8327f05..3c2ab16e997 100644 --- a/src/server/server.h +++ b/src/server/server.h @@ -285,9 +285,6 @@ class Server { Status ExecPropagatedCommand(const std::vector &tokens); Status ExecPropagateScriptCommand(const std::vector &tokens); - void SetCurrentConnection(redis::Connection *conn) { curr_connection_ = conn; } - redis::Connection *GetCurrentConnection() { return curr_connection_; } - LogCollector *GetPerfLog() { return &perf_log_; } LogCollector *GetSlowLog() { return &slow_log_; } void SlowlogPushEntryIfNeeded(const std::vector *args, uint64_t duration, const redis::Connection *conn); @@ -343,8 +340,6 @@ class Server { std::atomic lua_; - redis::Connection *curr_connection_ = nullptr; - // client counters std::atomic client_id_{1}; std::atomic connected_clients_{0}; diff --git a/src/server/worker.cc b/src/server/worker.cc index 4ddf31add0b..6420d76c361 100644 --- a/src/server/worker.cc +++ b/src/server/worker.cc @@ -76,7 +76,7 @@ Worker::Worker(Server *srv, Config *config) : srv(srv), base_(event_base_new()) } } } - lua_ = lua::CreateState(srv); + lua_ = lua::CreateState(); } Worker::~Worker() { diff --git a/src/storage/redis_db.cc b/src/storage/redis_db.cc index 7fe83477e9a..26864f3ee8b 100644 --- a/src/storage/redis_db.cc +++ b/src/storage/redis_db.cc @@ -21,16 +21,15 @@ #include "redis_db.h" #include -#include #include #include "cluster/redis_slot.h" #include "common/scope_exit.h" +#include "common/string_util.h" #include "db_util.h" #include "parse_util.h" #include "rocksdb/iterator.h" #include "rocksdb/status.h" -#include "server/server.h" #include "storage/iterator.h" #include "storage/redis_metadata.h" #include "storage/storage.h" @@ -113,7 +112,7 @@ rocksdb::Status Database::Expire(engine::Context &ctx, const Slice &user_key, ui std::string value; Metadata metadata(kRedisNone, false); - LockGuard guard(storage_->GetLockManager(), ns_key); + rocksdb::Status s = storage_->Get(ctx, ctx.GetReadOptions(), metadata_cf_handle_, ns_key, &value); if (!s.ok()) return s; @@ -151,7 +150,7 @@ rocksdb::Status Database::Del(engine::Context &ctx, const Slice &user_key) { std::string ns_key = AppendNamespacePrefix(user_key); std::string value; - LockGuard guard(storage_->GetLockManager(), ns_key); + rocksdb::Status s = storage_->Get(ctx, ctx.GetReadOptions(), metadata_cf_handle_, ns_key, &value); if (!s.ok()) return s; Metadata metadata(kRedisNone, false); @@ -166,13 +165,12 @@ rocksdb::Status Database::Del(engine::Context &ctx, const Slice &user_key) { rocksdb::Status Database::MDel(engine::Context &ctx, const std::vector &keys, uint64_t *deleted_cnt) { *deleted_cnt = 0; - std::vector lock_keys; - lock_keys.reserve(keys.size()); + std::vector ns_keys; + ns_keys.reserve(keys.size()); for (const auto &key : keys) { std::string ns_key = AppendNamespacePrefix(key); - lock_keys.emplace_back(std::move(ns_key)); + ns_keys.emplace_back(std::move(ns_key)); } - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); auto batch = storage_->GetWriteBatchBase(); WriteBatchLogData log_data(kRedisNone); @@ -182,8 +180,8 @@ rocksdb::Status Database::MDel(engine::Context &ctx, const std::vector &k } std::vector slice_keys; - slice_keys.reserve(lock_keys.size()); - for (const auto &ns_key : lock_keys) { + slice_keys.reserve(ns_keys.size()); + for (const auto &ns_key : ns_keys) { slice_keys.emplace_back(ns_key); } @@ -203,7 +201,7 @@ rocksdb::Status Database::MDel(engine::Context &ctx, const std::vector &k if (!s.ok()) continue; if (metadata.Expired()) continue; - s = batch->Delete(metadata_cf_handle_, lock_keys[i]); + s = batch->Delete(metadata_cf_handle_, ns_keys[i]); if (!s.ok()) return s; *deleted_cnt += 1; } @@ -249,11 +247,11 @@ rocksdb::Status Database::GetExpireTime(engine::Context &ctx, const Slice &user_ } rocksdb::Status Database::GetKeyNumStats(engine::Context &ctx, const std::string &prefix, KeyNumStats *stats) { - return Keys(ctx, prefix, nullptr, stats); + return Keys(ctx, prefix, "*", nullptr, stats); } -rocksdb::Status Database::Keys(engine::Context &ctx, const std::string &prefix, std::vector *keys, - KeyNumStats *stats) { +rocksdb::Status Database::Keys(engine::Context &ctx, const std::string &prefix, const std::string &suffix_glob, + std::vector *keys, KeyNumStats *stats) { uint16_t slot_id = 0; std::string ns_prefix; if (namespace_ != kDefaultNamespace || keys != nullptr) { @@ -277,6 +275,10 @@ rocksdb::Status Database::Keys(engine::Context &ctx, const std::string &prefix, if (!ns_prefix.empty() && !iter->key().starts_with(ns_prefix)) { break; } + auto [_, user_key] = ExtractNamespaceKey(iter->key(), storage_->IsSlotIdEncoded()); + if (!util::StringMatch(suffix_glob, user_key.ToString().substr(prefix.size()))) { + continue; + } Metadata metadata(kRedisNone, false); auto s = metadata.Decode(iter->value()); if (!s.ok()) continue; @@ -293,7 +295,6 @@ rocksdb::Status Database::Keys(engine::Context &ctx, const std::string &prefix, } } if (keys) { - auto [_, user_key] = ExtractNamespaceKey(iter->key(), storage_->IsSlotIdEncoded()); keys->emplace_back(user_key.ToString()); } } @@ -319,8 +320,8 @@ rocksdb::Status Database::Keys(engine::Context &ctx, const std::string &prefix, } rocksdb::Status Database::Scan(engine::Context &ctx, const std::string &cursor, uint64_t limit, - const std::string &prefix, std::vector *keys, std::string *end_cursor, - RedisType type) { + const std::string &prefix, const std::string &suffix_glob, + std::vector *keys, std::string *end_cursor, RedisType type) { end_cursor->clear(); uint64_t cnt = 0; uint16_t slot_start = 0; @@ -366,6 +367,10 @@ rocksdb::Status Database::Scan(engine::Context &ctx, const std::string &cursor, if (metadata.Expired()) continue; std::tie(std::ignore, user_key) = ExtractNamespaceKey(iter->key(), storage_->IsSlotIdEncoded()); + + if (!util::StringMatch(suffix_glob, user_key.substr(prefix.size()))) { + continue; + } keys->emplace_back(user_key); cnt++; } @@ -395,7 +400,7 @@ rocksdb::Status Database::Scan(engine::Context &ctx, const std::string &cursor, if (iter->Valid()) { std::tie(std::ignore, user_key) = ExtractNamespaceKey(iter->key(), storage_->IsSlotIdEncoded()); auto res = std::mismatch(prefix.begin(), prefix.end(), user_key.begin()); - if (res.first == prefix.end()) { + if (res.first == prefix.end() && util::StringMatch(suffix_glob, user_key.substr(prefix.size()))) { keys->emplace_back(user_key); } @@ -420,13 +425,13 @@ rocksdb::Status Database::RandomKey(engine::Context &ctx, const std::string &cur std::string end_cursor; std::vector keys; - auto s = Scan(ctx, cursor, RANDOM_KEY_SCAN_LIMIT, "", &keys, &end_cursor); + auto s = Scan(ctx, cursor, RANDOM_KEY_SCAN_LIMIT, "", "*", &keys, &end_cursor); if (!s.ok()) { return s; } if (keys.empty() && !cursor.empty()) { // if reach the end, restart from beginning - s = Scan(ctx, "", RANDOM_KEY_SCAN_LIMIT, "", &keys, &end_cursor); + s = Scan(ctx, "", RANDOM_KEY_SCAN_LIMIT, "", "*", &keys, &end_cursor); if (!s.ok()) { return s; } @@ -646,9 +651,6 @@ rocksdb::Status Database::typeInternal(engine::Context &ctx, const Slice &key, R rocksdb::Status Database::Copy(engine::Context &ctx, const std::string &key, const std::string &new_key, bool nx, bool delete_old, CopyResult *res) { - std::vector lock_keys = {key, new_key}; - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); - RedisType type = kRedisNone; auto s = typeInternal(ctx, key, &type); if (!s.ok()) return s; diff --git a/src/storage/redis_db.h b/src/storage/redis_db.h index 7111fed1099..41ed3daeb24 100644 --- a/src/storage/redis_db.h +++ b/src/storage/redis_db.h @@ -20,7 +20,6 @@ #pragma once -#include #include #include #include @@ -29,7 +28,6 @@ #include "cluster/cluster_defs.h" #include "redis_metadata.h" -#include "server/redis_reply.h" #include "storage.h" namespace redis { @@ -119,11 +117,12 @@ class Database { [[nodiscard]] rocksdb::Status FlushDB(engine::Context &ctx); [[nodiscard]] rocksdb::Status FlushAll(engine::Context &ctx); [[nodiscard]] rocksdb::Status GetKeyNumStats(engine::Context &ctx, const std::string &prefix, KeyNumStats *stats); - [[nodiscard]] rocksdb::Status Keys(engine::Context &ctx, const std::string &prefix, + [[nodiscard]] rocksdb::Status Keys(engine::Context &ctx, const std::string &prefix, const std::string &suffix_glob, std::vector *keys = nullptr, KeyNumStats *stats = nullptr); [[nodiscard]] rocksdb::Status Scan(engine::Context &ctx, const std::string &cursor, uint64_t limit, - const std::string &prefix, std::vector *keys, - std::string *end_cursor = nullptr, RedisType type = kRedisNone); + const std::string &prefix, const std::string &suffix_glob, + std::vector *keys, std::string *end_cursor = nullptr, + RedisType type = kRedisNone); [[nodiscard]] rocksdb::Status RandomKey(engine::Context &ctx, const std::string &cursor, std::string *key); std::string AppendNamespacePrefix(const Slice &user_key); [[nodiscard]] rocksdb::Status ClearKeysOfSlotRange(engine::Context &ctx, const rocksdb::Slice &ns, diff --git a/src/storage/scripting.cc b/src/storage/scripting.cc index f38d942237f..5768aee8169 100644 --- a/src/storage/scripting.cc +++ b/src/storage/scripting.cc @@ -57,15 +57,12 @@ enum { namespace lua { -lua_State *CreateState(Server *srv) { +lua_State *CreateState() { lua_State *lua = lua_open(); LoadLibraries(lua); RemoveUnsupportedFunctions(lua); LoadFuncs(lua); - lua_pushlightuserdata(lua, srv); - lua_setglobal(lua, REDIS_LUA_SERVER_PTR); - EnableGlobalsProtection(lua); return lua; } @@ -273,7 +270,10 @@ int RedisRegisterFunction(lua_State *lua) { } // store the map from function name to library name - auto s = GetServer(lua)->FunctionSetLib(name, libname); + auto *script_run_ctx = GetFromRegistry(lua, REGISTRY_SCRIPT_RUN_CTX_NAME); + CHECK_NOTNULL(script_run_ctx); + + auto s = script_run_ctx->conn->GetServer()->FunctionSetLib(name, libname); if (!s) { lua_pushstring(lua, "redis.register_function() failed to store informantion."); return lua_error(lua); @@ -305,6 +305,12 @@ Status FunctionLoad(redis::Connection *conn, const std::string &script, bool nee if (!s) return s; } + ScriptRunCtx script_run_ctx; + script_run_ctx.conn = conn; + script_run_ctx.flags = read_only ? ScriptFlagType::kScriptNoWrites : 0; + + SaveOnRegistry(lua, REGISTRY_SCRIPT_RUN_CTX_NAME, &script_run_ctx); + lua_pushstring(lua, libname.c_str()); lua_setglobal(lua, REDIS_FUNCTION_LIBNAME); auto libname_exit = MakeScopeExit([lua] { @@ -331,6 +337,8 @@ Status FunctionLoad(redis::Connection *conn, const std::string &script, bool nee return {Status::NotOK, "Error while running new function lib: " + err_msg}; } + RemoveFromRegistry(lua, REGISTRY_SCRIPT_RUN_CTX_NAME); + if (!FunctionIsLibExist(conn, libname, false, read_only)) { return {Status::NotOK, "Please register some function in FUNCTION LOAD"}; } @@ -396,6 +404,7 @@ Status FunctionCall(redis::Connection *conn, const std::string &name, const std: } ScriptRunCtx script_run_ctx; + script_run_ctx.conn = conn; script_run_ctx.flags = read_only ? ScriptFlagType::kScriptNoWrites : 0; lua_getglobal(lua, (REDIS_LUA_REGISTER_FUNC_FLAGS_PREFIX + name).c_str()); if (!lua_isnil(lua, -1)) { @@ -642,6 +651,7 @@ Status EvalGenericCommand(redis::Connection *conn, const std::string &body_or_sh } ScriptRunCtx current_script_run_ctx; + current_script_run_ctx.conn = conn; current_script_run_ctx.flags = read_only ? ScriptFlagType::kScriptNoWrites : 0; lua_getglobal(lua, fmt::format(REDIS_LUA_FUNC_SHA_FLAGS, funcname + 2).c_str()); if (!lua_isnil(lua, -1)) { @@ -709,14 +719,6 @@ int RedisCallCommand(lua_State *lua) { return RedisGenericCommand(lua, 1); } int RedisPCallCommand(lua_State *lua) { return RedisGenericCommand(lua, 0); } -Server *GetServer(lua_State *lua) { - lua_getglobal(lua, REDIS_LUA_SERVER_PTR); - auto srv = reinterpret_cast(lua_touserdata(lua, -1)); - lua_pop(lua, 1); - - return srv; -} - // TODO: we do not want to repeat same logic as Connection::ExecuteCommands, // so the function need to be refactored int RedisGenericCommand(lua_State *lua, int raise_error) { @@ -772,10 +774,10 @@ int RedisGenericCommand(lua_State *lua, int raise_error) { std::string cmd_name = attributes->name; - auto srv = GetServer(lua); + auto *conn = script_run_ctx->conn; + auto *srv = conn->GetServer(); Config *config = srv->GetConfig(); - redis::Connection *conn = srv->GetCurrentConnection(); if (config->cluster_enabled) { if (script_run_ctx->flags & ScriptFlagType::kScriptNoCluster) { PushError(lua, "Can not run script on cluster, 'no-cluster' flag is set"); @@ -901,8 +903,10 @@ int RedisReturnSingleFieldTable(lua_State *lua, const char *field) { } int RedisSetResp(lua_State *lua) { - auto srv = GetServer(lua); - auto conn = srv->GetCurrentConnection(); + auto *script_run_ctx = GetFromRegistry(lua, REGISTRY_SCRIPT_RUN_CTX_NAME); + CHECK_NOTNULL(script_run_ctx); + auto *conn = script_run_ctx->conn; + auto *srv = conn->GetServer(); if (lua_gettop(lua) != 1) { PushError(lua, "redis.setresp() requires one argument."); diff --git a/src/storage/scripting.h b/src/storage/scripting.h index 9aa4044ba3c..188f855c9ba 100644 --- a/src/storage/scripting.h +++ b/src/storage/scripting.h @@ -35,7 +35,6 @@ inline constexpr const char REDIS_LUA_FUNC_SHA_PREFIX[] = "f_"; inline constexpr const char REDIS_LUA_FUNC_SHA_FLAGS[] = "f_{}_flags_"; inline constexpr const char REDIS_LUA_REGISTER_FUNC_PREFIX[] = "__redis_registered_"; inline constexpr const char REDIS_LUA_REGISTER_FUNC_FLAGS_PREFIX[] = "__redis_registered_flags_"; -inline constexpr const char REDIS_LUA_SERVER_PTR[] = "__server_ptr"; inline constexpr const char REDIS_FUNCTION_LIBNAME[] = "REDIS_FUNCTION_LIBNAME"; inline constexpr const char REDIS_FUNCTION_NEEDSTORE[] = "REDIS_FUNCTION_NEEDSTORE"; inline constexpr const char REDIS_FUNCTION_LIBRARIES[] = "REDIS_FUNCTION_LIBRARIES"; @@ -43,9 +42,8 @@ inline constexpr const char REGISTRY_SCRIPT_RUN_CTX_NAME[] = "SCRIPT_RUN_CTX"; namespace lua { -lua_State *CreateState(Server *srv); +lua_State *CreateState(); void DestroyState(lua_State *lua); -Server *GetServer(lua_State *lua); void LoadFuncs(lua_State *lua); void LoadLibraries(lua_State *lua); @@ -150,6 +148,8 @@ struct ScriptRunCtx { // and is used to detect whether there is cross-slot access // between multiple commands in a script or function. int current_slot = -1; + // the current connection + redis::Connection *conn = nullptr; }; /// SaveOnRegistry saves user-defined data to lua REGISTRY diff --git a/src/storage/storage.cc b/src/storage/storage.cc index 2eead08ace6..234309f5f1a 100644 --- a/src/storage/storage.cc +++ b/src/storage/storage.cc @@ -868,9 +868,11 @@ Status Storage::BeginTxn() { // The EXEC command is exclusive and shouldn't have multi transaction at the same time, // so it's fine to reset the global write batch without any lock. is_txn_mode_ = true; - txn_write_batch_ = - std::make_unique(rocksdb::BytewiseComparator() /*default backup_index_comparator */, - 0 /* default reserved_bytes*/, GetWriteBatchMaxBytes()); + // Set overwrite_key to false to avoid overwriting the existing key in case + // like downstream would parse the replication log etc. + txn_write_batch_ = std::make_unique( + /*backup_index_comparator=*/rocksdb::BytewiseComparator(), + /*reserved_bytes=*/0, /*overwrite_key=*/false, /*max_bytes=*/GetWriteBatchMaxBytes()); return Status::OK(); } diff --git a/src/types/redis_bitmap.cc b/src/types/redis_bitmap.cc index ae9fefcd315..e75deea3644 100644 --- a/src/types/redis_bitmap.cc +++ b/src/types/redis_bitmap.cc @@ -184,7 +184,6 @@ rocksdb::Status Bitmap::SetBit(engine::Context &ctx, const Slice &user_key, uint std::string raw_value; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); BitmapMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata, &raw_value); if (!s.ok() && !s.IsNotFound()) return s; @@ -461,7 +460,6 @@ rocksdb::Status Bitmap::BitOp(engine::Context &ctx, BitOpFlags op_flag, const st const Slice &user_key, const std::vector &op_keys, int64_t *len) { std::string raw_value; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); std::vector> meta_pairs; uint64_t max_bitmap_size = 0; @@ -824,15 +822,9 @@ template rocksdb::Status Bitmap::bitfield(engine::Context &ctx, const Slice &user_key, const std::vector &ops, std::vector> *rets) { std::string ns_key = AppendNamespacePrefix(user_key); - - std::optional guard; - if constexpr (!ReadOnly) { - guard = LockGuard(storage_->GetLockManager(), ns_key); - } - BitmapMetadata metadata; std::string raw_value; - // TODO(mwish): maintain snapshot for read-only bitfield. + auto s = GetMetadata(ctx, ns_key, &metadata, &raw_value); if (!s.ok() && !s.IsNotFound()) { return s; diff --git a/src/types/redis_bloom_chain.cc b/src/types/redis_bloom_chain.cc index 30c07fe338c..7dea8189b16 100644 --- a/src/types/redis_bloom_chain.cc +++ b/src/types/redis_bloom_chain.cc @@ -126,7 +126,6 @@ rocksdb::Status BloomChain::Reserve(engine::Context &ctx, const Slice &user_key, uint16_t expansion) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); BloomChainMetadata bloom_chain_metadata; rocksdb::Status s = getBloomChainMetadata(ctx, ns_key, &bloom_chain_metadata); if (!s.ok() && !s.IsNotFound()) return s; @@ -156,7 +155,6 @@ rocksdb::Status BloomChain::InsertCommon(engine::Context &ctx, const Slice &user const BloomFilterInsertOptions &insert_options, std::vector *rets) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); BloomChainMetadata metadata; rocksdb::Status s = getBloomChainMetadata(ctx, ns_key, &metadata); diff --git a/src/types/redis_hash.cc b/src/types/redis_hash.cc index 8930c9fce64..905efdadd37 100644 --- a/src/types/redis_hash.cc +++ b/src/types/redis_hash.cc @@ -66,7 +66,6 @@ rocksdb::Status Hash::IncrBy(engine::Context &ctx, const Slice &user_key, const std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); HashMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) return s; @@ -117,7 +116,6 @@ rocksdb::Status Hash::IncrByFloat(engine::Context &ctx, const Slice &user_key, c std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); HashMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) return s; @@ -211,7 +209,7 @@ rocksdb::Status Hash::Delete(engine::Context &ctx, const Slice &user_key, const WriteBatchLogData log_data(kRedisHash); auto s = batch->PutLogData(log_data.Encode()); if (!s.ok()) return s; - LockGuard guard(storage_->GetLockManager(), ns_key); + s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s; @@ -245,7 +243,6 @@ rocksdb::Status Hash::MSet(engine::Context &ctx, const Slice &user_key, const st *added_cnt = 0; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); HashMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) return s; diff --git a/src/types/redis_hyperloglog.cc b/src/types/redis_hyperloglog.cc index 562a7d6b1f6..b7b2197f208 100644 --- a/src/types/redis_hyperloglog.cc +++ b/src/types/redis_hyperloglog.cc @@ -112,7 +112,6 @@ rocksdb::Status HyperLogLog::Add(engine::Context &ctx, const Slice &user_key, *ret = 0; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); HyperLogLogMetadata metadata{}; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) { @@ -238,7 +237,6 @@ rocksdb::Status HyperLogLog::Merge(engine::Context &ctx, const Slice &dest_user_ } std::string dest_key = AppendNamespacePrefix(dest_user_key); - LockGuard guard(storage_->GetLockManager(), dest_key); std::vector registers; HyperLogLogMetadata metadata; diff --git a/src/types/redis_json.cc b/src/types/redis_json.cc index 5120573e7ce..ba331ef96ff 100644 --- a/src/types/redis_json.cc +++ b/src/types/redis_json.cc @@ -124,8 +124,6 @@ rocksdb::Status Json::Set(engine::Context &ctx, const std::string &user_key, con const std::string &value) { auto ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); - JsonMetadata metadata; JsonValue origin; auto s = read(ctx, ns_key, &metadata, &origin); @@ -190,8 +188,6 @@ rocksdb::Status Json::ArrAppend(engine::Context &ctx, const std::string &user_ke append_values.emplace_back(std::move(value.value)); } - LockGuard guard(storage_->GetLockManager(), ns_key); - JsonMetadata metadata; JsonValue value; auto s = read(ctx, ns_key, &metadata, &value); @@ -248,8 +244,6 @@ rocksdb::Status Json::Merge(engine::Context &ctx, const std::string &user_key, c const std::string &merge_value, bool &result) { auto ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); - JsonMetadata metadata; JsonValue json_val; @@ -279,8 +273,6 @@ rocksdb::Status Json::Clear(engine::Context &ctx, const std::string &user_key, c size_t *result) { auto ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); - JsonValue json_val; JsonMetadata metadata; auto s = read(ctx, ns_key, &metadata, &json_val); @@ -327,8 +319,6 @@ rocksdb::Status Json::ArrInsert(engine::Context &ctx, const std::string &user_ke insert_values.emplace_back(std::move(value.value)); } - LockGuard guard(storage_->GetLockManager(), ns_key); - JsonMetadata metadata; JsonValue value; auto s = read(ctx, ns_key, &metadata, &value); @@ -349,8 +339,6 @@ rocksdb::Status Json::Toggle(engine::Context &ctx, const std::string &user_key, Optionals *results) { auto ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); - JsonMetadata metadata; JsonValue origin; auto s = read(ctx, ns_key, &metadata, &origin); @@ -367,8 +355,6 @@ rocksdb::Status Json::ArrPop(engine::Context &ctx, const std::string &user_key, std::vector> *results) { auto ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); - JsonMetadata metadata; JsonValue json_val; auto s = read(ctx, ns_key, &metadata, &json_val); @@ -403,8 +389,6 @@ rocksdb::Status Json::ArrTrim(engine::Context &ctx, const std::string &user_key, int64_t stop, Optionals *results) { auto ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); - JsonMetadata metadata; JsonValue json_val; auto s = read(ctx, ns_key, &metadata, &json_val); @@ -424,7 +408,7 @@ rocksdb::Status Json::Del(engine::Context &ctx, const std::string &user_key, con *result = 0; auto ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); + JsonValue json_val; JsonMetadata metadata; auto s = read(ctx, ns_key, &metadata, &json_val); @@ -473,8 +457,6 @@ rocksdb::Status Json::numop(engine::Context &ctx, JsonValue::NumOpEnum op, const auto s = read(ctx, ns_key, &metadata, &json_val); if (!s.ok()) return s; - LockGuard guard(storage_->GetLockManager(), ns_key); - auto res = json_val.NumOp(path, number, op, result); if (!res) { return rocksdb::Status::InvalidArgument(res.Msg()); @@ -571,7 +553,6 @@ rocksdb::Status Json::MSet(engine::Context &ctx, const std::vector std::string ns_key = AppendNamespacePrefix(user_key); ns_keys.emplace_back(std::move(ns_key)); } - MultiLockGuard guard(storage_->GetLockManager(), ns_keys); auto batch = storage_->GetWriteBatchBase(); WriteBatchLogData log_data(kRedisJson); diff --git a/src/types/redis_json.h b/src/types/redis_json.h index 1a69408ee87..54831472dc1 100644 --- a/src/types/redis_json.h +++ b/src/types/redis_json.h @@ -86,7 +86,7 @@ class Json : public Database { private: rocksdb::Status write(engine::Context &ctx, Slice ns_key, JsonMetadata *metadata, const JsonValue &json_val); rocksdb::Status read(engine::Context &ctx, const Slice &ns_key, JsonMetadata *metadata, JsonValue *value); - static rocksdb::Status parse(const JsonMetadata &metadata, const Slice &json_byt, JsonValue *value); + static rocksdb::Status parse(const JsonMetadata &metadata, const Slice &json_byte, JsonValue *value); rocksdb::Status create(engine::Context &ctx, const std::string &ns_key, JsonMetadata &metadata, const std::string &value); rocksdb::Status del(engine::Context &ctx, const Slice &ns_key); diff --git a/src/types/redis_list.cc b/src/types/redis_list.cc index e640ffb6005..acef1ed9cc6 100644 --- a/src/types/redis_list.cc +++ b/src/types/redis_list.cc @@ -63,7 +63,7 @@ rocksdb::Status List::push(engine::Context &ctx, const Slice &user_key, const st WriteBatchLogData log_data(kRedisList, {std::to_string(cmd)}); auto s = batch->PutLogData(log_data.Encode()); if (!s.ok()) return s; - LockGuard guard(storage_->GetLockManager(), ns_key); + s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !(create_if_missing && s.IsNotFound())) { return s.IsNotFound() ? rocksdb::Status::OK() : s; @@ -108,7 +108,6 @@ rocksdb::Status List::PopMulti(engine::Context &ctx, const rocksdb::Slice &user_ std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); ListMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s; @@ -177,7 +176,6 @@ rocksdb::Status List::Rem(engine::Context &ctx, const Slice &user_key, int count std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); ListMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s; @@ -271,7 +269,6 @@ rocksdb::Status List::Insert(engine::Context &ctx, const Slice &user_key, const *new_size = 0; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); ListMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s; @@ -462,7 +459,6 @@ rocksdb::Status List::Pos(engine::Context &ctx, const Slice &user_key, const Sli rocksdb::Status List::Set(engine::Context &ctx, const Slice &user_key, int index, Slice elem) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); ListMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s; @@ -501,7 +497,6 @@ rocksdb::Status List::lmoveOnSingleList(engine::Context &ctx, const rocksdb::Sli std::string *elem) { std::string ns_key = AppendNamespacePrefix(src); - LockGuard guard(storage_->GetLockManager(), ns_key); ListMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) { @@ -567,8 +562,6 @@ rocksdb::Status List::lmoveOnTwoLists(engine::Context &ctx, const rocksdb::Slice std::string src_ns_key = AppendNamespacePrefix(src); std::string dst_ns_key = AppendNamespacePrefix(dst); - std::vector lock_keys{src_ns_key, dst_ns_key}; - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); ListMetadata src_metadata(false); auto s = GetMetadata(ctx, src_ns_key, &src_metadata); if (!s.ok()) { @@ -636,7 +629,6 @@ rocksdb::Status List::Trim(engine::Context &ctx, const Slice &user_key, int star uint32_t trim_cnt = 0; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); ListMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s; diff --git a/src/types/redis_set.cc b/src/types/redis_set.cc index 043e0d36cea..01849328c60 100644 --- a/src/types/redis_set.cc +++ b/src/types/redis_set.cc @@ -37,7 +37,6 @@ rocksdb::Status Set::GetMetadata(engine::Context &ctx, const Slice &ns_key, SetM rocksdb::Status Set::Overwrite(engine::Context &ctx, Slice user_key, const std::vector &members) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); SetMetadata metadata; auto batch = storage_->GetWriteBatchBase(); WriteBatchLogData log_data(kRedisSet); @@ -62,7 +61,6 @@ rocksdb::Status Set::Add(engine::Context &ctx, const Slice &user_key, const std: std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); SetMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) return s; @@ -100,7 +98,6 @@ rocksdb::Status Set::Remove(engine::Context &ctx, const Slice &user_key, const s std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); SetMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s; @@ -218,9 +215,6 @@ rocksdb::Status Set::Take(engine::Context &ctx, const Slice &user_key, std::vect std::string ns_key = AppendNamespacePrefix(user_key); - std::optional lock_guard; - if (pop) lock_guard.emplace(storage_->GetLockManager(), ns_key); - SetMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s; @@ -290,14 +284,6 @@ rocksdb::Status Set::Scan(engine::Context &ctx, const Slice &user_key, const std * DIFF key1 key2 key3 = {b,d} */ rocksdb::Status Set::Diff(engine::Context &ctx, const std::vector &keys, std::vector *members) { - std::vector lock_keys; - lock_keys.reserve(keys.size()); - for (const auto key : keys) { - std::string ns_key = AppendNamespacePrefix(key); - lock_keys.emplace_back(std::move(ns_key)); - } - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); - members->clear(); std::vector source_members; auto s = Members(ctx, keys[0], &source_members); @@ -329,14 +315,6 @@ rocksdb::Status Set::Diff(engine::Context &ctx, const std::vector &keys, * UNION key1 key2 key3 = {a,b,c,d,e} */ rocksdb::Status Set::Union(engine::Context &ctx, const std::vector &keys, std::vector *members) { - std::vector lock_keys; - lock_keys.reserve(keys.size()); - for (const auto key : keys) { - std::string ns_key = AppendNamespacePrefix(key); - lock_keys.emplace_back(std::move(ns_key)); - } - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); - members->clear(); std::map union_members; @@ -363,14 +341,6 @@ rocksdb::Status Set::Union(engine::Context &ctx, const std::vector &keys, * INTER key1 key2 key3 = {c} */ rocksdb::Status Set::Inter(engine::Context &ctx, const std::vector &keys, std::vector *members) { - std::vector lock_keys; - lock_keys.reserve(keys.size()); - for (const auto key : keys) { - std::string ns_key = AppendNamespacePrefix(key); - lock_keys.emplace_back(std::move(ns_key)); - } - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); - members->clear(); std::map member_counters; diff --git a/src/types/redis_sortedint.cc b/src/types/redis_sortedint.cc index 98e1f7a5133..f29ca7de616 100644 --- a/src/types/redis_sortedint.cc +++ b/src/types/redis_sortedint.cc @@ -38,7 +38,6 @@ rocksdb::Status Sortedint::Add(engine::Context &ctx, const Slice &user_key, cons std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); SortedintMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) return s; @@ -75,7 +74,6 @@ rocksdb::Status Sortedint::Remove(engine::Context &ctx, const Slice &user_key, c std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); SortedintMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s; diff --git a/src/types/redis_stream.cc b/src/types/redis_stream.cc index 009a98c0a8d..df9284c2db5 100644 --- a/src/types/redis_stream.cc +++ b/src/types/redis_stream.cc @@ -96,7 +96,6 @@ rocksdb::Status Stream::Add(engine::Context &ctx, const Slice &stream_name, cons std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); StreamMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) return s; @@ -333,7 +332,6 @@ rocksdb::Status Stream::DeletePelEntries(engine::Context &ctx, const Slice &stre std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); StreamMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) { @@ -400,7 +398,7 @@ rocksdb::Status Stream::ClaimPelEntries(engine::Context &ctx, const Slice &strea const std::vector &entry_ids, const StreamClaimOptions &options, StreamClaimResult *result) { std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); + StreamMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s; @@ -536,7 +534,6 @@ rocksdb::Status Stream::AutoClaim(engine::Context &ctx, const Slice &stream_name std::string ns_key = AppendNamespacePrefix(stream_name); StreamMetadata metadata(false); - LockGuard guard(storage_->GetLockManager(), ns_key); auto s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) { // not found will be caught by outside with no such key or consumer group return s; @@ -692,7 +689,6 @@ rocksdb::Status Stream::CreateGroup(engine::Context &ctx, const Slice &stream_na } std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); StreamMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) { @@ -745,7 +741,6 @@ rocksdb::Status Stream::DestroyGroup(engine::Context &ctx, const Slice &stream_n *delete_cnt = 0; std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); StreamMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) { @@ -849,14 +844,14 @@ rocksdb::Status Stream::createConsumerWithoutLock(engine::Context &ctx, const Sl rocksdb::Status Stream::CreateConsumer(engine::Context &ctx, const Slice &stream_name, const std::string &group_name, const std::string &consumer_name, int *created_number) { std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); + return createConsumerWithoutLock(ctx, stream_name, group_name, consumer_name, created_number); } rocksdb::Status Stream::DestroyConsumer(engine::Context &ctx, const Slice &stream_name, const std::string &group_name, const std::string &consumer_name, uint64_t &deleted_pel) { std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); + StreamMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) { @@ -923,7 +918,7 @@ rocksdb::Status Stream::DestroyConsumer(engine::Context &ctx, const Slice &strea rocksdb::Status Stream::GroupSetId(engine::Context &ctx, const Slice &stream_name, const std::string &group_name, const StreamXGroupCreateOptions &options) { std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); + StreamMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) { @@ -965,7 +960,6 @@ rocksdb::Status Stream::DeleteEntries(engine::Context &ctx, const Slice &stream_ std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); StreamMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) { @@ -1211,7 +1205,6 @@ rocksdb::Status Stream::GetStreamInfo(engine::Context &ctx, const rocksdb::Slice uint64_t count, StreamInfo *info) { std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); StreamMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s; @@ -1452,7 +1445,6 @@ rocksdb::Status Stream::RangeWithPending(engine::Context &ctx, const Slice &stre } std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); StreamMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); @@ -1582,8 +1574,6 @@ rocksdb::Status Stream::Trim(engine::Context &ctx, const Slice &stream_name, con std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); - StreamMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) { @@ -1696,8 +1686,6 @@ rocksdb::Status Stream::SetId(engine::Context &ctx, const Slice &stream_name, co std::string ns_key = AppendNamespacePrefix(stream_name); - LockGuard guard(storage_->GetLockManager(), ns_key); - StreamMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) { diff --git a/src/types/redis_string.cc b/src/types/redis_string.cc index 7d78534256a..210fb697b91 100644 --- a/src/types/redis_string.cc +++ b/src/types/redis_string.cc @@ -118,7 +118,6 @@ rocksdb::Status String::Append(engine::Context &ctx, const std::string &user_key *new_size = 0; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); std::string raw_value; rocksdb::Status s = getRawValue(ctx, ns_key, &raw_value); if (!s.ok() && !s.IsNotFound()) return s; @@ -156,7 +155,6 @@ rocksdb::Status String::GetEx(engine::Context &ctx, const std::string &user_key, std::optional expire) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); rocksdb::Status s = getValue(ctx, ns_key, value); if (!s.ok()) return s; @@ -190,7 +188,6 @@ rocksdb::Status String::GetSet(engine::Context &ctx, const std::string &user_key rocksdb::Status String::GetDel(engine::Context &ctx, const std::string &user_key, std::string *value) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); rocksdb::Status s = getValue(ctx, ns_key, value); if (!s.ok()) return s; @@ -199,7 +196,7 @@ rocksdb::Status String::GetDel(engine::Context &ctx, const std::string &user_key rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, const std::string &value) { std::vector pairs{StringPair{user_key, value}}; - return MSet(ctx, pairs, /*expire=*/0, /*lock=*/true); + return MSet(ctx, pairs, /*expire=*/0); } rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, const std::string &value, @@ -207,7 +204,6 @@ rocksdb::Status String::Set(engine::Context &ctx, const std::string &user_key, c uint64_t expire = 0; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); bool need_old_value = args.type != StringSetType::NONE || args.get || args.keep_ttl; if (need_old_value) { std::string old_value; @@ -289,7 +285,6 @@ rocksdb::Status String::SetRange(engine::Context &ctx, const std::string &user_k const std::string &value, uint64_t *new_size) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); std::string raw_value; rocksdb::Status s = getRawValue(ctx, ns_key, &raw_value); if (!s.ok() && !s.IsNotFound()) return s; @@ -329,7 +324,6 @@ rocksdb::Status String::IncrBy(engine::Context &ctx, const std::string &user_key int64_t *new_value) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); std::string raw_value; rocksdb::Status s = getRawValue(ctx, ns_key, &raw_value); if (!s.ok() && !s.IsNotFound()) return s; @@ -366,7 +360,7 @@ rocksdb::Status String::IncrBy(engine::Context &ctx, const std::string &user_key rocksdb::Status String::IncrByFloat(engine::Context &ctx, const std::string &user_key, double increment, double *new_value) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); + std::string raw_value; rocksdb::Status s = getRawValue(ctx, ns_key, &raw_value); if (!s.ok() && !s.IsNotFound()) return s; @@ -397,21 +391,7 @@ rocksdb::Status String::IncrByFloat(engine::Context &ctx, const std::string &use return updateRawValue(ctx, ns_key, raw_value); } -rocksdb::Status String::MSet(engine::Context &ctx, const std::vector &pairs, uint64_t expire_ms, - bool lock) { - // Data race, key string maybe overwrite by other key while didn't lock the keys here, - // to improve the set performance - std::optional guard; - if (lock) { - std::vector lock_keys; - lock_keys.reserve(pairs.size()); - for (const StringPair &pair : pairs) { - std::string ns_key = AppendNamespacePrefix(pair.key); - lock_keys.emplace_back(std::move(ns_key)); - } - guard.emplace(storage_->GetLockManager(), lock_keys); - } - +rocksdb::Status String::MSet(engine::Context &ctx, const std::vector &pairs, uint64_t expire_ms) { auto batch = storage_->GetWriteBatchBase(); WriteBatchLogData log_data(kRedisString); auto s = batch->PutLogData(log_data.Encode()); @@ -434,25 +414,19 @@ rocksdb::Status String::MSetNX(engine::Context &ctx, const std::vector lock_keys; - lock_keys.reserve(pairs.size()); std::vector keys; keys.reserve(pairs.size()); for (StringPair pair : pairs) { std::string ns_key = AppendNamespacePrefix(pair.key); - lock_keys.emplace_back(std::move(ns_key)); keys.emplace_back(pair.key); } - // Lock these keys before doing anything. - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); - if (Exists(ctx, keys, &exists).ok() && exists > 0) { return rocksdb::Status::OK(); } - rocksdb::Status s = MSet(ctx, pairs, /*expire_ms=*/expire_ms, /*lock=*/false); + rocksdb::Status s = MSet(ctx, pairs, /*expire_ms=*/expire_ms); if (!s.ok()) return s; *flag = true; @@ -471,7 +445,6 @@ rocksdb::Status String::CAS(engine::Context &ctx, const std::string &user_key, c std::string current_value; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); rocksdb::Status s = getValue(ctx, ns_key, ¤t_value); if (!s.ok() && !s.IsNotFound()) { @@ -507,7 +480,6 @@ rocksdb::Status String::CAD(engine::Context &ctx, const std::string &user_key, c std::string current_value; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); rocksdb::Status s = getValue(ctx, ns_key, ¤t_value); if (!s.ok() && !s.IsNotFound()) { diff --git a/src/types/redis_string.h b/src/types/redis_string.h index b218bae346a..e5025d64fc6 100644 --- a/src/types/redis_string.h +++ b/src/types/redis_string.h @@ -100,8 +100,7 @@ class String : public Database { rocksdb::Status IncrByFloat(engine::Context &ctx, const std::string &user_key, double increment, double *new_value); std::vector MGet(engine::Context &ctx, const std::vector &keys, std::vector *values); - rocksdb::Status MSet(engine::Context &ctx, const std::vector &pairs, uint64_t expire_ms, - bool lock = true); + rocksdb::Status MSet(engine::Context &ctx, const std::vector &pairs, uint64_t expire_ms); rocksdb::Status MSetNX(engine::Context &ctx, const std::vector &pairs, uint64_t expire_ms, bool *flag); rocksdb::Status CAS(engine::Context &ctx, const std::string &user_key, const std::string &old_value, const std::string &new_value, uint64_t expire_ms, int *flag); diff --git a/src/types/redis_zset.cc b/src/types/redis_zset.cc index 4474b348e5b..5fa586d6160 100644 --- a/src/types/redis_zset.cc +++ b/src/types/redis_zset.cc @@ -42,7 +42,6 @@ rocksdb::Status ZSet::Add(engine::Context &ctx, const Slice &user_key, ZAddFlags std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); ZSetMetadata metadata; rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok() && !s.IsNotFound()) return s; @@ -159,7 +158,6 @@ rocksdb::Status ZSet::Pop(engine::Context &ctx, const Slice &user_key, int count std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); ZSetMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s; @@ -224,8 +222,6 @@ rocksdb::Status ZSet::RangeByRank(engine::Context &ctx, const Slice &user_key, c std::string ns_key = AppendNamespacePrefix(user_key); - std::optional lock_guard; - if (spec.with_deletion) lock_guard.emplace(storage_->GetLockManager(), ns_key); ZSetMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); @@ -306,8 +302,6 @@ rocksdb::Status ZSet::RangeByScore(engine::Context &ctx, const Slice &user_key, std::string ns_key = AppendNamespacePrefix(user_key); - std::optional lock_guard; - if (spec.with_deletion) lock_guard.emplace(storage_->GetLockManager(), ns_key); ZSetMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s; @@ -429,10 +423,6 @@ rocksdb::Status ZSet::RangeByLex(engine::Context &ctx, const Slice &user_key, co std::string ns_key = AppendNamespacePrefix(user_key); - std::optional lock_guard; - if (spec.with_deletion) { - lock_guard.emplace(storage_->GetLockManager(), ns_key); - } ZSetMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s; @@ -526,7 +516,6 @@ rocksdb::Status ZSet::Remove(engine::Context &ctx, const Slice &user_key, const *removed_cnt = 0; std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); ZSetMetadata metadata(false); rocksdb::Status s = GetMetadata(ctx, ns_key, &metadata); if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s; @@ -621,7 +610,6 @@ rocksdb::Status ZSet::Rank(engine::Context &ctx, const Slice &user_key, const Sl rocksdb::Status ZSet::Overwrite(engine::Context &ctx, const Slice &user_key, const MemberScores &mscores) { std::string ns_key = AppendNamespacePrefix(user_key); - LockGuard guard(storage_->GetLockManager(), ns_key); ZSetMetadata metadata; auto batch = storage_->GetWriteBatchBase(); WriteBatchLogData log_data(kRedisZSet); @@ -658,14 +646,6 @@ rocksdb::Status ZSet::InterStore(engine::Context &ctx, const Slice &dst, const s rocksdb::Status ZSet::Inter(engine::Context &ctx, const std::vector &keys_weights, AggregateMethod aggregate_method, std::vector *members) { - std::vector lock_keys; - lock_keys.reserve(keys_weights.size()); - for (const auto &key_weight : keys_weights) { - std::string ns_key = AppendNamespacePrefix(key_weight.key); - lock_keys.emplace_back(std::move(ns_key)); - } - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); - std::map dst_zset; std::map member_counters; std::vector target_mscores; @@ -723,14 +703,6 @@ rocksdb::Status ZSet::Inter(engine::Context &ctx, const std::vector & rocksdb::Status ZSet::InterCard(engine::Context &ctx, const std::vector &user_keys, uint64_t limit, uint64_t *inter_cnt) { - std::vector lock_keys; - lock_keys.reserve(user_keys.size()); - for (const auto &user_key : user_keys) { - std::string ns_key = AppendNamespacePrefix(user_key); - lock_keys.emplace_back(std::move(ns_key)); - } - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); - std::vector mscores_list; mscores_list.reserve(user_keys.size()); RangeScoreSpec spec; @@ -780,14 +752,6 @@ rocksdb::Status ZSet::UnionStore(engine::Context &ctx, const Slice &dst, const s rocksdb::Status ZSet::Union(engine::Context &ctx, const std::vector &keys_weights, AggregateMethod aggregate_method, std::vector *members) { - std::vector lock_keys; - lock_keys.reserve(keys_weights.size()); - for (const auto &key_weight : keys_weights) { - std::string ns_key = AppendNamespacePrefix(key_weight.key); - lock_keys.emplace_back(std::move(ns_key)); - } - MultiLockGuard guard(storage_->GetLockManager(), lock_keys); - std::map dst_zset; std::vector target_mscores; uint64_t target_size = 0; diff --git a/tests/cppunit/status_test.cc b/tests/cppunit/status_test.cc index 5c800c9e261..192c52803cc 100644 --- a/tests/cppunit/status_test.cc +++ b/tests/cppunit/status_test.cc @@ -226,3 +226,31 @@ TEST(StatusOr, Prefixed) { ASSERT_EQ(g(-2).Msg(), "oh: hi"); ASSERT_EQ(*g(5), 36); } + +TEST(GetOrRet, RocksdbStatus) { + auto f = [](int x) -> Status { + if (x < 10) return {Status::NotOK}; + return Status::OK(); + }; + + auto g = [&f](int x) -> Status { + GET_OR_RET(f(x)); + return Status::OK(); + }; + + ASSERT_TRUE(g(10)); + ASSERT_FALSE(g(1)); + + auto f2 = [](int x) -> rocksdb::Status { + if (x < 10) return rocksdb::Status::InvalidArgument(""); + return rocksdb::Status::OK(); + }; + + auto g2 = [&f2](int x) -> rocksdb::Status { + GET_OR_RET(f2(x)); + return rocksdb::Status::OK(); + }; + + ASSERT_TRUE(g2(10).ok()); + ASSERT_FALSE(g2(1).ok()); +} diff --git a/tests/cppunit/string_util_test.cc b/tests/cppunit/string_util_test.cc index f95ccbff3dd..1d24cf594e4 100644 --- a/tests/cppunit/string_util_test.cc +++ b/tests/cppunit/string_util_test.cc @@ -22,6 +22,7 @@ #include +#include #include #include #include @@ -84,6 +85,161 @@ TEST(StringUtil, HasPrefix) { ASSERT_FALSE(util::HasPrefix("has", "has_prefix")); } +TEST(StringUtil, ValidateGlob) { + const auto expect_ok = [](std::string_view glob) { + const auto result = util::ValidateGlob(glob); + EXPECT_TRUE(result.IsOK()) << glob << ": " << result.Msg(); + }; + + const auto expect_error = [](std::string_view glob, std::string_view expected_error) { + const auto result = util::ValidateGlob(glob); + EXPECT_FALSE(result.IsOK()); + EXPECT_EQ(result.Msg(), expected_error) << glob; + }; + + expect_ok("a"); + expect_ok("\\*"); + expect_ok("\\?"); + expect_ok("\\["); + expect_ok("\\]"); + expect_ok("a*"); + expect_ok("a?"); + expect_ok("[ab]"); + expect_ok("[^ab]"); + expect_ok("[a-c]"); + // Surprisingly valid: this accepts the characters {a, b, c, e, f, g, -} + expect_ok("[a-c-e-g]"); + expect_ok("[^a-c]"); + expect_ok("[-]"); + expect_ok("[\\]]"); + expect_ok("[\\\\]"); + expect_ok("[\\?]"); + expect_ok("[\\*]"); + expect_ok("[\\[]"); + + expect_error("[", "Unterminated [ group"); + expect_error("]", "Unmatched unescaped ]"); + expect_error("[a", "Unterminated [ group"); + expect_error("\\", "Trailing unescaped backslash"); + + // Weird case: we open a character class, with the range 'a' to ']', but then never close it + expect_error("[a-]", "Unterminated [ group"); + expect_ok("[a-]]"); +} + +TEST(StringUtil, StringMatch) { + /* Some basic tests */ + EXPECT_TRUE(util::StringMatch("a", "a")); + EXPECT_FALSE(util::StringMatch("a", "b")); + EXPECT_FALSE(util::StringMatch("a", "aa")); + EXPECT_FALSE(util::StringMatch("a", "")); + EXPECT_TRUE(util::StringMatch("", "")); + EXPECT_FALSE(util::StringMatch("", "a")); + EXPECT_TRUE(util::StringMatch("*", "")); + EXPECT_TRUE(util::StringMatch("*", "a")); + + /* Simple character class tests */ + EXPECT_TRUE(util::StringMatch("[a]", "a")); + EXPECT_FALSE(util::StringMatch("[a]", "b")); + EXPECT_FALSE(util::StringMatch("[^a]", "a")); + EXPECT_TRUE(util::StringMatch("[^a]", "b")); + EXPECT_TRUE(util::StringMatch("[ab]", "a")); + EXPECT_TRUE(util::StringMatch("[ab]", "b")); + EXPECT_FALSE(util::StringMatch("[ab]", "c")); + EXPECT_TRUE(util::StringMatch("[^ab]", "c")); + EXPECT_TRUE(util::StringMatch("[a-c]", "b")); + EXPECT_FALSE(util::StringMatch("[a-c]", "d")); + + /* Corner cases in character class parsing */ + EXPECT_TRUE(util::StringMatch("[a-c-e-g]", "-")); + EXPECT_FALSE(util::StringMatch("[a-c-e-g]", "d")); + EXPECT_TRUE(util::StringMatch("[a-c-e-g]", "f")); + + /* Escaping */ + EXPECT_TRUE(util::StringMatch("\\?", "?")); + EXPECT_FALSE(util::StringMatch("\\?", "a")); + EXPECT_TRUE(util::StringMatch("\\*", "*")); + EXPECT_FALSE(util::StringMatch("\\*", "a")); + EXPECT_TRUE(util::StringMatch("\\[", "[")); + EXPECT_TRUE(util::StringMatch("\\]", "]")); + EXPECT_TRUE(util::StringMatch("\\\\", "\\")); + EXPECT_TRUE(util::StringMatch("[\\.]", ".")); + EXPECT_TRUE(util::StringMatch("[\\-]", "-")); + EXPECT_TRUE(util::StringMatch("[\\[]", "[")); + EXPECT_TRUE(util::StringMatch("[\\]]", "]")); + EXPECT_TRUE(util::StringMatch("[\\\\]", "\\")); + EXPECT_TRUE(util::StringMatch("[\\?]", "?")); + EXPECT_TRUE(util::StringMatch("[\\*]", "*")); + + /* Simple wild cards */ + EXPECT_TRUE(util::StringMatch("?", "a")); + EXPECT_FALSE(util::StringMatch("?", "aa")); + EXPECT_FALSE(util::StringMatch("??", "a")); + EXPECT_TRUE(util::StringMatch("?x?", "axb")); + EXPECT_FALSE(util::StringMatch("?x?", "abx")); + EXPECT_FALSE(util::StringMatch("?x?", "xab")); + + /* Asterisk wild cards (backtracking) */ + EXPECT_FALSE(util::StringMatch("*??", "a")); + EXPECT_TRUE(util::StringMatch("*??", "ab")); + EXPECT_TRUE(util::StringMatch("*??", "abc")); + EXPECT_TRUE(util::StringMatch("*??", "abcd")); + EXPECT_FALSE(util::StringMatch("??*", "a")); + EXPECT_TRUE(util::StringMatch("??*", "ab")); + EXPECT_TRUE(util::StringMatch("??*", "abc")); + EXPECT_TRUE(util::StringMatch("??*", "abcd")); + EXPECT_FALSE(util::StringMatch("?*?", "a")); + EXPECT_TRUE(util::StringMatch("?*?", "ab")); + EXPECT_TRUE(util::StringMatch("?*?", "abc")); + EXPECT_TRUE(util::StringMatch("?*?", "abcd")); + EXPECT_TRUE(util::StringMatch("*b", "b")); + EXPECT_TRUE(util::StringMatch("*b", "ab")); + EXPECT_FALSE(util::StringMatch("*b", "ba")); + EXPECT_TRUE(util::StringMatch("*b", "bb")); + EXPECT_TRUE(util::StringMatch("*b", "abb")); + EXPECT_TRUE(util::StringMatch("*b", "bab")); + EXPECT_TRUE(util::StringMatch("*bc", "abbc")); + EXPECT_TRUE(util::StringMatch("*bc", "bc")); + EXPECT_TRUE(util::StringMatch("*bc", "bbc")); + EXPECT_TRUE(util::StringMatch("*bc", "bcbc")); + + /* Multiple asterisks (complex backtracking) */ + EXPECT_TRUE(util::StringMatch("*ac*", "abacadaeafag")); + EXPECT_TRUE(util::StringMatch("*ac*ae*ag*", "abacadaeafag")); + EXPECT_TRUE(util::StringMatch("*a*b*[bc]*[ef]*g*", "abacadaeafag")); + EXPECT_FALSE(util::StringMatch("*a*b*[ef]*[cd]*g*", "abacadaeafag")); + EXPECT_TRUE(util::StringMatch("*abcd*", "abcabcabcabcdefg")); + EXPECT_TRUE(util::StringMatch("*ab*cd*", "abcabcabcabcdefg")); + EXPECT_TRUE(util::StringMatch("*abcd*abcdef*", "abcabcdabcdeabcdefg")); + EXPECT_FALSE(util::StringMatch("*abcd*", "abcabcabcabcefg")); + EXPECT_FALSE(util::StringMatch("*ab*cd*", "abcabcabcabcefg")); + + /* Robustness to exponential blow-ups with lots of non-collapsible asterisks */ + EXPECT_TRUE( + util::StringMatch("?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*a", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")); + EXPECT_FALSE( + util::StringMatch("?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*?*b", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")); +} + +TEST(StringUtil, SplitGlob) { + using namespace std::string_literals; + + // Basic functionality: no escaped characters + EXPECT_EQ(util::SplitGlob(""), std::make_pair(""s, ""s)); + EXPECT_EQ(util::SplitGlob("string"), std::make_pair("string"s, ""s)); + EXPECT_EQ(util::SplitGlob("string*"), std::make_pair("string"s, "*"s)); + EXPECT_EQ(util::SplitGlob("*string"), std::make_pair(""s, "*string"s)); + EXPECT_EQ(util::SplitGlob("str*ing"), std::make_pair("str"s, "*ing"s)); + EXPECT_EQ(util::SplitGlob("string?"), std::make_pair("string"s, "?"s)); + EXPECT_EQ(util::SplitGlob("?string"), std::make_pair(""s, "?string"s)); + EXPECT_EQ(util::SplitGlob("ab[cd]ef"), std::make_pair("ab"s, "[cd]ef"s)); + + // Escaped characters; also tests that prefix is trimmed of backslashes + EXPECT_EQ(util::SplitGlob("str\\*ing*"), std::make_pair("str*ing"s, "*"s)); + EXPECT_EQ(util::SplitGlob("str\\?ing?"), std::make_pair("str?ing"s, "?"s)); + EXPECT_EQ(util::SplitGlob("str\\[ing[a]"), std::make_pair("str[ing"s, "[a]"s)); +} + TEST(StringUtil, EscapeString) { std::unordered_map origin_to_escaped = { {"abc", "abc"}, diff --git a/tests/gocase/integration/cluster/cluster_test.go b/tests/gocase/integration/cluster/cluster_test.go index 560479479b6..05ec27f028f 100644 --- a/tests/gocase/integration/cluster/cluster_test.go +++ b/tests/gocase/integration/cluster/cluster_test.go @@ -154,58 +154,79 @@ func TestClusterNodes(t *testing.T) { } func TestClusterReplicas(t *testing.T) { - srv := util.StartServer(t, map[string]string{"cluster-enabled": "yes"}) - defer srv.Close() - ctx := context.Background() - rdb := srv.NewClient() - defer func() { require.NoError(t, rdb.Close()) }() + srv1 := util.StartServer(t, map[string]string{"cluster-enabled": "yes"}) + rdb1 := srv1.NewClient() + srv2 := util.StartServer(t, map[string]string{"cluster-enabled": "yes"}) + rdb2 := srv2.NewClient() + + defer func() { + srv1.Close() + srv2.Close() + require.NoError(t, rdb1.Close()) + require.NoError(t, rdb2.Close()) + }() nodes := "" master1ID := "bb2e5b3c5282086df51eff6b3e35519aede96fa6" - master1Node := fmt.Sprintf("%s %s %d master - 0-8191", master1ID, srv.Host(), srv.Port()) + master1Node := fmt.Sprintf("%s %s %d master - 0-8191", master1ID, srv1.Host(), srv1.Port()) nodes += master1Node + "\n" master2ID := "159dde1194ebf5bfc5a293dff839c3d1476f2a49" - master2Node := fmt.Sprintf("%s %s %d master - 8192-16383", master2ID, srv.Host(), srv.Port()) + master2Node := fmt.Sprintf("%s %s %d master - 8192-16383", master2ID, srv1.Host(), srv1.Port()) nodes += master2Node + "\n" replica2ID := "7dbee3d628f04cc5d763b36e92b10533e627a1d0" - replica2Node := fmt.Sprintf("%s %s %d slave %s", replica2ID, srv.Host(), srv.Port(), master2ID) + replica2Node := fmt.Sprintf("%s %s %d slave %s", replica2ID, srv2.Host(), srv2.Port(), master2ID) nodes += replica2Node - require.NoError(t, rdb.Do(ctx, "clusterx", "SETNODES", nodes, "2").Err()) - require.EqualValues(t, "2", rdb.Do(ctx, "clusterx", "version").Val()) + require.NoError(t, rdb1.Do(ctx, "clusterx", "SETNODES", nodes, "2").Err()) + require.EqualValues(t, "2", rdb1.Do(ctx, "clusterx", "version").Val()) + require.NoError(t, rdb2.Do(ctx, "clusterx", "SETNODES", nodes, "2").Err()) t.Run("with replicas", func(t *testing.T) { - replicas, err := rdb.Do(ctx, "cluster", "replicas", "159dde1194ebf5bfc5a293dff839c3d1476f2a49").Text() + replicas, err := rdb1.Do(ctx, "cluster", "replicas", "159dde1194ebf5bfc5a293dff839c3d1476f2a49").Text() require.NoError(t, err) fields := strings.Split(replicas, " ") require.Len(t, fields, 8) - require.Equal(t, fmt.Sprintf("%s@%d", srv.HostPort(), srv.Port()+10000), fields[1]) + require.Equal(t, fmt.Sprintf("%s@%d", srv2.HostPort(), srv2.Port()+10000), fields[1]) require.Equal(t, "slave", fields[2]) require.Equal(t, master2ID, fields[3]) require.Equal(t, "connected\n", fields[7]) }) t.Run("without replicas", func(t *testing.T) { - replicas, err := rdb.Do(ctx, "cluster", "replicas", "bb2e5b3c5282086df51eff6b3e35519aede96fa6").Text() + replicas, err := rdb1.Do(ctx, "cluster", "replicas", "bb2e5b3c5282086df51eff6b3e35519aede96fa6").Text() require.NoError(t, err) require.Empty(t, replicas) }) t.Run("send command to replica", func(t *testing.T) { - err := rdb.Do(ctx, "cluster", "replicas", "7dbee3d628f04cc5d763b36e92b10533e627a1d0").Err() + err := rdb1.Do(ctx, "cluster", "replicas", "7dbee3d628f04cc5d763b36e92b10533e627a1d0").Err() require.Error(t, err) require.Contains(t, err.Error(), "The node isn't a master") }) t.Run("unknown node", func(t *testing.T) { - err := rdb.Do(ctx, "cluster", "replicas", "unknown").Err() + err := rdb1.Do(ctx, "cluster", "replicas", "unknown").Err() require.Error(t, err) require.Contains(t, err.Error(), "Invalid cluster node id") }) + + t.Run("remove the replication if the node is not in the cluster", func(t *testing.T) { + require.Equal(t, "slave", util.FindInfoEntry(rdb2, "role")) + // remove the cluster replica node + clusterNode := fmt.Sprintf("%s\n%s", master1Node, master2Node) + err := rdb1.Do(ctx, "clusterx", "SETNODES", clusterNode, "3").Err() + require.NoError(t, err) + err = rdb2.Do(ctx, "clusterx", "SETNODES", clusterNode, "3").Err() + require.NoError(t, err) + + require.Eventually(t, func() bool { + return util.FindInfoEntry(rdb2, "role") == "master" + }, 5*time.Second, 100*time.Millisecond) + }) } func TestClusterDumpAndLoadClusterNodesInfo(t *testing.T) { diff --git a/tests/gocase/unit/keyspace/keyspace_test.go b/tests/gocase/unit/keyspace/keyspace_test.go index 6fbb84a7b94..37b86afd1d8 100644 --- a/tests/gocase/unit/keyspace/keyspace_test.go +++ b/tests/gocase/unit/keyspace/keyspace_test.go @@ -27,6 +27,7 @@ import ( "github.com/apache/kvrocks/tests/gocase/util" "github.com/stretchr/testify/require" + "golang.org/x/exp/slices" ) func TestKeyspace(t *testing.T) { @@ -65,10 +66,6 @@ func TestKeyspace(t *testing.T) { require.Equal(t, []string{"foo_a", "foo_b", "foo_c"}, keys) }) - t.Run("KEYS with invalid pattern", func(t *testing.T) { - require.Error(t, rdb.Keys(ctx, "*ab*").Err()) - }) - t.Run("KEYS to get all keys", func(t *testing.T) { keys := rdb.Keys(ctx, "*").Val() sort.Slice(keys, func(i, j int) bool { @@ -77,12 +74,58 @@ func TestKeyspace(t *testing.T) { require.Equal(t, []string{"foo_a", "foo_b", "foo_c", "key_x", "key_y", "key_z"}, keys) }) + t.Run("KEYS with invalid patterns", func(t *testing.T) { + require.Error(t, rdb.Keys(ctx, "[").Err()) + require.Error(t, rdb.Keys(ctx, "\\").Err()) + require.Error(t, rdb.Keys(ctx, "[a-]").Err()) + require.Error(t, rdb.Keys(ctx, "[a").Err()) + }) + t.Run("DBSize", func(t *testing.T) { require.NoError(t, rdb.Do(ctx, "dbsize", "scan").Err()) time.Sleep(100 * time.Millisecond) require.EqualValues(t, 6, rdb.Do(ctx, "dbsize").Val()) }) + t.Run("KEYS with non-trivial patterns", func(t *testing.T) { + require.NoError(t, rdb.FlushDB(ctx).Err()) + for _, key := range []string{"aa", "aab", "aabb", "ab", "abb"} { + require.NoError(t, rdb.Set(ctx, key, "hello", 0).Err()) + } + + keys := rdb.Keys(ctx, "a*").Val() + slices.Sort(keys) + require.Equal(t, []string{"aa", "aab", "aabb", "ab", "abb"}, keys) + + keys = rdb.Keys(ctx, "aa").Val() + slices.Sort(keys) + require.Equal(t, []string{"aa"}, keys) + + keys = rdb.Keys(ctx, "aa*").Val() + slices.Sort(keys) + require.Equal(t, []string{"aa", "aab", "aabb"}, keys) + + keys = rdb.Keys(ctx, "a?").Val() + slices.Sort(keys) + require.Equal(t, []string{"aa", "ab"}, keys) + + keys = rdb.Keys(ctx, "a*?").Val() + slices.Sort(keys) + require.Equal(t, []string{"aa", "aab", "aabb", "ab", "abb"}, keys) + + keys = rdb.Keys(ctx, "ab*").Val() + slices.Sort(keys) + require.Equal(t, []string{"ab", "abb"}, keys) + + keys = rdb.Keys(ctx, "*ab").Val() + slices.Sort(keys) + require.Equal(t, []string{"aab", "ab"}, keys) + + keys = rdb.Keys(ctx, "*ab*").Val() + slices.Sort(keys) + require.Equal(t, []string{"aab", "aabb", "ab", "abb"}, keys) + }) + t.Run("DEL all keys", func(t *testing.T) { vals := rdb.Keys(ctx, "*").Val() require.EqualValues(t, len(vals), rdb.Del(ctx, vals...).Val()) diff --git a/tests/gocase/unit/protocol/regression_test.go b/tests/gocase/unit/protocol/regression_test.go index 7dd4ea22b41..091bed26c29 100644 --- a/tests/gocase/unit/protocol/regression_test.go +++ b/tests/gocase/unit/protocol/regression_test.go @@ -23,7 +23,6 @@ import ( "context" "fmt" "testing" - "time" "github.com/apache/kvrocks/tests/gocase/util" "github.com/stretchr/testify/require" @@ -42,8 +41,6 @@ func TestRegression(t *testing.T) { proto := "*3\r\n$5\r\nBLPOP\r\n$6\r\nhandle\r\n$1\r\n0\r\n" require.NoError(t, c.Write(fmt.Sprintf("%s%s", proto, proto))) - // TODO: Remove time.Sleep after fix issue #2473 - time.Sleep(100 * time.Millisecond) resList := []string{"*2", "$6", "handle", "$1", "a"} v := rdb.RPush(ctx, "handle", "a") diff --git a/tests/gocase/unit/scan/scan_test.go b/tests/gocase/unit/scan/scan_test.go index cade5dcc5cc..5d2f9fb9a1f 100644 --- a/tests/gocase/unit/scan/scan_test.go +++ b/tests/gocase/unit/scan/scan_test.go @@ -77,7 +77,6 @@ func ScanTest(t *testing.T, rdb *redis.Client, ctx context.Context) { require.NoError(t, rdb.FlushDB(ctx).Err()) util.Populate(t, rdb, "", 1000, 10) keys := scanAll(t, rdb) - keys = slices.Compact(keys) require.Len(t, keys, 1000) }) @@ -85,7 +84,6 @@ func ScanTest(t *testing.T, rdb *redis.Client, ctx context.Context) { require.NoError(t, rdb.FlushDB(ctx).Err()) util.Populate(t, rdb, "", 1000, 10) keys := scanAll(t, rdb, "count", 5) - keys = slices.Compact(keys) require.Len(t, keys, 1000) }) @@ -93,15 +91,46 @@ func ScanTest(t *testing.T, rdb *redis.Client, ctx context.Context) { require.NoError(t, rdb.FlushDB(ctx).Err()) util.Populate(t, rdb, "key:", 1000, 10) keys := scanAll(t, rdb, "match", "key:*") - keys = slices.Compact(keys) require.Len(t, keys, 1000) }) - t.Run("SCAN MATCH invalid pattern", func(t *testing.T) { + t.Run("SCAN MATCH non-trivial pattern", func(t *testing.T) { require.NoError(t, rdb.FlushDB(ctx).Err()) - util.Populate(t, rdb, "*ab", 1000, 10) - // SCAN MATCH with invalid pattern should return an error - require.Error(t, rdb.Do(context.Background(), "SCAN", "match", "*ab*").Err()) + + for _, key := range []string{"aa", "aab", "aabb", "ab", "abb", "ba"} { + require.NoError(t, rdb.Set(ctx, key, "hello", 0).Err()) + } + + keys := scanAll(t, rdb, "match", "a*") + require.Equal(t, []string{"aa", "aab", "aabb", "ab", "abb"}, keys) + + keys = scanAll(t, rdb, "match", "aa") + require.Equal(t, []string{"aa"}, keys) + + keys = scanAll(t, rdb, "match", "aa*") + require.Equal(t, []string{"aa", "aab", "aabb"}, keys) + + keys = scanAll(t, rdb, "match", "a?") + require.Equal(t, []string{"aa", "ab"}, keys) + + keys = scanAll(t, rdb, "match", "a*?") + require.Equal(t, []string{"aa", "aab", "aabb", "ab", "abb"}, keys) + + keys = scanAll(t, rdb, "match", "ab*") + require.Equal(t, []string{"ab", "abb"}, keys) + + keys = scanAll(t, rdb, "match", "*ab") + require.Equal(t, []string{"aab", "ab"}, keys) + + keys = scanAll(t, rdb, "match", "*ab*") + require.Equal(t, []string{"aab", "aabb", "ab", "abb"}, keys) + + // Special case: using [b]* instead of b* forces the a full scan of the keyspace, + // matching every result with the pattern. We ask for exactly one key, but the + // first 5 keys don't match the pattern. This tests that SCAN returns a valid + // cursor even when the first [limit] keys don't satisfy the pattern. + keys = scanAll(t, rdb, "match", "[b]*", "count", "1") + require.Equal(t, []string{"ba"}, keys) }) t.Run("SCAN guarantees check under write load", func(t *testing.T) { @@ -226,6 +255,7 @@ func ScanTest(t *testing.T, rdb *redis.Client, ctx context.Context) { require.NoError(t, rdb.SAdd(ctx, "set", elements...).Err()) keys, _, err := rdb.SScan(ctx, "set", 0, "", 10000).Result() require.NoError(t, err) + slices.Sort(keys) keys = slices.Compact(keys) require.Len(t, keys, 100) }) @@ -307,6 +337,11 @@ func ScanTest(t *testing.T, rdb *redis.Client, ctx context.Context) { require.NoError(t, rdb.Do(ctx, "SCAN", "0", "match", "a*", "count", "1").Err()) util.ErrorRegexp(t, rdb.Do(ctx, "SCAN", "0", "count", "1", "match", "a*", "hello").Err(), ".*syntax error.*") util.ErrorRegexp(t, rdb.Do(ctx, "SCAN", "0", "count", "1", "match", "a*", "hello", "hi").Err(), ".*syntax error.*") + + util.ErrorRegexp(t, rdb.Do(ctx, "SCAN", "0", "match", "[").Err(), ".*Invalid glob pattern.*") + util.ErrorRegexp(t, rdb.Do(ctx, "SCAN", "0", "match", "\\").Err(), ".*Invalid glob pattern.*") + util.ErrorRegexp(t, rdb.Do(ctx, "SCAN", "0", "match", "[a").Err(), ".*Invalid glob pattern.*") + util.ErrorRegexp(t, rdb.Do(ctx, "SCAN", "0", "match", "[a-]").Err(), ".*Invalid glob pattern.*") }) t.Run("SCAN with type args ", func(t *testing.T) { @@ -406,6 +441,8 @@ func scanAll(t testing.TB, rdb *redis.Client, args ...interface{}) (keys []strin keys = append(keys, keyList...) if c == "0" { + slices.Sort(keys) + keys = slices.Compact(keys) return } } diff --git a/tests/gocase/unit/type/list/list_test.go b/tests/gocase/unit/type/list/list_test.go index 8b7a2c58f4e..f7509300236 100644 --- a/tests/gocase/unit/type/list/list_test.go +++ b/tests/gocase/unit/type/list/list_test.go @@ -376,22 +376,13 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() createList("blist", []string{"a", "b", large, "c", "d"}) - // TODO: Remove time.Sleep after fix issue #2473 - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blpop", "blist", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist", "a"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("brpop", "blist", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist", "d"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blpop", "blist", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist", "b"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("brpop", "blist", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist", "c"}) }) @@ -400,23 +391,15 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { defer func() { require.NoError(t, rd.Close()) }() createList("blist1", []string{"a", large, "c"}) createList("blist2", []string{"d", large, "f"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blpop", "blist1", "blist2", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist1", "a"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("brpop", "blist1", "blist2", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist1", "c"}) require.EqualValues(t, 1, rdb.LLen(ctx, "blist1").Val()) require.EqualValues(t, 3, rdb.LLen(ctx, "blist2").Val()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blpop", "blist2", "blist2", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist2", "d"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("brpop", "blist2", "blist2", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist2", "f"}) require.EqualValues(t, 1, rdb.LLen(ctx, "blist1").Val()) require.EqualValues(t, 1, rdb.LLen(ctx, "blist2").Val()) @@ -427,13 +410,9 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, "blist1").Err()) createList("blist2", []string{"d", large, "f"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blpop", "blist1", "blist2", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist2", "d"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("brpop", "blist1", "blist2", "1")) - time.Sleep(100 * time.Millisecond) rd.MustReadStrings(t, []string{"blist2", "f"}) require.EqualValues(t, 0, rdb.LLen(ctx, "blist1").Val()) require.EqualValues(t, 1, rdb.LLen(ctx, "blist2").Val()) @@ -444,25 +423,17 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, "list1", "list2").Err()) - time.Sleep(time.Millisecond * 100) require.NoError(t, rd.WriteArgs("blpop", "list1", "list2", "list2", "list1", "0")) - time.Sleep(time.Millisecond * 100) require.NoError(t, rdb.LPush(ctx, "list1", "a").Err()) rd.MustReadStrings(t, []string{"list1", "a"}) - time.Sleep(time.Millisecond * 100) require.NoError(t, rd.WriteArgs("blpop", "list1", "list2", "list2", "list1", "0")) - time.Sleep(time.Millisecond * 100) require.NoError(t, rdb.LPush(ctx, "list2", "b").Err()) rd.MustReadStrings(t, []string{"list2", "b"}) require.NoError(t, rdb.LPush(ctx, "list1", "a").Err()) require.NoError(t, rdb.LPush(ctx, "list2", "b").Err()) - time.Sleep(time.Millisecond * 100) require.NoError(t, rd.WriteArgs("blpop", "list1", "list2", "list2", "list1", "0")) - time.Sleep(time.Millisecond * 100) rd.MustReadStrings(t, []string{"list1", "a"}) - time.Sleep(time.Millisecond * 100) require.NoError(t, rd.WriteArgs("blpop", "list1", "list2", "list2", "list1", "0")) - time.Sleep(time.Millisecond * 100) rd.MustReadStrings(t, []string{"list2", "b"}) }) @@ -470,9 +441,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, "blist", "target").Err()) - time.Sleep(time.Millisecond * 100) require.NoError(t, rd.WriteArgs("blpop", "blist", "0")) - time.Sleep(time.Millisecond * 100) require.EqualValues(t, 2, rdb.LPush(ctx, "blist", "foo", "bar").Val()) rd.MustReadStrings(t, []string{"blist", "bar"}) require.Equal(t, "foo", rdb.LRange(ctx, "blist", 0, -1).Val()[0]) @@ -483,9 +452,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, "blist1").Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs(popType, "blist1", "0")) - time.Sleep(100 * time.Millisecond) require.NoError(t, rdb.RPush(ctx, "blist1", "foo").Err()) rd.MustReadStrings(t, []string{"blist1", "foo"}) require.EqualValues(t, 0, rdb.Exists(ctx, "blist1").Val()) @@ -495,7 +462,6 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rd.WriteArgs(popType, "blist1", "-1")) - time.Sleep(100 * time.Millisecond) rd.MustMatch(t, ".*negative.*") }) @@ -505,7 +471,6 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rd.WriteArgs(popType, "blist1", "0")) - time.Sleep(time.Millisecond * 1000) require.NoError(t, rdb.RPush(ctx, "blist1", "foo").Err()) rd.MustReadStrings(t, []string{"blist1", "foo"}) }) @@ -515,7 +480,6 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, "blist1", "blist2").Err()) require.NoError(t, rdb.Set(ctx, "blist2", "nolist", 0).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs(popType, "blist1", "blist2", "1")) rd.MustMatch(t, ".*WRONGTYPE.*") }) @@ -524,7 +488,6 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, "blist1", "blist2").Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs(popType, "blist1", "blist2", "1")) rd.MustMatch(t, "") }) @@ -533,16 +496,12 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, "blist1", "blist2").Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs(popType, "blist1", "blist2", "4")) - time.Sleep(100 * time.Millisecond) require.NoError(t, rdb.RPush(ctx, "blist1", "foo").Err()) rd.MustReadStrings(t, []string{"blist1", "foo"}) require.EqualValues(t, 0, rdb.Exists(ctx, "blist1").Val()) require.EqualValues(t, 0, rdb.Exists(ctx, "blist2").Val()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs(popType, "blist1", "blist2", "1")) - time.Sleep(100 * time.Millisecond) require.NoError(t, rdb.RPush(ctx, "blist2", "foo").Err()) rd.MustReadStrings(t, []string{"blist2", "foo"}) require.EqualValues(t, 0, rdb.Exists(ctx, "blist1").Val()) @@ -950,9 +909,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { require.NoError(t, rdb.Del(ctx, "target_key{t}").Err()) require.NoError(t, rdb.RPush(ctx, "target_key{t}", 1).Err()) createList("list{t}", []string{"a", "b", "c", "d"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("lmove", "list{t}", "target_key{t}", from, to)) - time.Sleep(100 * time.Millisecond) r, err1 := rd.ReadLine() require.Equal(t, "$1", r) require.NoError(t, err1) @@ -998,9 +955,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { require.NoError(t, rdb.Del(ctx, "target_key{t}").Err()) require.NoError(t, rdb.RPush(ctx, "target_key{t}", 1).Err()) createList("list{t}", []string{"a", "b", "c", "d"}) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmove", "list{t}", "target_key{t}", from, to, "1")) - time.Sleep(100 * time.Millisecond) r, err1 := rd.ReadLine() require.Equal(t, "$1", r) require.NoError(t, err1) @@ -1026,9 +981,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, "blist", "target").Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmove", "blist", "target", "left", "right", "0")) - time.Sleep(100 * time.Millisecond) require.EqualValues(t, 2, rdb.LPush(ctx, "blist", "foo", "bar").Val()) rd.MustRead(t, "$3") require.Equal(t, "bar", rdb.LRange(ctx, "target", 0, -1).Val()[0]) @@ -1436,9 +1389,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, key1, key2).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmpop", "1", "1", key1, direction, "count", "1")) - time.Sleep(time.Millisecond * 100) require.NoError(t, rdb.RPush(ctx, key1, "ONE", "TWO").Err()) if direction == "LEFT" { rd.MustReadStringsWithKey(t, key1, []string{"ONE"}) @@ -1452,9 +1403,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, key1, key2).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmpop", "1", "1", key1, direction, "count", "2")) - time.Sleep(time.Millisecond * 100) require.NoError(t, rdb.RPush(ctx, key1, "ONE", "TWO").Err()) if direction == "LEFT" { rd.MustReadStringsWithKey(t, key1, []string{"ONE", "TWO"}) @@ -1468,9 +1417,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, key1, key2).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmpop", "1", "1", key1, direction, "count", "10")) - time.Sleep(time.Millisecond * 100) require.NoError(t, rdb.RPush(ctx, key1, "ONE", "TWO").Err()) if direction == "LEFT" { rd.MustReadStringsWithKey(t, key1, []string{"ONE", "TWO"}) @@ -1484,9 +1431,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, key1, key2).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmpop", "1", "2", key1, key2, direction, "count", "2")) - time.Sleep(time.Millisecond * 100) require.NoError(t, rdb.RPush(ctx, key1, "ONE", "TWO").Err()) if direction == "LEFT" { rd.MustReadStringsWithKey(t, key1, []string{"ONE", "TWO"}) @@ -1500,9 +1445,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, key1, key2).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmpop", "1", "2", key1, key2, direction, "count", "2")) - time.Sleep(time.Millisecond * 100) require.NoError(t, rdb.RPush(ctx, key2, "one", "two").Err()) if direction == "LEFT" { rd.MustReadStringsWithKey(t, key2, []string{"one", "two"}) @@ -1516,9 +1459,10 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, key1, key2).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmpop", "1", "2", key1, key2, direction, "count", "2")) - time.Sleep(time.Millisecond * 100) + // https://github.com/apache/kvrocks/issues/2617 + // WriteArgs are required to be executed first + time.Sleep(100 * time.Millisecond) require.NoError(t, rdb.RPush(ctx, key2, "one", "two").Err()) require.NoError(t, rdb.RPush(ctx, key1, "ONE", "TWO").Err()) if direction == "LEFT" { @@ -1534,9 +1478,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, key1, key2).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmpop", "1", "2", key1, key2, direction)) - time.Sleep(time.Millisecond * 100) require.NoError(t, rdb.RPush(ctx, key2, "one", "two").Err()) if direction == "LEFT" { rd.MustReadStringsWithKey(t, key2, []string{"one"}) @@ -1551,9 +1493,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, key1, key2).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmpop", "1", "2", key1, key2, direction)) - time.Sleep(time.Millisecond * 1200) rd.MustMatch(t, "") }) @@ -1562,9 +1502,7 @@ func testList(t *testing.T, configs util.KvrocksServerConfigs) { rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() require.NoError(t, rdb.Del(ctx, key1, key2).Err()) - time.Sleep(100 * time.Millisecond) require.NoError(t, rd.WriteArgs("blmpop", "0", "2", key1, key2, direction, "count", "2")) - time.Sleep(time.Millisecond * 1200) require.NoError(t, rdb.RPush(ctx, key2, "one", "two").Err()) if direction == "LEFT" { rd.MustReadStringsWithKey(t, key2, []string{"one", "two"}) diff --git a/tests/gocase/unit/type/zset/zset_test.go b/tests/gocase/unit/type/zset/zset_test.go index ad57defaa85..fce7e96a652 100644 --- a/tests/gocase/unit/type/zset/zset_test.go +++ b/tests/gocase/unit/type/zset/zset_test.go @@ -332,8 +332,6 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, enabledRES rdb.ZAdd(ctx, "zsetb", redis.Z{Score: 1, Member: "d"}, redis.Z{Score: 2, Member: "e"}) require.EqualValues(t, 3, rdb.ZCard(ctx, "zseta").Val()) require.EqualValues(t, 2, rdb.ZCard(ctx, "zsetb").Val()) - // TODO: Remove time.Sleep after fix issue #2473 - time.Sleep(time.Millisecond * 100) resultz := rdb.BZPopMin(ctx, 0, "zseta", "zsetb").Val().Z require.Equal(t, redis.Z{Score: 1, Member: "a"}, resultz) resultz = rdb.BZPopMin(ctx, 0, "zseta", "zsetb").Val().Z @@ -349,9 +347,7 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, enabledRES rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() - time.Sleep(time.Millisecond * 100) require.NoError(t, rd.WriteArgs("bzpopmin", "zseta", "0")) - time.Sleep(time.Millisecond * 100) rdb.ZAdd(ctx, "zseta", redis.Z{Score: 1, Member: "a"}) rd.MustReadStrings(t, []string{"zseta", "a", "1"}) }) @@ -363,7 +359,6 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, enabledRES rdb.ZAdd(ctx, "zsetb", redis.Z{Score: 1, Member: "d"}, redis.Z{Score: 2, Member: "e"}) require.EqualValues(t, 3, rdb.ZCard(ctx, "zseta").Val()) require.EqualValues(t, 2, rdb.ZCard(ctx, "zsetb").Val()) - time.Sleep(time.Millisecond * 100) resultz := rdb.BZPopMax(ctx, 0, "zseta", "zsetb").Val().Z require.Equal(t, redis.Z{Score: 3, Member: "c"}, resultz) resultz = rdb.BZPopMax(ctx, 0, "zseta", "zsetb").Val().Z @@ -379,9 +374,7 @@ func basicTests(t *testing.T, rdb *redis.Client, ctx context.Context, enabledRES rd := srv.NewTCPClient() defer func() { require.NoError(t, rd.Close()) }() - time.Sleep(time.Millisecond * 100) require.NoError(t, rd.WriteArgs("bzpopmax", "zseta", "0")) - time.Sleep(time.Millisecond * 100) rdb.ZAdd(ctx, "zseta", redis.Z{Score: 1, Member: "a"}) rd.MustReadStrings(t, []string{"zseta", "a", "1"}) })