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

testdata: add a lagging commit test #137

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented Jan 25, 2024

This commit adds a test demonstrating the effect of delayed commit on a follower node after a network hiccup between the leader and this follower.

In the described scenario, after the moment of committing an entry on the leader, it takes HeartbeatInterval + 3/2 * RTT until the follower learns this entry is committed.

This is suboptimal, and could take HeartbeatInverval + 1/2 * RTT if the leader didn't cut the commit index at Progress.Match before sending it to the follower.

Informs #138

@pav-kv
Copy link
Contributor Author

pav-kv commented Jan 25, 2024

@nvanbenschoten @ahrtr PTAL. This is a regression test for potential future fixes for #138.

@pav-kv pav-kv force-pushed the lagging-commit-test branch from 9ef2cb9 to 53d373c Compare January 25, 2024 22:49
@pav-kv pav-kv requested a review from ahrtr January 25, 2024 23:28
This commit adds a test demonstrating the effect of delayed commit on a
follower node after a network hiccup between the leader and this
follower.

In the described scenario, after the moment of committing an entry on
the leader, it takes HeartbeatInterval + 3/2 * RTT until the follower
learns this entry is committed.

This is suboptimal, and could take HeartbeatInverval + 1/2 * RTT if the
leader didn't cut the commit index at Progress.Match before sending it
to the follower.

Signed-off-by: Pavel Kalinnikov <[email protected]>
@pav-kv pav-kv force-pushed the lagging-commit-test branch from 53d373c to 8a26275 Compare January 25, 2024 23:31
@erikgrinaker
Copy link
Contributor

This is suboptimal, and could take HeartbeatInverval + 1/2 * RTT if the leader didn't cut the commit index at Progress.Match before sending it to the follower.

Why do we care? Follower reads?

@pav-kv
Copy link
Contributor Author

pav-kv commented Jan 31, 2024

Why do we care? Follower reads?

Yes, and also the general "convergence" of the system (resulting in fewer messages etc). For other kinds of reads, like quorum reads, this also seems relevant.

@pav-kv
Copy link
Contributor Author

pav-kv commented Jan 31, 2024

@ahrtr Good to merge?

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

lgtm

@ahrtr ahrtr merged commit 2f10997 into etcd-io:main Jan 31, 2024
10 checks passed
@pav-kv pav-kv deleted the lagging-commit-test branch January 31, 2024 18:07
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.

3 participants