-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Ensure the right leader is picked in watcher results for observe. #18169
base: main
Are you sure you want to change the base?
Conversation
Hi @jcferretti. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Hi @jcferretti, thanks for your contribution. Could you please sign your commit? So the developer certificate of origin (DCO) check passes, i.e:
Thanks! Ref: https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#commit-your-change |
…aign and observe impls. Signed-off-by: Cristian Ferretti <jcferretti2020@gmail.com>
ced6138
to
7e419db
Compare
Done, thank you, sorry about missing that. |
Please add a test to reproduce the issue which you are trying to fix firstly. |
Can you please comment on the issue report regarding the analysis there? Thanks. |
hdr, kv = &wr.Header, ev.Kv | ||
// may have multiple revs; hdr.rev = the last rev | ||
// set to kv's rev in case batch has multiple Puts | ||
hdr.Revision = kv.ModRevision | ||
break | ||
if kv == nil || ev.Kv.CreateRevision < kv.CreateRevision { | ||
hdr, kv = &wr.Header, ev.Kv | ||
// may have multiple revs; hdr.rev = the last rev | ||
// set to kv's rev in case batch has multiple Puts | ||
hdr.Revision = kv.ModRevision | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see any issue here.
-
The
for
condition already guarantees that thekv == nil
,
etcd/client/v3/concurrency/election.go
Line 191 in debc8fb
for kv == nil { -
Also please read https://etcd.io/docs/v3.5/learning/api_guarantees/#watch-apis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue as I see it is not kv being null. The issue as I see it is the possibility that the watch returns more than one PUT, and the current code ignores any other PUT aside from the first.
Can you please make explicit what specific watch guarantee ensures we can't get more than one PUT?
Please also see #18163 (comment)
Thanks for your time.
Fixes #18163