Fix progress notification for watch that doesn't get any events #17557
Conversation
|
@serathius: GitHub didn't allow me to request PR reviews from the following users: scpmw. Note that only etcd-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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/test-infra repository. |
|
/cc @wojtek-t @p0lyn0mial |
|
@serathius: GitHub didn't allow me to request PR reviews from the following users: wojtek-t, p0lyn0mial. Note that only etcd-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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/test-infra repository. |
serathius
force-pushed
the
progressrequest-new-watch
branch
from
7bd31d2
to
d2b7790
Compare
|
/retest |
|
ping @ahrtr |
serathius
force-pushed
the
progressrequest-new-watch
branch
from
d2b7790
to
cb0328c
Compare
|
/cc @wojtek-t @p0lyn0mial Thanks @serathius for putting together this PR! |
|
@p0lyn0mial: GitHub didn't allow me to request PR reviews from the following users: p0lyn0mial, wojtek-t. Note that only etcd-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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/test-infra repository. |
When implementing the fix for progress notifications (etcd-io#15237) we made a incorrect assumption that that unsynched watches will always get at least one event. Unsynched watches include not only slow watchers, but also newly created watches that requested current or older revision. In case that non of the events match watch filter, those newly created watches might become synched without any event going through. Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
serathius
force-pushed
the
progressrequest-new-watch
branch
from
cb0328c
to
6103504
Compare
| sws.deferredProgress = true | ||
| } | ||
| } | ||
| sws.watchStream.RequestProgressAll() |
There was a problem hiding this comment.
nit: print a debug log when it fails to send out the progress notification.
There was a problem hiding this comment.
Let me address it in a separate PR, I think more places in watch code need debug logging.
|
So the logic here is that because the server failed to answer the progress notification request on newly created watches, we now also don't send it in other scenarios where it is unsynced? If so it should really be documented, because this effectively changes the semantics of Which - as #15237 - is okay with me, as I had to implement the re-try workaround anyway. But this might be confusing to others. |
When implementing the fix for progress notifications (#15237) we made a incorrect assumption that that unsynched watches will always get at least one event.
Unsynched watches include not only slow watchers, but also newly created watches that requested old revision. In case that non of the events match watch filter, those newly created watches might become synched without any event going through.
Fixes #17507
/cc @scpmw @ahrtr