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

Add support of stream migration during slot migration process #1197

Merged
merged 3 commits into from
Dec 21, 2022

Conversation

torwig
Copy link
Contributor

@torwig torwig commented Dec 19, 2022

The XSETID command was implemented. This command is used only during stream key migration.
It differs from the same Redis command just in one case: in Redis, you can't apply it to a non-existing stream; in Kvrocks you can because streams are allowed to be empty and there should be a way to migrate an empty stream (with no prior XADD command which can create a stream if it doesn't exist).
Related C++ unit tests were added.

Stream key migration was implemented. The key of the stream type is migrated via a separate function.
Related Go tests were added.

Refactor and tidy cluster-related code (src/cluster folder).

Closes #1087

@git-hulk git-hulk requested review from git-hulk, caipengbo, jbonofre, ShooterIT and tisonkun and removed request for jbonofre December 20, 2022 01:38
bool MigrateSimpleKey(const rocksdb::Slice &key, const Metadata &metadata, const std::string &bytes,
std::string *restore_cmds);
bool MigrateComplexKey(const rocksdb::Slice &key, const Metadata &metadata, std::string *restore_cmds);
bool MigrateStream(const rocksdb::Slice &key, const StreamMetadata &metadata, std::string *restore_cmds);
Copy link
Contributor

@caipengbo caipengbo Dec 21, 2022

Choose a reason for hiding this comment

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

By the way, this type of functions uses bool as its return value. I think it's a more elegant way to return Status, and move the LOG information from these internal functions to Status Msg, and return it to the upper level.

Copy link
Member

Choose a reason for hiding this comment

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

We can refactor them in further PRs : )

@PragmaTwice
Copy link
Member

Thanks all. Merging...

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.

Add support for migrating STREAM in cluster mode
4 participants