apache / kvrocks

Apache Kvrocks is a distributed key value NoSQL database that uses RocksDB as storage engine and is compatible with Redis protocol.
https://kvrocks.apache.org/
Apache License 2.0
3.44k stars 442 forks source link

Persist the cluster-enabled status in RocksDB #1410

Open git-hulk opened 1 year ago

git-hulk commented 1 year ago

Search before asking

Motivation

Currently, we encode the slot id as the prefix of the key if the cluster mode was enabled according to Kvrocks's configuration. But it would go wrong if we start the Kvrocks with cluster-enabled yes, but change it to cluster-enabled no after restarting.

For the key encoding can see: https://kvrocks.apache.org/community/data-structure-on-rocksdb

Solution

Persist the cluster-enabled configuration in the RocksDB(can use propagate column family) when starting the server, then check if the Kvrocks's cluster-enabled configuration matched the database status.

Are you willing to submit a PR?

infdahai commented 1 year ago

Let's try this.

git-hulk commented 1 year ago

@infdahai Assigned, let's go ahead.

infdahai commented 1 year ago

@git-hulk I find many go tests use the config map including "cluster-enabled": "yes" and the StartServerWithCLIOptions function writes the option to the kvrocks.conf file.

func TestClusterDumpAndLoadClusterNodesInfo(t *testing.T) {
    srv1 := util.StartServer(t, map[string]string{
        "bind":            "0.0.0.0",
        "cluster-enabled": "yes",
    })

in StartServerWithCLIOptions func(), https://github.com/apache/incubator-kvrocks/blob/30e9fd788a4acb585566a008f4d5fff65ee06ce3/tests/gocase/util/server.go#L197-L207

I haven't seen a commad that changes cluster mode, so it means we should define the option in the initial config file if we want to use cluster mode. Restart() will read the kvrocks.conf and set the origin value. https://github.com/apache/incubator-kvrocks/blob/30e9fd788a4acb585566a008f4d5fff65ee06ce3/tests/gocase/util/server.go#L116-L128

change it to cluster-enabled no` after restarting.

so I don't know how this happens, could you explain it?

git-hulk commented 1 year ago

Hi @infdahai

Sorry for missing this comment.

I haven't seen a commad that changes cluster mode, so it means we should define the option in the initial config file if we want to use cluster mode. Restart() will read the kvrocks.conf and set the origin value.

Yeah, that's right that we have no command to set the cluster mode, it must be specified in the configuration file. But the current restart function cannot change the kvrocks.conf, so you need to support overriding the configuration in the restart function first if wanna test this scenario.

git-hulk commented 1 year ago

@inf Are you still working on this?

infdahai commented 1 year ago

@git-hulk I think I know what to do. A new pull request is on the way.

git-hulk commented 1 year ago

Cool, thanks. No hurry, just for asking if you're working on this.

chrisxu333 commented 7 months ago

@infdahai are you still working on this? If not I'd like to take over. @git-hulk

git-hulk commented 7 months ago

@chrisxu333 Done, thank you!