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

ZOOKEEPER-3023: Sync and commit diff log entries before NEWLEADER ack #1848

Conversation

kezhuw
Copy link
Member

@kezhuw kezhuw commented Apr 2, 2022

ZOOKEEPER-2678 could skip snapshot in diff sync, but diff txns are
logged and committed after NEWLEADER ack. ZOOKEEPER-3911 moves txn
logging before NEWLEADER ack, but the txn logging is asynchronous. So it
is indeterminate whether diff txns have been persisted to disk or not
after NEWLEADER ack.

This commit try to sync and commit txn logs synchronously before ack to
NEWLEADER thus provides strong guarantee that follower is in sync with
leader after NEWLEADER ack received.

This behavior is consistent with pre ZOOKEEPER-2678 and easy to test.

@kezhuw kezhuw force-pushed the ZOOKEEPER-3023-sync-follower-before-ack-newleader branch from 50af53f to 838c941 Compare April 2, 2022 14:29
@kezhuw
Copy link
Member Author

kezhuw commented Apr 5, 2022

@eolivelli @hanm @nkalmar @afine @fpj Could you please take a look at this pr ? I think it is related to #157 and #1445.

@kezhuw kezhuw force-pushed the ZOOKEEPER-3023-sync-follower-before-ack-newleader branch from 838c941 to f4b0b76 Compare June 28, 2022 14:26
ZOOKEEPER-2678 could skip snapshot in diff sync, but diff txns are
logged and committed after NEWLEADER ack. ZOOKEEPER-3911 moves txn
logging before NEWLEADER ack, but the txn logging is asynchronous. So it
is indeterminate whether diff txns have been persisted to disk or not
after NEWLEADER ack.

This commit try to sync and commit txn logs synchronously before ack to
NEWLEADER thus provides strong guarantee that follower is in sync with
leader after NEWLEADER ack received.

This behavior is consistent with pre ZOOKEEPER-2678 and easy to test.
@kezhuw
Copy link
Member Author

kezhuw commented May 4, 2023

I have updated this pr with a fixup commit to remove #1445's effect, but I still leave room for ZOOKEEPER-4643. See #1925 (comment).

@AlphaCanisMajoris
Copy link
Contributor

AlphaCanisMajoris commented Mar 18, 2024

Hi .

I use the TLA+ specifications to verify the correctness of the fix in this pr. It can avoid the issues of ZOOKEEPER-3023, ZOOKEEPER-4394, ZOOKEEPER-4646 and ZOOKEEPER-4685.

Besides, I really like this fix. It is really simple and easy to understand.

@kezhuw
Copy link
Member Author

kezhuw commented Mar 18, 2024

This should be superceded by #2111. Both try to log diff entries synchronous before NEWLEADER ack and #2028 is similar. Additionally, #2111 should fix ZOOKEEPER-4643 also.

I am not sure testNormalFollowerRunWithDiff is still flaky or not. I will check it later. No sure it is worth to revert it to pre ZOOKEEPER-2678 given my comments in ZOOKEEPER-3023.

@AlphaCanisMajoris
Copy link
Contributor

This should be superceded by #2111. Both try to log diff entries synchronous before NEWLEADER ack and #2028 is similar. Additionally, #2111 should fix ZOOKEEPER-4643 also.

That's cool. The code fix of #2111 is basically the same as that of #2028. It should fix ZOOKEEPER-4643, ZOOKEEPER-4646 and ZOOKEEPER-4685.

I am not sure testNormalFollowerRunWithDiff is still flaky or not. I will check it later. No sure it is worth to revert it to pre ZOOKEEPER-2678 given my comments in ZOOKEEPER-3023.

In my understanding, #2111 cannot avoid the issue ZOOKEEPER-4394, right? Not all the pending requests need to be logged before the follower replies ACK of NEWLEADER.

@kezhuw
Copy link
Member Author

kezhuw commented May 21, 2024

Superceded by #2111 and #2152.

@kezhuw kezhuw closed this May 21, 2024
@kezhuw kezhuw deleted the ZOOKEEPER-3023-sync-follower-before-ack-newleader branch October 14, 2024 02:38
@kezhuw kezhuw restored the ZOOKEEPER-3023-sync-follower-before-ack-newleader branch October 14, 2024 02:39
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.

2 participants