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

PSYNC based on Unique Replication Sequence ID #538

Merged
merged 6 commits into from
May 24, 2022

Conversation

ShooterIT
Copy link
Member

@ShooterIT ShooterIT commented Apr 22, 2022

Background

Currently, master only checks sequence number when replica asks for PSYNC, that is not enough since they may have different replication history even the replica asking sequence is in the range of the master current WAL.

Bug report in #462

PS: Master check also DB name before checking sequence number, but it is also not enough since it is setting in conf, and don't change when role is changed.

Solution

We design 'PSYNC based on Unique Replication Sequence ID', we add unique replication id for every write batch (the operation of each command on the storage engine), so the combination of replication id and sequence is unique for write batch. The master can identify whether the replica has the same replication history by checking replication id and sequence.
Like Redis, replicas can partially resynchronize with new master if old master is failed and new selected master has largest offset, and replicas can also partially resynchronize with master after restarting if the replicas' conf file has the its origin master.

Unique replication id will be changed when replicas become master.

By default, it is not enabled since this stricter check may easily lead to full synchronization. You should enable it if you want to data correctness, maybe we enable it by default in kvrocks 3.0.

@ShooterIT ShooterIT added the bug type bug label Apr 22, 2022
@ShooterIT ShooterIT requested a review from git-hulk April 22, 2022 07:24
@ShooterIT ShooterIT linked an issue Apr 22, 2022 that may be closed by this pull request
@ShooterIT ShooterIT added release notes major decision Requires project management committee consensus labels Apr 22, 2022
src/server.cc Outdated Show resolved Hide resolved
src/main.cc Outdated Show resolved Hide resolved
@git-hulk
Copy link
Member

@ShooterIT Need to add the License Header for new files

git-hulk
git-hulk previously approved these changes May 19, 2022
Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

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

LGTM

@ShooterIT ShooterIT merged commit 9ecf5c7 into apache:unstable May 24, 2022
@ShooterIT ShooterIT deleted the rsid-psync branch May 24, 2022 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug type bug major decision Requires project management committee consensus release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Incorrect data after SLAVEOF
2 participants