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

applyV2 should apply on backend only once #13000

Merged
merged 1 commit into from
May 19, 2021

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented May 18, 2021

During review of: #12988 spotted that PUT is actially writing to v3-backend.
If we are replaying WAL log, it might happened that backend's applied_index is > than the WAL's log entry. In such situation we should skip applying on backend V3.
I think both the methods (setVersion, setMembersAttributes) are in practice idempotent so its not that 'serious' problem, but for formal correctness adding the proper checks.

@ptabor ptabor requested a review from gyuho May 18, 2021 18:08
@ptabor ptabor changed the title applyV2 should reapply on backend only once applyV2 should apply on backend only once May 18, 2021
During review of:  etcd-io#12988 spotted
that PUT is actially writing to v3-backend.
If we are replaying WAL log, it might happened that backend's
applied_index is > than the WAL's log entry. In such situation we should
skip applying on backend V3.
I think both the methods (setVersion, setMembersAttributes) are in
practice idempotent so its not that 'serious' problem, but for
formal correctness adding the proper checks.
Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the follow up!

@ptabor ptabor merged commit 788bc53 into etcd-io:main May 19, 2021
@ptabor ptabor deleted the 20210519-shouldApplyV3 branch May 19, 2021 00:09
@chaochn47
Copy link
Member

Thanks for fixing it. Without shouldApplyV3 It broke the original consistency check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants