Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
5 tasks done
caipengbo opened this issue Jan 11, 2023 · 15 comments
Closed
5 tasks done
Assignees
Labels

Comments

@caipengbo
Copy link
Contributor

caipengbo commented Jan 11, 2023

Motivation

Currently, kvrocks performs slot migration by converting the data into RESP and then sending it to the destination(#430).
We made a little optimizations in #904 to speed up migration.

However, there are still some problems with command-based slot migration:

Solution

We can use rawkv-based data migration, where we directly send rocksdb's key values data to the target.

With rawkv-based migration, neither the source nor the destination need to judge expire time, which completely solves the data inconsistency problem. It is equivalent to translating the raw key values data from one instance to another.

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

Plan

I decomposed this issues into those tasks:

@caipengbo caipengbo added the enhancement type enhancement label Jan 11, 2023
@caipengbo caipengbo self-assigned this Jan 11, 2023
@git-hulk
Copy link
Member

@caipengbo Do you mean that we can send the write batch directly instead of the RESP format?

@caipengbo
Copy link
Contributor Author

Do you mean that we can send the write batch directly instead of the RESP format?

@git-hulk Yup, we send the write batch directly, and the target calls the rocksdb::Write() interface directly

@git-hulk
Copy link
Member

Do you mean that we can send the write batch directly instead of the RESP format?

@git-hulk Yup, we send the write batch directly, and the target calls the rocksdb::Write() interface directly

Got it, thanks a lot

@git-hulk git-hulk added help wanted Good for newcomers good first issue Good for newcomers labels Apr 14, 2023
@ZENOTME
Copy link
Contributor

ZENOTME commented Apr 28, 2023

I'm interested in this issue, but I'm confused about

With rawkv-based migration, neither the source nor the destination need to judge expire time

Do you mean that we can send the write batch directly instead of the RESP format?

Does it means that: for a key-value pair with expire (k,v,expire), just send the (k,v) and ignore the expire?

@caipengbo
Copy link
Contributor Author

Does it means that: for a key-value pair with expire (k,v,expire), just send the (k,v) and ignore the expire?

Yes, it ignore the expire. Let the target determine the expire.

@ZENOTME
Copy link
Contributor

ZENOTME commented Apr 28, 2023

Let the target determine the expire.

Do you means the case like:

  1. migrate (k,v) to target, and now (k,v) don't have expire
  2. And then target get a command like 'EXPIRE k 10', now (k,v) have a new expire.

@caipengbo
Copy link
Contributor Author

Do you means the case like:

Yes

@git-hulk
Copy link
Member

git-hulk commented May 9, 2023

@caipengbo Can you submit an issue to track this?

@caipengbo
Copy link
Contributor Author

@caipengbo Can you submit an issue to track this?

Yeah, I plan to solve this issue after slot batch PR merged @git-hulk

@caipengbo caipengbo self-assigned this May 9, 2023
@git-hulk
Copy link
Member

git-hulk commented May 9, 2023

@caipengbo Thanks a lot

@git-hulk
Copy link
Member

git-hulk commented Jan 3, 2024

@caipengbo PR #1534 has been pending for a while. This feature is valuable since it can help the cluster migration avoid depending on the log data of the write batch except for the performance benefit. I'm very eager to see this feature done, but #1534 has too many conflicts and it's a bit hard to review for now.

So I'm not sure if you're willing to continue working on this feature. If yes, I think we can
break down this feature into a few PRs and add more tests like:

  • Add the ApplyBatch command
  • Implement the merge iterator for iterating SST keys
  • Implement WAL iterator for migrating by WAL logs
  • Implement the migration by applying raw batch
  • Make the raw batch migration parameters configurable

@caipengbo
Copy link
Contributor Author

So I'm not sure if you're willing to continue working on this feature. If yes, I think we can break down this feature into a few PRs and add more tests.

Indeed, the previous code was too much for CR, and I was happy to go ahead and split it up into multiple PRs. @git-hulk

@git-hulk
Copy link
Member

git-hulk commented Jan 3, 2024

Indeed, the previous code was too much for CR, and I was happy to go ahead and split it up into multiple PRs. @git-hulk

@caipengbo Thank you!

@caipengbo
Copy link
Contributor Author

I'm going to restart this work. I tracked some tasks in this issue, and I may change the task names in the future PRs.

@caipengbo caipengbo added tracking-issue and removed good first issue Good for newcomers help wanted Good for newcomers labels Jan 3, 2024
@git-hulk
Copy link
Member

git-hulk commented Jan 3, 2024

I'm going to restart this work. I tracked some tasks in this issue, and I may change the task names in the future PRs.

cool, can also convert those sub-tasks into tracking issues.

@git-hulk git-hulk closed this as completed Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

No branches or pull requests

3 participants