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

integration: add WatchProgressNotifyInterval in integration test #12271

Merged

Conversation

jingyih
Copy link
Contributor

@jingyih jingyih commented Sep 6, 2020

Flag WatchProgressNotifyInterval was added to etcdserver in #12216. This PR adds the corresponding flag to integration test so we can utilize it in tests.

Fixes #12265

@jingyih jingyih changed the title integration: add flag to configure watch progress notify interval in integration test integration: add WatchProgressNotifyInterval in integration test Sep 6, 2020
@jingyih
Copy link
Contributor Author

jingyih commented Sep 6, 2020

cc @wojtek-t

Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

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

Nice! lgtm
Thanks @jingyih

Copy link
Contributor

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

Thanks a lot @jingyih - just one minor comment, other than that LGTM.

if !resp.IsProgressNotify() {
t.Fatalf("expected resp.IsProgressNotify() == true")
}
case <-time.After(2 * progressInterval):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this may be a little flaky in cpu-starved environments, I suggest increasing this a little bit (to at least 1s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Thanks!

@jingyih jingyih force-pushed the add_watch_notify_interval_flag_in_testing branch from 00b6cb1 to 7fc4364 Compare September 7, 2020 15:11
@jingyih jingyih force-pushed the add_watch_notify_interval_flag_in_testing branch from 7fc4364 to 73817b5 Compare September 7, 2020 15:33
@wojtek-t
Copy link
Contributor

wojtek-t commented Sep 8, 2020

LGTM - thanks!

@wojtek-t
Copy link
Contributor

wojtek-t commented Sep 9, 2020

@gyuho @jpbetz - would you be able to merge this? IIRC it's blocking cherrypicking back to 3.4, which in turn is blocking https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1904-efficient-watch-resumption (or to be more specific: kubernetes/kubernetes#94364)

@jpbetz
Copy link
Contributor

jpbetz commented Sep 9, 2020

LGTM

@jpbetz jpbetz merged commit 10fa961 into etcd-io:master Sep 9, 2020
jpbetz added a commit that referenced this pull request Sep 10, 2020
…1-upstream-release-3.4

Automated cherry pick of #12271 on release 3.4
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.

Allow for configuring WatchProgressNotifyInterval in integration tests
4 participants