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

Refactor kvrocks2redis via rocksdb secondary instance #1963

Merged
merged 16 commits into from
Jan 3, 2024

Conversation

maochongxin
Copy link
Contributor

@maochongxin maochongxin commented Dec 22, 2023

Using RSI (Secondary Instance) for synchronization, replacing the previous method of using OpenForReadOnly followed by PSync.
First, open the data directory using OpenAsSecondary, and reuse the previous method of parsing the checkpoint to complete a "full sync". Then, use TryCatchUpWithPrimary to synchronize the new wal.

Try to fix #1832

@maochongxin
Copy link
Contributor Author

I'm still trying to add some integration tests

@tisonkun
Copy link
Member

Thanks for your contribution @maochongxin!

The PR description is empty. Can you add some background so that reviewers can understand the motivation and how you plan to implement it?

@maochongxin
Copy link
Contributor Author

Thanks for your contribution @maochongxin!

The PR description is empty. Can you add some background so that reviewers can understand the motivation and how you plan to implement it?

Thank you for the reminder. I have now provided the background information for this matter.

src/config/config.h Outdated Show resolved Hide resolved
src/storage/storage.cc Outdated Show resolved Hide resolved
src/storage/storage.cc Outdated Show resolved Hide resolved
Co-authored-by: Twice <twice@apache.org>
Copy link
Member

@PragmaTwice PragmaTwice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest looks good to me! It would be great if you can add some test for it.

@maochongxin
Copy link
Contributor Author

The rest looks good to me! It would be great if you can add some test for it.

Added test for data consistency between the source and destination nodes.

@PragmaTwice
Copy link
Member

The rest looks good to me! It would be great if you can add some test for it.

Added test for data consistency between the source and destination nodes.

Great! Could you also add tests for the incremental data? e.g. kvrocks2redis can catch new data after starting if it is written to the source kvrocks node.

PragmaTwice
PragmaTwice previously approved these changes Jan 2, 2024
@PragmaTwice
Copy link
Member

PragmaTwice commented Jan 2, 2024

We need to add apache license header to newly added files.

ERROR the following files don't have a valid license header: 
utils/kvrocks2redis/tests/check_consistency.py 

Refer to x.py for the same header.

@maochongxin
Copy link
Contributor Author

We need to add apache license header to newly added files.

ERROR the following files don't have a valid license header: 
utils/kvrocks2redis/tests/check_consistency.py 

Refer to x.py for the same header.

Thank you, updated.

Copy link

sonarcloud bot commented Jan 3, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

5 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@git-hulk git-hulk merged commit 574d438 into apache:unstable Jan 3, 2024
29 checks passed
@PragmaTwice
Copy link
Member

Thank you for your contribution!

@jihuayu jihuayu mentioned this pull request Mar 11, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor kvrocks2redis via rocksdb secondary instance
4 participants