From d9ecf554df90d458165846a5b19c330077d1fbef Mon Sep 17 00:00:00 2001 From: LiuYuHui Date: Fri, 15 Mar 2024 22:54:06 +0800 Subject: [PATCH 1/6] Feat: add support of readonly and readwrite --- src/cluster/cluster.cc | 3 ++- src/cluster/cluster.h | 9 +++++++++ src/commands/cmd_cluster.cc | 21 +++++++++++++++++++-- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/cluster/cluster.cc b/src/cluster/cluster.cc index ff98da876b1..ec40afb0d88 100644 --- a/src/cluster/cluster.cc +++ b/src/cluster/cluster.cc @@ -802,7 +802,8 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons } if (myself_ && myself_->role == kClusterSlave && !(attributes->flags & redis::kCmdWrite) && - nodes_.find(myself_->master_id) != nodes_.end() && nodes_[myself_->master_id] == slots_nodes_[slot]) { + nodes_.find(myself_->master_id) != nodes_.end() && nodes_[myself_->master_id] == slots_nodes_[slot] && + this->cluster_mode_ == Cluster::ClusterMode::READONLY) { return Status::OK(); // My master is serving this slot } diff --git a/src/cluster/cluster.h b/src/cluster/cluster.h index 8b8132ab0db..11fc1e76624 100644 --- a/src/cluster/cluster.h +++ b/src/cluster/cluster.h @@ -68,6 +68,8 @@ class SyncMigrateContext; class Cluster { public: + enum class ClusterMode { READONLY, READWRITE }; + explicit Cluster(Server *srv, std::vector binds, int port); Status SetClusterNodes(const std::string &nodes_str, int64_t version, bool force); Status GetClusterNodes(std::string *nodes_str); @@ -92,6 +94,11 @@ class Cluster { static bool SubCommandIsExecExclusive(const std::string &subcommand); + Status SetClusterMode(ClusterMode mode) { + cluster_mode_ = mode; + return Status::OK(); + } + private: std::string genNodesDescription(); std::string genNodesInfo(); @@ -111,4 +118,6 @@ class Cluster { std::map migrated_slots_; std::set imported_slots_; + + ClusterMode cluster_mode_{ClusterMode::READWRITE}; }; diff --git a/src/commands/cmd_cluster.cc b/src/commands/cmd_cluster.cc index 96ecfbde28b..6247cbab62f 100644 --- a/src/commands/cmd_cluster.cc +++ b/src/commands/cmd_cluster.cc @@ -292,8 +292,25 @@ static uint64_t GenerateClusterFlag(const std::vector &args) { return 0; } +class CommandReadOnly : public Commander { + public: + Status Execute(Server *srv, Connection *conn, std::string *output) override { + *output = redis::SimpleString("READONLY"); + return srv->cluster->SetClusterMode(Cluster::ClusterMode::READONLY); + } +}; + +class CommandReadWrite : public Commander { + public: + Status Execute(Server *srv, Connection *conn, std::string *output) override { + *output = redis::SimpleString("READWRITE"); + return srv->cluster->SetClusterMode(Cluster::ClusterMode::READWRITE); + } +}; + REDIS_REGISTER_COMMANDS(MakeCmdAttr("cluster", -2, "cluster no-script", 0, 0, 0, GenerateClusterFlag), - MakeCmdAttr("clusterx", -2, "cluster no-script", 0, 0, 0, - GenerateClusterFlag), ) + MakeCmdAttr("clusterx", -2, "cluster no-script", 0, 0, 0, GenerateClusterFlag), + MakeCmdAttr("readonly", 1, "cluster no-multi", 0, 0, 0), + MakeCmdAttr("readwrite", 1, "cluster no-multi", 0, 0, 0), ) } // namespace redis From 1a41210fc022c3d03d35f2c2e7c9dbc6f4875365 Mon Sep 17 00:00:00 2001 From: LiuYuHui Date: Sat, 16 Mar 2024 22:54:29 +0800 Subject: [PATCH 2/6] change default option to readonly --- src/cluster/cluster.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cluster/cluster.h b/src/cluster/cluster.h index 11fc1e76624..23b4eadfd20 100644 --- a/src/cluster/cluster.h +++ b/src/cluster/cluster.h @@ -119,5 +119,5 @@ class Cluster { std::map migrated_slots_; std::set imported_slots_; - ClusterMode cluster_mode_{ClusterMode::READWRITE}; + ClusterMode cluster_mode_{ClusterMode::READONLY}; }; From 41d55f61582f64f3faade3092636f936fc220324 Mon Sep 17 00:00:00 2001 From: LiuYuHui Date: Sun, 17 Mar 2024 14:45:01 +0800 Subject: [PATCH 3/6] add go integration test --- tests/gocase/integration/cluster/cluster_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/gocase/integration/cluster/cluster_test.go b/tests/gocase/integration/cluster/cluster_test.go index 8bc42fdafdb..b01e0f62877 100644 --- a/tests/gocase/integration/cluster/cluster_test.go +++ b/tests/gocase/integration/cluster/cluster_test.go @@ -369,4 +369,18 @@ func TestClusterMultiple(t *testing.T) { require.ErrorContains(t, rdb[1].Do(ctx, "EXEC").Err(), "EXECABORT") require.Equal(t, "no-multi", rdb[1].Get(ctx, util.SlotTable[0]).Val()) }) + + t.Run("requests on cluster are ok when enable readonly", func(t *testing.T) { + + require.NoError(t, rdb[3].Do(ctx, "READONLY").Err()) + require.NoError(t, rdb[2].Set(ctx, util.SlotTable[8192], 8192, 0).Err()) + util.WaitForOffsetSync(t, rdb[2], rdb[3]) + // request node3 that serves slot 8192, that's ok + require.Equal(t, "8192", rdb[3].Get(ctx, util.SlotTable[8192]).Val()) + + require.NoError(t, rdb[3].Do(ctx, "READWRITE").Err()) + + // when enable READWRITE, request node3 that serves slot 8192, that's not ok + util.ErrorRegexp(t, rdb[3].Get(ctx, util.SlotTable[8192]).Err(), fmt.Sprintf("MOVED 8192.*%d.*", srv[2].Port())) + }) } From 0dc9357078692cf2a6e0bb6bd237c7686864bf1f Mon Sep 17 00:00:00 2001 From: LiuYuHui Date: Sun, 17 Mar 2024 18:15:25 +0800 Subject: [PATCH 4/6] address comment --- src/cluster/cluster.cc | 2 +- src/cluster/cluster.h | 7 ------- src/commands/cmd_cluster.cc | 11 +++++++---- src/server/redis_connection.h | 1 + 4 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/cluster/cluster.cc b/src/cluster/cluster.cc index ec40afb0d88..7717de120d0 100644 --- a/src/cluster/cluster.cc +++ b/src/cluster/cluster.cc @@ -803,7 +803,7 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons if (myself_ && myself_->role == kClusterSlave && !(attributes->flags & redis::kCmdWrite) && nodes_.find(myself_->master_id) != nodes_.end() && nodes_[myself_->master_id] == slots_nodes_[slot] && - this->cluster_mode_ == Cluster::ClusterMode::READONLY) { + !conn->IsFlagEnabled(redis::Connection::KReadWrite)) { return Status::OK(); // My master is serving this slot } diff --git a/src/cluster/cluster.h b/src/cluster/cluster.h index 23b4eadfd20..c0eb5407d49 100644 --- a/src/cluster/cluster.h +++ b/src/cluster/cluster.h @@ -94,11 +94,6 @@ class Cluster { static bool SubCommandIsExecExclusive(const std::string &subcommand); - Status SetClusterMode(ClusterMode mode) { - cluster_mode_ = mode; - return Status::OK(); - } - private: std::string genNodesDescription(); std::string genNodesInfo(); @@ -118,6 +113,4 @@ class Cluster { std::map migrated_slots_; std::set imported_slots_; - - ClusterMode cluster_mode_{ClusterMode::READONLY}; }; diff --git a/src/commands/cmd_cluster.cc b/src/commands/cmd_cluster.cc index 6247cbab62f..aed7835ec7b 100644 --- a/src/commands/cmd_cluster.cc +++ b/src/commands/cmd_cluster.cc @@ -23,6 +23,7 @@ #include "cluster/sync_migrate_context.h" #include "commander.h" #include "error_constants.h" +#include "status.h" namespace redis { @@ -295,16 +296,18 @@ static uint64_t GenerateClusterFlag(const std::vector &args) { class CommandReadOnly : public Commander { public: Status Execute(Server *srv, Connection *conn, std::string *output) override { - *output = redis::SimpleString("READONLY"); - return srv->cluster->SetClusterMode(Cluster::ClusterMode::READONLY); + *output = redis::SimpleString("OK"); + conn->DisableFlag(redis::Connection::KReadWrite); + return Status::OK(); } }; class CommandReadWrite : public Commander { public: Status Execute(Server *srv, Connection *conn, std::string *output) override { - *output = redis::SimpleString("READWRITE"); - return srv->cluster->SetClusterMode(Cluster::ClusterMode::READWRITE); + *output = redis::SimpleString("OK"); + conn->EnableFlag(redis::Connection::KReadWrite); + return Status::OK(); } }; diff --git a/src/server/redis_connection.h b/src/server/redis_connection.h index 506f30a9ed0..ca4e6fc03e6 100644 --- a/src/server/redis_connection.h +++ b/src/server/redis_connection.h @@ -45,6 +45,7 @@ class Connection : public EvbufCallbackBase { kCloseAfterReply = 1 << 6, kCloseAsync = 1 << 7, kMultiExec = 1 << 8, + KReadWrite = 1 << 9, }; explicit Connection(bufferevent *bev, Worker *owner); From 9d16f254fff3d6c1284bfa18f60b6d7a8428c6e8 Mon Sep 17 00:00:00 2001 From: LiuYuHui Date: Sun, 17 Mar 2024 18:17:34 +0800 Subject: [PATCH 5/6] remove duplicate mode --- src/cluster/cluster.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/cluster/cluster.h b/src/cluster/cluster.h index c0eb5407d49..8b8132ab0db 100644 --- a/src/cluster/cluster.h +++ b/src/cluster/cluster.h @@ -68,8 +68,6 @@ class SyncMigrateContext; class Cluster { public: - enum class ClusterMode { READONLY, READWRITE }; - explicit Cluster(Server *srv, std::vector binds, int port); Status SetClusterNodes(const std::string &nodes_str, int64_t version, bool force); Status GetClusterNodes(std::string *nodes_str); From d33894cdd7e5cc66d8e33f0f4681f2882110f3bf Mon Sep 17 00:00:00 2001 From: LiuYuHui Date: Sun, 17 Mar 2024 19:52:55 +0800 Subject: [PATCH 6/6] change default option to READWRITE --- src/cluster/cluster.cc | 2 +- src/commands/cmd_cluster.cc | 4 ++-- src/server/redis_connection.h | 2 +- tests/gocase/integration/cluster/cluster_test.go | 5 +++++ 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/cluster/cluster.cc b/src/cluster/cluster.cc index 7717de120d0..f6f3525b4a9 100644 --- a/src/cluster/cluster.cc +++ b/src/cluster/cluster.cc @@ -803,7 +803,7 @@ Status Cluster::CanExecByMySelf(const redis::CommandAttributes *attributes, cons if (myself_ && myself_->role == kClusterSlave && !(attributes->flags & redis::kCmdWrite) && nodes_.find(myself_->master_id) != nodes_.end() && nodes_[myself_->master_id] == slots_nodes_[slot] && - !conn->IsFlagEnabled(redis::Connection::KReadWrite)) { + conn->IsFlagEnabled(redis::Connection::KReadOnly)) { return Status::OK(); // My master is serving this slot } diff --git a/src/commands/cmd_cluster.cc b/src/commands/cmd_cluster.cc index aed7835ec7b..99a79f14d5b 100644 --- a/src/commands/cmd_cluster.cc +++ b/src/commands/cmd_cluster.cc @@ -297,7 +297,7 @@ class CommandReadOnly : public Commander { public: Status Execute(Server *srv, Connection *conn, std::string *output) override { *output = redis::SimpleString("OK"); - conn->DisableFlag(redis::Connection::KReadWrite); + conn->EnableFlag(redis::Connection::KReadOnly); return Status::OK(); } }; @@ -306,7 +306,7 @@ class CommandReadWrite : public Commander { public: Status Execute(Server *srv, Connection *conn, std::string *output) override { *output = redis::SimpleString("OK"); - conn->EnableFlag(redis::Connection::KReadWrite); + conn->DisableFlag(redis::Connection::KReadOnly); return Status::OK(); } }; diff --git a/src/server/redis_connection.h b/src/server/redis_connection.h index ca4e6fc03e6..79b9dd18f8e 100644 --- a/src/server/redis_connection.h +++ b/src/server/redis_connection.h @@ -45,7 +45,7 @@ class Connection : public EvbufCallbackBase { kCloseAfterReply = 1 << 6, kCloseAsync = 1 << 7, kMultiExec = 1 << 8, - KReadWrite = 1 << 9, + KReadOnly = 1 << 9, }; explicit Connection(bufferevent *bev, Worker *owner); diff --git a/tests/gocase/integration/cluster/cluster_test.go b/tests/gocase/integration/cluster/cluster_test.go index b01e0f62877..695eb33cd24 100644 --- a/tests/gocase/integration/cluster/cluster_test.go +++ b/tests/gocase/integration/cluster/cluster_test.go @@ -334,6 +334,11 @@ func TestClusterMultiple(t *testing.T) { require.ErrorContains(t, rdb[3].Set(ctx, util.SlotTable[16383], 16383, 0).Err(), "MOVED") // request a read-only command to node3 that serve slot 16383, that's ok util.WaitForOffsetSync(t, rdb[2], rdb[3]) + //the default option is READWRITE, which will redirect both read and write to master + require.ErrorContains(t, rdb[3].Get(ctx, util.SlotTable[16383]).Err(), "MOVED") + + require.NoError(t, rdb[3].Do(ctx, "READONLY").Err()) + require.Equal(t, "16383", rdb[3].Get(ctx, util.SlotTable[16383]).Val()) })