Add the support of the load RDB command #1798

xq2010 · 2023-10-09T05:37:46Z

This PR is for #1753

git-hulk · 2023-10-09T06:31:52Z

Hi @xq2010 Thanks for your excellent contribution first!

About how to load the RDB file, I'm wondering if it's better to export a command to do this. For example, we can support a new command like load $type $path which allows loading RDB file from local.

$ redis-cli 6666> load RDB /path/to/your/rdb_file

So that we don't need to introduce a new tool. How do you feel about this? cc @apache/kvrocks-committers

xq2010 · 2023-10-09T06:45:27Z

Hi @xq2010 Thanks for your excellent contribution first!↳

About how to load the RDB file, I'm wondering if it's better to export a command to do this. For example, we can support a new command like load $type $path which allows loading RDB file from local.↳
$ redis-cli 6666> load RDB /path/to/your/rdb_file
So that we don't need to introduce a new tool. How do you feel about this? cc @apache/kvrocks-committers↳

If we add the 'load rdb' command, we need to determine how to process an existing key that's present in both kvrocks and rdb, particularly when the key's type is a string in rdb. It will overwrite the existing key in kvrocks, which may be of a different type."

git-hulk · 2023-10-09T06:49:27Z

If we add the 'load rdb' command, we need to determine how to process an existing key that's present in both kvrocks and rdb, particularly when the key's type is a string in rdb. It will overwrite the existing key in kvrocks, which may be of a different type."

Yes, it's a good concern. I prefer overwriting by default if it exists and we can offer an option NX like the set command to let users determine in the future. This is my personal perspective, would like to hear more voices about the loading way design.

xq2010 · 2023-10-09T06:58:22Z

Yes, it's a good concern. I prefer overwriting by default if it exists and we can offer an option NX like the set command to let users determine in the future. This is my personal perspective, would like to hear more voices about the loading way design.↳

Using NX is a good idea, I would also like to hear more opinions.

tests/testdata/corrupt_empty_keys.rdb

PragmaTwice · 2023-10-09T11:22:35Z

Thanks for your contribution!

Agree with @git-hulk, I think it would be better to just implement it like a command inside the kvrocks process instead of a new program.

xq2010 · 2023-10-09T14:44:48Z

@git-hulk @PragmaTwice Okay, I will add a command to load the RDB file. The command might be rdb load <path> [namespace]. Do you have any suggestions or advice?

git-hulk · 2023-10-09T14:48:02Z

@git-hulk @PragmaTwice Okay, I will add a command to load the RDB file. The command might be rdb load [namespace]. Do you have any suggestions or advice?

Thank you! Sounds good to me, but [namespace] seems unnecessary since it knows the current namespace once it enters the command.

xq2010 · 2023-10-09T15:24:36Z

Thank you! Sounds good to me, but [namespace] seems unnecessary since it knows the current namespace once it enters the command.↳

Yes, it's unnecessary to use a namespace.

If the RDB file contains multiple db, the current implementation sets the namespace of each db to <ns>_<db number>. The password will be the same as the namespace. Only db 0 will use the current namespace.
Perhaps using a parameter to specify which db to load would be better. If so, the command would be rdb load <path> [database number (default 0)].

git-hulk · 2023-10-09T15:34:39Z

Perhaps using a parameter to specify which db to load would be better. If so, the command would be rdb load [database number (default 0)].

@xq2010 Yes, I think this is a good solution.

… to load rdb file, delete redis2kvrocks and rdb files

git-hulk · 2023-10-10T15:31:48Z

@xq2010 Cool! The overall design is nice. I'll take another pass to take a look at detail implementation tomorrow.

tisonkun

Generally looks good. But I don't have time to check in details for giving an approval on the implementation 🤣

CMakeLists.txt

…efactor rdb_test, and remove testdatas

src/storage/rdb.h

src/storage/rdb.cc

tests/cppunit/rdb_test.cc

tests/cppunit/rdb_util.h

tests/cppunit/rdb_stream_test.cc

src/storage/rdb.cc

src/commands/cmd_server.cc

src/common/rdb_stream.h

src/commands/cmd_server.cc

torwig

@xq2010 Good job, thank you for your effort. Just a little nitpicking from me :)

src/common/rdb_stream.cc

src/storage/rdb.cc

src/common/rdb_stream.h

xq2010 · 2023-10-12T02:33:58Z

Thanks for your review, I will review it later

src/common/rdb_stream.cc

src/common/rdb_stream.h

git-hulk · 2023-10-16T06:40:33Z

Hi @xq2010 How's this PR going? Should we take another round review?

xq2010 · 2023-10-16T08:00:53Z

@git-hulk Thank you for your review. I've made some corrections. Could you please review it once more?

git-hulk · 2023-10-16T08:04:05Z

@git-hulk Thank you for your review. I've made some corrections. Could you please review it once more?

Sure, thanks for your efforts!

git-hulk · 2023-10-17T08:41:30Z

I would like to merge this PR today if no more comments.

mapleFU · 2023-10-17T08:52:08Z

I'm a bit busy these few days. I'll go through it before Thursday. You can merge this first if other doesn't have any comments.

torwig

Generally, LGTM.

mapleFU · 2023-10-19T13:56:02Z

src/common/rdb_stream.cc

+#include "fmt/format.h"
+#include "vendor/crc64.h"
+
+StatusOr<size_t> RdbStringStream::Read(char *buf, size_t n) {


The syntax of RdbStringStream is different from RdbFileStream. The previous one will return a smaller size when n is greater than remaining buffer, but RdbFileStream will always return Status::NotOK. Is this expected?

Oh I make it wrong. So they're same.

mapleFU · 2023-10-19T14:02:22Z

src/storage/rdb.cc

+  std::vector<char> vec(len);
+  GET_OR_RET(stream_->Read(vec.data(), len));


Could it be len == 0?

Yes, it is possible, could you help me fix it?

#1839
I've create a pull request for it.

Thanks again for your contribution!

feat(load rdb file): add redis2kvrocks to load rdb file to kvrocks

30904c1

xq2010 mentioned this pull request Oct 9, 2023

Add the support of loading data from RDB file #1753

Closed

2 tasks

tisonkun reviewed Oct 9, 2023

View reviewed changes

tests/testdata/corrupt_empty_keys.rdb Outdated Show resolved Hide resolved

xq2010 and others added 2 commits October 10, 2023 18:52

Merge branch 'apache:unstable' into feature/redis2kvrocks

df55a52

refactor(load rdb file): use command: rdb load <path> [NX] [DB index]…

75355de

… to load rdb file, delete redis2kvrocks and rdb files

git-hulk requested review from git-hulk, torwig, PragmaTwice, enjoy-binbin and tisonkun October 10, 2023 14:44

tisonkun reviewed Oct 10, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Revert changes in CMakeLists.txt, fix test errors of RdbFileStream, r…

b47820e

…efactor rdb_test, and remove testdatas

git-hulk reviewed Oct 11, 2023

View reviewed changes

git-hulk requested a review from tisonkun October 11, 2023 11:53

torwig reviewed Oct 11, 2023

View reviewed changes

src/commands/cmd_server.cc Outdated Show resolved Hide resolved

src/common/rdb_stream.h Outdated Show resolved Hide resolved

PragmaTwice reviewed Oct 11, 2023

View reviewed changes

src/commands/cmd_server.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed Oct 11, 2023

View reviewed changes

src/commands/cmd_server.cc Outdated Show resolved Hide resolved

git-hulk changed the title ~~feat(load rdb file): add redis2kvrocks to load rdb file to kvrocks~~ Add the support of the load RDB command Oct 11, 2023

torwig reviewed Oct 11, 2023

View reviewed changes

PragmaTwice reviewed Oct 11, 2023

View reviewed changes

src/common/rdb_stream.h Show resolved Hide resolved

mapleFU reviewed Oct 12, 2023

View reviewed changes

src/common/rdb_stream.cc Show resolved Hide resolved

src/common/rdb_stream.cc Outdated Show resolved Hide resolved

src/common/rdb_stream.h Outdated Show resolved Hide resolved

fix:Adjust the code according to the review

cfd1565

git-hulk requested review from torwig, mapleFU, git-hulk and PragmaTwice October 16, 2023 08:04

Merge branch 'unstable' into feature/redis2kvrocks

ba998cc

git-hulk approved these changes Oct 16, 2023

View reviewed changes

Merge branch 'unstable' into feature/redis2kvrocks

3f26e8d

torwig approved these changes Oct 17, 2023

View reviewed changes

git-hulk merged commit 45deba9 into apache:unstable Oct 17, 2023
28 checks passed

xq2010 deleted the feature/redis2kvrocks branch October 18, 2023 06:34

git-hulk mentioned this pull request Oct 18, 2023

Add rdb load command apache/kvrocks-website#167

Merged

mapleFU reviewed Oct 19, 2023

View reviewed changes

mapleFU mentioned this pull request Oct 19, 2023

RDB: style enhancement for rdb load #1839

Merged

git-hulk mentioned this pull request Nov 13, 2023

Restore command parse string error #1894

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the support of the load RDB command #1798

Add the support of the load RDB command #1798

xq2010 commented Oct 9, 2023

git-hulk commented Oct 9, 2023 •

edited

Loading

xq2010 commented Oct 9, 2023 •

edited

Loading

git-hulk commented Oct 9, 2023 •

edited

Loading

xq2010 commented Oct 9, 2023

PragmaTwice commented Oct 9, 2023

xq2010 commented Oct 9, 2023 •

edited

Loading

git-hulk commented Oct 9, 2023

xq2010 commented Oct 9, 2023

git-hulk commented Oct 9, 2023 •

edited

Loading

git-hulk commented Oct 10, 2023

tisonkun left a comment

torwig left a comment

xq2010 commented Oct 12, 2023

git-hulk commented Oct 16, 2023

xq2010 commented Oct 16, 2023

git-hulk commented Oct 16, 2023

git-hulk commented Oct 17, 2023

mapleFU commented Oct 17, 2023

torwig left a comment

mapleFU Oct 19, 2023

mapleFU Oct 19, 2023

mapleFU Oct 19, 2023

xq2010 Oct 20, 2023

mapleFU Oct 20, 2023

		std::vector<char> vec(len);
		GET_OR_RET(stream_->Read(vec.data(), len));

Add the support of the load RDB command #1798

Add the support of the load RDB command #1798

Conversation

xq2010 commented Oct 9, 2023

git-hulk commented Oct 9, 2023 • edited Loading

xq2010 commented Oct 9, 2023 • edited Loading

git-hulk commented Oct 9, 2023 • edited Loading

xq2010 commented Oct 9, 2023

PragmaTwice commented Oct 9, 2023

xq2010 commented Oct 9, 2023 • edited Loading

git-hulk commented Oct 9, 2023

xq2010 commented Oct 9, 2023

git-hulk commented Oct 9, 2023 • edited Loading

git-hulk commented Oct 10, 2023

tisonkun left a comment

Choose a reason for hiding this comment

torwig left a comment

Choose a reason for hiding this comment

xq2010 commented Oct 12, 2023

git-hulk commented Oct 16, 2023

xq2010 commented Oct 16, 2023

git-hulk commented Oct 16, 2023

git-hulk commented Oct 17, 2023

mapleFU commented Oct 17, 2023

torwig left a comment

Choose a reason for hiding this comment

mapleFU Oct 19, 2023

Choose a reason for hiding this comment

mapleFU Oct 19, 2023

Choose a reason for hiding this comment

mapleFU Oct 19, 2023

Choose a reason for hiding this comment

xq2010 Oct 20, 2023

Choose a reason for hiding this comment

mapleFU Oct 20, 2023

Choose a reason for hiding this comment

git-hulk commented Oct 9, 2023 •

edited

Loading

xq2010 commented Oct 9, 2023 •

edited

Loading

git-hulk commented Oct 9, 2023 •

edited

Loading

xq2010 commented Oct 9, 2023 •

edited

Loading

git-hulk commented Oct 9, 2023 •

edited

Loading