Support rapid slot migration based on raw key value #1534

caipengbo · 2023-06-30T11:24:16Z

This PR introduces a new migration type called rawkv-based migration(in code named raw_key_value), which directly sends rocksdb's key values to the target.

As mentioned in in #1223, rawkv-based migration does not need to determine the expiration time during migration, which solves the data consistency problem and avoids the migration failure problem that frequently occurs when migrating data to be expired.

It can save a lot of CPU by eliminating the need to convert Key-Value to the RESP. In my tests, it can save up to 2x CPU on the target side; for small values(100byte), the rawkv-based migration is 2.75 times faster than the command-based migration

For the stability and code review simplicity, I did not combine the two migration types in this CR. Instead, I used two different if else branches. I've included the hiredis library for interacting with the destination, and in the future we can use hiredis to streamline command-based migrations as well.

Here is the basic idea of this pull:

Directly migrating the underlying data between different machines may run into clock rollback issues, which may introduce the problem of duplicate versions in complex data types, so before each migration, I use the TIME command to check the machine clock gap.
In order to avoid incomplete data caused by the expiration of the destination data during the migration process, we will prohibit the expiration of the slot during the migration
Instead of using a separate thread to transfer raw key value batches, I used the BATCHSET command so that the outside could use this command to inject data directly(may saving some CPU). BATCHSET command returns an integer, currently is the return data size, the reason for returning an integer is that I plan to make adaptive throttling.
I have introduced an iterator (MigrateIterator) and a sender (BatchSender). This decouples the traversal and sending, which makes the code clearer.

git-hulk · 2023-07-12T05:29:53Z

cool, thanks for your great efforts!

PragmaTwice · 2023-07-12T05:48:52Z

kvrocks.conf

+# - redis_command: Migrate data by redis serialization protocol(RESP).
+# - raw_key_value: Migrate the raw key value data of the storage engine directly.


redis-command and raw-key-value?

I want to distinguish the item value from the item name

I mean dash instead of underlines

PragmaTwice · 2023-07-12T05:55:34Z

src/cluster/slot_migrate.h

@@ -169,6 +195,7 @@ class SlotMigrator : public redis::Database {
  std::string dst_ip_;
  int dst_port_ = -1;
  UniqueFD dst_fd_;
+  redisContext *dst_redis_context_ = nullptr;


It seems the redisFree call for deallocating dst_redis_context_ is missing?

PragmaTwice · 2023-07-12T05:58:17Z

src/cluster/batch_sender.cc

+    if (reply != nullptr) {
+      freeReplyObject(reply);
+    }


nullptr check seems unnecessary here

seems the check still exists here

PragmaTwice

I recommend you to add unique_ptr wrappers for redisContext and redisReply pointers. You can refer to event_util.h.

PragmaTwice · 2023-07-12T07:07:02Z

It seems some memory leaks are found by ASAN.

https://github.com/apache/kvrocks/actions/runs/5527836049/jobs/10085132766?pr=1534

caipengbo · 2023-07-16T08:36:00Z

I recommend you to add unique_ptr wrappers for redisContext and redisReply pointers. You can refer to event_util.h.

@PragmaTwice I wanted to use unique_ptr to encapsulate redisContext and redisReply, but I found that it is only used for free. Currently hiredis does not use much, maybe simply handle it. We can better encapsulate hiredis when we use it more in the future.

PragmaTwice · 2023-07-16T09:11:27Z

src/cluster/batch_sender.cc

+  });
+
+  if (redis_context->err != 0) {
+    return {Status::NotOK, std::string(redis_context->errstr)};


Suggested change

return {Status::NotOK, std::string(redis_context->errstr)};

return {Status::NotOK, redis_context->errstr};

PragmaTwice · 2023-07-16T09:11:52Z

src/cluster/batch_sender.cc

+    auto error_str = std::string(reply->str);
+    return {Status::NotOK, error_str};


Suggested change

auto error_str = std::string(reply->str);

return {Status::NotOK, error_str};

return {Status::NotOK, reply->str};

actually i do not understand why construct a temp string here

PragmaTwice · 2023-07-16T09:18:37Z

src/cluster/slot_migrate.cc

+    return {Status::NotOK, "get invalid reply from TIME command"};
+  }
+
+  uint64_t dst_timestamp = std::stoul(reply->element[0]->str) * 1000000 + std::stoul(reply->element[1]->str);


stoul will throws exceptions while the parsing fails, so i wonder if the parsing will always succeed here?

Yes, normal TIME command will parse successfully.

PragmaTwice · 2023-07-16T09:30:40Z

src/common/io_util.cc

+Status CreateRedisContextFromConnectedFd(int fd, int timeout, redisContext **redis_context) {
+  *redis_context = redisConnectFd(fd);
+  if (*redis_context == nullptr) {
+    return {Status::NotOK, "init failed"};
+  }
+
+  if ((*redis_context)->err != 0) {
+    auto error_str = std::string((*redis_context)->errstr);
+    redisFree(*redis_context);
+    return {Status::NotOK, error_str};
+  }
+
+  if (redisSetTimeout(*redis_context, timeval{timeout, 0}) != REDIS_OK) {
+    redisFree(*redis_context);
+    return {Status::NotOK, "set timeout failed"};
+  }
+  return Status::OK();
+}


Suggested change

Status CreateRedisContextFromConnectedFd(int fd, int timeout, redisContext **redis_context) {

*redis_context = redisConnectFd(fd);

if (*redis_context == nullptr) {

return {Status::NotOK, "init failed"};

}

if ((*redis_context)->err != 0) {

auto error_str = std::string((*redis_context)->errstr);

redisFree(*redis_context);

return {Status::NotOK, error_str};

}

if (redisSetTimeout(*redis_context, timeval{timeout, 0}) != REDIS_OK) {

redisFree(*redis_context);

return {Status::NotOK, "set timeout failed"};

}

return Status::OK();

}

using StaticRedisFree = StaticFunction<decltype(redisFree), redisFree>;

struct UniqueRedisContext : std::unique_ptr<redisContext, StaticRedisFree> {

using BaseType = std::unique_ptr<redisContext, StaticRedisFree>;

using BaseType::BaseType;

};

StatusOr<UniqueRedisContext> CreateRedisContextFromConnectedFd(int fd, int timeout) {

auto redis_context = UniqueRedisContext{redisConnectFd(fd)};

if (!redis_context) {

return {Status::NotOK, "init failed"};

}

if (redis_context->err != 0) {

return {Status::NotOK, redis_context->errstr};

}

if (redisSetTimeout(redis_context.get(), timeval{timeout, 0}) != REDIS_OK) {

return {Status::NotOK, "set timeout failed"};

}

return redis_context;

}

git-hulk

It's a bit more complex than my imagination, maybe it needs some time to test on the local.

src/cluster/batch_sender.cc

git-hulk · 2023-08-07T15:16:31Z

src/cluster/migrate_iterator.cc

+  return log_data_;
+}
+
+void MigrateIterator::findMetaData() {


findMetaData => findMetadata

git-hulk · 2023-08-07T15:17:07Z

src/cluster/migrate_iterator.cc

+  if (redis_type == RedisType::kRedisStream) {
+    stream_iter_->Reset();
+  } else {
+    subdata_iter_->Reset();


subdata_iter_ => subkey_iter_ to keep consistent with the column family name

git-hulk · 2023-08-07T15:20:57Z

src/cluster/migrate_iterator.cc

+
+  items_.push_back(MigrateItem{metadata_cf_, metadata_iter_->key().ToString(), metadata_iter_->value().ToString()});
+
+  if (redis_type == RedisType::kRedisStream) {


If we can move this reset inside the initSubData?

Yes, it looks fine

PragmaTwice · 2023-08-09T01:10:48Z

For introducing hiredis, here are something need to be clarified:

how is TLS support of hiredis? can we directly pass SSL_CTX and ssl_st to it?
as hiredis is a c library, we need a c++ wrapper. kvrocks is not written in C, and we need to follow modern C++ principles. raw C style is not welcomed.
will the existing code (like, in replication) be replaced with hiredis? or more concrete, how hiredis can help with the client connection reading/writing?

caipengbo · 2023-08-09T01:26:23Z

hiredis isn't used much at the moment, so I've included a lightweight hiredis.

There is no TLS requirement, we can simply wrapper hiredis now.

In the future, if we need more, we can introduce some more complex clients, such as redis-plus-plus, for now I think a lightweight library hiredis will do.

PragmaTwice · 2023-08-09T01:28:43Z

There is no TLS requirement

TLS support for replication is an ongoing effort, and we will support TLS for slot migration in the future. So we need to consider these problems now, to prevent TLS being more hard to support.

caipengbo · 2023-08-09T01:33:39Z

In the future, if there is a need, we can import other libraries and use this library on other existing code. For now, we can start with a simple library.

PragmaTwice · 2023-08-09T01:41:44Z

In the future, if there is a need, we can import other libraries and use this library on other existing code.

We cannot introduce libraries so casually.

When introducing a library, we need to make sure that it can indeed solve the requirements/simplify the code and is easy to maintain. We will not randomly introduce a library in the early stages and then introduce other similar libraries later when problems arise: this is not something a well-managed project should do, as it would make the code worse by having multiple libraries doing the same thing. We need to carefully consider before introducing any library.

I am not against introducing hiredis, but I believe that these issues should be considered clearly when introducing it. Instead of "randomly introducing a simple library and considering what comes next later."

hiredis seems to support TLS, but I'm not sure if it can reuse our SSL_CTX instead of reconstructing it.

cc @git-hulk

caipengbo · 2023-08-09T01:46:12Z

We certainly can't just arbitrarily import a library, so I've introduced the most official, and simplest library — hiredis.

git-hulk · 2023-08-09T01:55:06Z

Yes, need to think through before introducing the new library. Kvrocks's dependencies are more and more, we need to take care of this situation to avoid being out of control.

PragmaTwice · 2023-08-09T02:00:38Z

We certainly can't just arbitrarily import a library, so I've introduced the most official, and simplest library — hiredis.

I know that hiredis is the official library, but it doesn't seem to answer our question.

I would like to provide more specific explanations and understand the context of introducing hiredis:

In the current codebase, such as in replication, when we need to construct a client connection, we directly build a raw TCP connection and then use our own redis protocol tools (such as redis::SimpleString and other protocol encoding/decoding functions).

These processes are not well encapsulated and there are even some manual protocol constructions, such as SockSend(..., "+OK\r\n") (although it is not in a client), which is an existing issue.

Of course, we can introduce a library to solve this problem. However, it is important to consider the aforementioned points when introducing a library:

in Support TLS for replication #1630, we add TLS support for client connection. how can we rewrite them in hiredis? (as we do not want to have multiple ways to construct client connection)
some code style problem as hiredis is a c library
how to replace the existing code (like, in replication) with hiredis

caipengbo · 2023-08-09T02:12:56Z

I understand what you mean, you want to use a client library to do all the things you mentioned above.

I introduced the hiredis just to make it easier to send and receive responses and I've also privately written a multi-threaded version of the kvrocks migration tool that also uses hiredis. I'm not thinking that we have to use hiredis to do everything related to the redis protocol and redis parsing in the existing code.

These are the things you mention that I probably don't have the answers to yet, and I don't have the answers yet to figure out how to do them, so introducing a simple, easily replaceable library is the best option for now. If we come up with a better way to handle the things you mentioned above, we can easily replace hiredis(reuse is better).

git-hulk · 2023-08-09T02:26:50Z

I'm fine to import the hiredis which can make the read/write operation easier, but for other libraries like redis-plus-plus, it needs the solid reason why we need it.

caipengbo added 5 commits June 30, 2023 19:20

Slot migration based on raw key value

d1f8c6c

Support stream and add testcase

0bb556f

merge to latest unstable

edb833f

Rename MigrateBatch to BatchSender

4b386e9

format code

e54a756

caipengbo marked this pull request as ready for review July 12, 2023 05:27

caipengbo requested review from git-hulk, PragmaTwice and torwig July 12, 2023 05:28

PragmaTwice reviewed Jul 12, 2023

View reviewed changes

PragmaTwice changed the title ~~Slot migration based on raw key value~~ Support rapid slot migration based on raw key value Jul 12, 2023

xiaobiaozhao self-requested a review July 13, 2023 14:40

Minor revision

19a2403

caipengbo requested a review from PragmaTwice July 16, 2023 08:37

PragmaTwice reviewed Jul 16, 2023

View reviewed changes

git-hulk added the major decision Requires project management committee consensus label Aug 2, 2023

git-hulk reviewed Aug 7, 2023

View reviewed changes

git-hulk mentioned this pull request Aug 8, 2023

Track issues: improvements of the kvrocks2redis #1643

Open

7 tasks

git-hulk mentioned this pull request Jan 3, 2024

Improve slot migration speed and resource consumption using raw key values #1223

Closed

5 tasks

git-hulk closed this Jan 26, 2024

caipengbo mentioned this pull request Jan 30, 2024

Allow to migrate cluster slot by raw write batch #2067

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support rapid slot migration based on raw key value #1534

Support rapid slot migration based on raw key value #1534

caipengbo commented Jun 30, 2023 •

edited

Loading

git-hulk commented Jul 12, 2023

PragmaTwice Jul 12, 2023

caipengbo Jul 12, 2023

PragmaTwice Jul 12, 2023

PragmaTwice Jul 12, 2023

PragmaTwice Jul 12, 2023

PragmaTwice Jul 16, 2023

PragmaTwice left a comment •

edited

Loading

PragmaTwice commented Jul 12, 2023 •

edited

Loading

caipengbo commented Jul 16, 2023 •

edited

Loading

PragmaTwice Jul 16, 2023

PragmaTwice Jul 16, 2023

PragmaTwice Jul 16, 2023

PragmaTwice Jul 16, 2023 •

edited

Loading

caipengbo Jul 16, 2023

PragmaTwice Jul 16, 2023 •

edited

Loading

git-hulk left a comment

git-hulk Aug 7, 2023

git-hulk Aug 7, 2023

git-hulk Aug 7, 2023

caipengbo Aug 9, 2023

PragmaTwice commented Aug 9, 2023 •

edited

Loading

caipengbo commented Aug 9, 2023

PragmaTwice commented Aug 9, 2023 •

edited

Loading

caipengbo commented Aug 9, 2023

PragmaTwice commented Aug 9, 2023

caipengbo commented Aug 9, 2023 •

edited

Loading

git-hulk commented Aug 9, 2023

PragmaTwice commented Aug 9, 2023 •

edited

Loading

caipengbo commented Aug 9, 2023 •

edited

Loading

git-hulk commented Aug 9, 2023

		# - redis_command: Migrate data by redis serialization protocol(RESP).
		# - raw_key_value: Migrate the raw key value data of the storage engine directly.

	return {Status::NotOK, std::string(redis_context->errstr)};
	return {Status::NotOK, redis_context->errstr};

		auto error_str = std::string(reply->str);
		return {Status::NotOK, error_str};

	auto error_str = std::string(reply->str);
	return {Status::NotOK, error_str};
	return {Status::NotOK, reply->str};


		items_.push_back(MigrateItem{metadata_cf_, metadata_iter_->key().ToString(), metadata_iter_->value().ToString()});

		if (redis_type == RedisType::kRedisStream) {

Support rapid slot migration based on raw key value #1534

Support rapid slot migration based on raw key value #1534

Conversation

caipengbo commented Jun 30, 2023 • edited Loading

git-hulk commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice left a comment • edited Loading

Choose a reason for hiding this comment

PragmaTwice commented Jul 12, 2023 • edited Loading

caipengbo commented Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

git-hulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragmaTwice commented Aug 9, 2023 • edited Loading

caipengbo commented Aug 9, 2023

PragmaTwice commented Aug 9, 2023 • edited Loading

caipengbo commented Aug 9, 2023

PragmaTwice commented Aug 9, 2023

caipengbo commented Aug 9, 2023 • edited Loading

git-hulk commented Aug 9, 2023

PragmaTwice commented Aug 9, 2023 • edited Loading

caipengbo commented Aug 9, 2023 • edited Loading

git-hulk commented Aug 9, 2023

caipengbo commented Jun 30, 2023 •

edited

Loading

PragmaTwice left a comment •

edited

Loading

PragmaTwice commented Jul 12, 2023 •

edited

Loading

caipengbo commented Jul 16, 2023 •

edited

Loading

PragmaTwice Jul 16, 2023 •

edited

Loading

PragmaTwice Jul 16, 2023 •

edited

Loading

PragmaTwice commented Aug 9, 2023 •

edited

Loading

PragmaTwice commented Aug 9, 2023 •

edited

Loading

caipengbo commented Aug 9, 2023 •

edited

Loading

PragmaTwice commented Aug 9, 2023 •

edited

Loading

caipengbo commented Aug 9, 2023 •

edited

Loading