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

etcdserver: resend ReadIndex request on empty apply request #12795

Merged

Conversation

wpedrak
Copy link
Contributor

@wpedrak wpedrak commented Mar 23, 2021

Empty apply indicates first commit in current term. It is first time when follower is sure, that its ReadIndex request can be processed.

Idea for this PR was born in discussion under #12762 (comment).

server/etcdserver/server.go Outdated Show resolved Hide resolved
server/etcdserver/server.go Show resolved Hide resolved
server/etcdserver/v3_server.go Show resolved Hide resolved
@wpedrak wpedrak force-pushed the resend-read-index-on-first-commit-in-term branch from 06a627a to 1d96146 Compare March 24, 2021 14:52
@@ -2067,6 +2081,8 @@ func (s *EtcdServer) applyEntryNormal(e *raftpb.Entry) {
// raft state machine may generate noop entry when leader confirmation.
// skip it in advance to avoid some potential bug in the future
if len(e.Data) == 0 {
s.notifyAboutFirstCommitInTerm()

select {
case s.forceVersionC <- struct{}{}:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add at least a TODO to replace forceVersionC with FirstCommitInTermNotify as they do exactly the same thing.

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

FTR:
Please add an integration test whether the notification really happens during leadership change.

@wpedrak wpedrak force-pushed the resend-read-index-on-first-commit-in-term branch from 1d96146 to 58ceed4 Compare March 31, 2021 09:42
@wpedrak
Copy link
Contributor Author

wpedrak commented Mar 31, 2021

Test I've added was flaky once I've set cluster size to 21. It was failing with got error during leadership transfer once per ~10 invocations. I didn't saw any failed tests with clusterSize := 3 (run it over 300 times). I suspect some bug in underlying functions (or I simply don't understand some edge case). Would highly appreciate any feedback on it.

tests/integration/v3_leadership_test.go Outdated Show resolved Hide resolved
tests/integration/v3_leadership_test.go Outdated Show resolved Hide resolved
Empty apply indicates first commit in current term. It is first time when follower is sure, that it's ReadIndex request can be processed.
@wpedrak wpedrak force-pushed the resend-read-index-on-first-commit-in-term branch from 58ceed4 to 943bcc3 Compare April 9, 2021 09:31

err = group.Wait()
if err != nil {
t.Errorf("korona %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops :D

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

Thank you.
LGTM to me. Just a single comment to fix.

@wpedrak wpedrak force-pushed the resend-read-index-on-first-commit-in-term branch from 943bcc3 to 08ea9cb Compare April 9, 2021 14:08
@ptabor ptabor merged commit 7f97dfd into etcd-io:master Apr 9, 2021
@wpedrak wpedrak deleted the resend-read-index-on-first-commit-in-term branch April 12, 2021 08:13
@serathius serathius mentioned this pull request Jul 21, 2022
25 tasks
ahrtr added a commit to ahrtr/etcd that referenced this pull request Jul 25, 2022
Backport etcd-io#12795 to 3.4

Signed-off-by: Benjamin Wang <wachao@vmware.com>
tjungblu pushed a commit to tjungblu/etcd that referenced this pull request Sep 8, 2022
Backport etcd-io#12795 to 3.4

Signed-off-by: Benjamin Wang <wachao@vmware.com>
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.

2 participants