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

*: add experimental flag for watch notify interval #12216

Merged

Conversation

jingyih
Copy link
Contributor

@jingyih jingyih commented Aug 14, 2020

Make watch progress notify configurable via flag --experimental-watch-progress-notify-interval. It is useful in cases such as https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1904-efficient-watch-resumption

@jingyih
Copy link
Contributor Author

jingyih commented Aug 14, 2020

cc @xiang90 @gyuho @wojtek-t @ptabor

@jingyih jingyih added the WIP label Aug 14, 2020
@jingyih jingyih changed the title [WIP] *: add experimental flag for watch notify interval *: add experimental flag for watch notify interval Aug 14, 2020
@jingyih jingyih removed the WIP label Aug 14, 2020
@wojtek-t
Copy link
Contributor

LGTM - thanks!

[FTR: we were doing both some scale and correctness tests of prototype based on https://github.com//pull/11463 (which this one seem to be just rebase of) and that all worked fine.]

@gyuho
Copy link
Contributor

gyuho commented Aug 14, 2020

@jingyih Can we enforce a minimum duration value for this field to prevent misuse?

This seems useful for testing. What's the other use case and what's the value that Kubernetes wants to use?

Also, please update the changelog.

@wojtek-t
Copy link
Contributor

This seems useful for testing. What's the other use case and what's the value that Kubernetes wants to use?

We were scale testing it in 5k-node tests. 250ms worked perfectly fine, 100ms was signifcantly visible in metrics, though it still kind-of worked.
In Kubernetes - I'm targeting to setting it to O(5s) (at least initially).

@jingyih jingyih force-pushed the experimental_flag_for_watch_notify_interval branch from f3d5ead to 9a69847 Compare August 14, 2020 19:02
@jingyih
Copy link
Contributor Author

jingyih commented Aug 14, 2020

Thanks @gyuho!

Can we enforce a minimum duration value for this field to prevent misuse?

I added 100ms minimum duration.

This seems useful for testing. What's the other use case and what's the value that Kubernetes wants to use?

Please refer to @wojtek-t 's previous comment. Also as mentioned in the PR description, this is useful to https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1904-efficient-watch-resumption

Also, please update the changelog.

Done.

@gyuho gyuho merged commit 92f9e6e into etcd-io:master Aug 14, 2020
@wojtek-t
Copy link
Contributor

Cool - great to see that merged!

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.

3 participants