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 #11463

Conversation

jingyih
Copy link
Contributor

@jingyih jingyih commented Dec 18, 2019

Make watch progress notify configurable via flag --experimental-watch-progress-notify-interval.

@jingyih
Copy link
Contributor Author

jingyih commented Dec 18, 2019

cc @jpbetz

@codecov-io
Copy link

codecov-io commented Dec 18, 2019

Codecov Report

Merging #11463 into master will decrease coverage by 0.25%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11463      +/-   ##
==========================================
- Coverage   64.21%   63.96%   -0.26%     
==========================================
  Files         403      403              
  Lines       38079    38083       +4     
==========================================
- Hits        24453    24358      -95     
- Misses      11993    12066      +73     
- Partials     1633     1659      +26
Impacted Files Coverage Δ
etcdserver/config.go 79.51% <ø> (ø) ⬆️
embed/config.go 50.84% <ø> (ø) ⬆️
etcdserver/api/v3rpc/watch.go 76.94% <0%> (-2.8%) ⬇️
etcdmain/config.go 84% <100%> (+0.07%) ⬆️
embed/etcd.go 68.23% <100%> (+0.06%) ⬆️
client/client.go 53.26% <0%> (-30.72%) ⬇️
client/members.go 51.61% <0%> (-13.71%) ⬇️
auth/simple_token.go 65.85% <0%> (-11.39%) ⬇️
pkg/netutil/netutil.go 63.11% <0%> (-7.38%) ⬇️
clientv3/namespace/watch.go 87.87% <0%> (-6.07%) ⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5770a6d...e628ced. Read the comment docs.

@xiang90
Copy link
Contributor

xiang90 commented Dec 20, 2019

shall we make this a global flag? or a per watch configuration?

@xiang90
Copy link
Contributor

xiang90 commented Dec 20, 2019

my initial thought is to support global min value (do not overload server if user sets a very small value at per watcher level) and a per watcher value.

@jingyih
Copy link
Contributor Author

jingyih commented Dec 20, 2019

my initial thought is to support global min value (do not overload server if user sets a very small value at per watcher level) and a per watcher value.

This sounds good to me. So the global min value would be configured as a server flag, and the per watcher value is configured via an optional field in watch create request?

@xiang90
Copy link
Contributor

xiang90 commented Dec 21, 2019

This sounds good to me. So the global min value would be configured as a server flag, and the per watcher value is configured via an optional field in watch create request?

yes. we can do this in two steps though.

@stale
Copy link

stale bot commented Apr 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 6, 2020
@stale stale bot closed this Apr 27, 2020
@jingyih jingyih changed the title [Do-not-review] add experimental flag for watch notify interval *: add experimental flag for watch notify interval Aug 14, 2020
@jingyih
Copy link
Contributor Author

jingyih commented Aug 14, 2020

Reopening this PR (just rebased). It seems useful to make the watch progress notify interval configurable. For example, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1904-efficient-watch-resumption

cc @xiang90 @gyuho @wojtek-t @ptabor

@jingyih
Copy link
Contributor Author

jingyih commented Aug 14, 2020

I cannot reopen this PR... Let me create a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants