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

Fix missing stream increment batch in cluster migration #1345

Merged
merged 5 commits into from
Mar 24, 2023
Merged

Fix missing stream increment batch in cluster migration #1345

merged 5 commits into from
Mar 24, 2023

Conversation

git-hulk
Copy link
Member

Currently, the cluster slot migration will split into two stages: send snapshots and sync WAL if the snapshot was sent. For the stream, we only parsed the KVs from the snapshot and missed the WAL part.

@git-hulk git-hulk requested review from torwig, PragmaTwice and ShooterIT and removed request for torwig March 22, 2023 16:23
@git-hulk
Copy link
Member Author

@torwig I'm not quite familiar with the XSETID command, so I didn't handle it now.

@torwig
Copy link
Contributor

torwig commented Mar 22, 2023

@git-hulk Thanks for your work. SETID is intended for things like replication (I can say it is an internal command) to set the values of last_added_id, entries_added, max_deleted_id. I used it as the last command in slot migration to update the stream's metadata, because with the XADD you can't migrate last_added_id, entries_added and max_deleted_id, because it actually just adds entries but not deletes them.

@git-hulk
Copy link
Member Author

@torwig Thank you. So for the XSETID command, I should parse the last_added_id / entries_added / max_deleted_id, then translate it into the XSETID. Is it right?

@ShooterIT
Copy link
Member

BTW, do we implement #577 (comment)?

@git-hulk
Copy link
Member Author

@ShooterIT Yes, it's finished but missed the WAL's increment sync.

@torwig
Copy link
Contributor

torwig commented Mar 23, 2023

@git-hulk Yes, absolutely.

@PragmaTwice PragmaTwice added bug type bug A-cluster area cluster labels Mar 23, 2023
git-hulk and others added 3 commits March 24, 2023 00:23
migration

Currently, the cluster slot migration will split into two stages: send
snapshots and sync WAL if the snapshot was sent. For the stream, we only
parsed the KVs from snapshot and missing the WAL part.
Co-authored-by: Twice <twice@apache.org>
@git-hulk git-hulk requested a review from PragmaTwice March 23, 2023 16:34
torwig
torwig previously approved these changes Mar 23, 2023
PragmaTwice
PragmaTwice previously approved these changes Mar 24, 2023
@git-hulk
Copy link
Member Author

Thanks all, merging..

@git-hulk git-hulk merged commit 76a80b5 into apache:unstable Mar 24, 2023
enjoy-binbin added a commit to enjoy-binbin/kvrocks that referenced this pull request Mar 24, 2023
There is an warning in the code:
```
/incubator-kvrocks/src/storage/batch_extractor.cc:89:50: warning: enum constant in boolean context [-Wint-in-bool-context]
   89 |       StreamMetadata stream_metadata(kRedisStream);
```

This was introduced in apache#1345
git-hulk pushed a commit that referenced this pull request Mar 24, 2023
There is a warning in the code:
```
/incubator-kvrocks/src/storage/batch_extractor.cc:89:50: warning: enum constant in boolean context [-Wint-in-bool-context]
   89 |       StreamMetadata stream_metadata(kRedisStream);
```

This was introduced in #1345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cluster area cluster bug type bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants