Skip to content

rebuild replication backlog index when master restart #9720

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.

Sign up for GitHub

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

Jump to bottom
Merged
oranagra merged 1 commit into from
Nov 2, 2021
Merged

rebuild replication backlog index when master restart #9720

oranagra merged 1 commit into from
Nov 2, 2021

Conversation

@soloestoy
Copy link
Contributor

@soloestoy soloestoy commented Nov 2, 2021 •
edited by oranagra
Loading

After PR #9166 , replication backlog is not a real block of memory, just contains a reference points to replication buffer's block and the blocks index (to accelerate search offset when partial sync), so we need update both replication buffer's block's offset and replication backlog blocks index's offset when master restart from RDB, since the server.master_repl_offset is changed.
The implications of this bug was just a slow search, but not a replication failure.

@oranagra
Copy link
Member

oranagra commented Nov 2, 2021

@soloestoy can you please add some details in the top comment.

@oranagra
Copy link
Member

oranagra commented Nov 2, 2021

ok, so we forgot to re-build the rax in loadDataFromDisk, don't we have a test that should have failed?
i.e. one that attempts to psync from a restarted master?

@ShooterIT
Copy link
Member

ShooterIT commented Nov 2, 2021 •
edited
Loading

Thanks @soloestoy
I think we don't index replication buffer blocks which are generated during loading, this makes it slow to search offset when partial sync if there are many expire keys, we need to search from the first node since there is no index.

we already updated replication buffer blocks' offset, so tests are passed.

@oranagra
Copy link
Member

oranagra commented Nov 2, 2021

ohh, so the implications of this bug was just a slow search, but not a replication failure?
@soloestoy mind telling us how did you find it?

@soloestoy
Copy link
Contributor Author

soloestoy commented Nov 2, 2021

ohh, so the implications of this bug was just a slow search, but not a replication failure?

Yes, I think it doesn't lead to replication failure or data inconsistency.

@soloestoy mind telling us how did you find it?

By reading the code... I missed some PRs code review, and recently have time to review them.

@oranagra oranagra merged commit d08f055 into redis:unstable Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

@ShooterIT ShooterIT ShooterIT approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Redis 7.0
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@soloestoy @oranagra @ShooterIT