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.

Sign up for GitHub

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

Jump to bottom

Fix default watch cache size to be 1000 instead of upstream's 100 #14052

Merged
merged 3 commits into from
May 23, 2017
Merged

Fix default watch cache size to be 1000 instead of upstream's 100 #14052

merged 3 commits into from
May 23, 2017

Conversation

sttts
Copy link
Contributor

@sttts sttts commented May 4, 2017 •
edited
Loading

Fixes #13942

@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch from 42b2746 to 9282202 Compare May 4, 2017 15:10
@ncdc
Copy link
Contributor

ncdc commented May 4, 2017

Is there an UPSTREAM for this?

@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch 2 times, most recently from 5c9996b to b717a66 Compare May 4, 2017 15:13
@sttts
Copy link
Contributor Author

sttts commented May 5, 2017

@ncdc will create that now.

@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch 3 times, most recently from 01d7ed5 to 0567c0d Compare May 5, 2017 20:36
@sttts sttts changed the title WIP: Turn watch cache capacity into tri-state var: default, off, value Fix default watch cache size to be 1000 instead of upstream's 100. May 5, 2017
@sttts sttts changed the title Fix default watch cache size to be 1000 instead of upstream's 100. Fix default watch cache size to be 1000 instead of upstream's 100 May 5, 2017
@sttts
Copy link
Contributor Author

sttts commented May 5, 2017

[test]

@sttts sttts assigned deads2k and ncdc May 5, 2017
@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch from 0567c0d to b8427a6 Compare May 5, 2017 20:38
@enj
Copy link
Contributor

enj commented May 7, 2017

Doesn't upstream do crazy magic to determine cache size kubernetes/kubernetes#40493?

@sttts
Copy link
Contributor Author

sttts commented May 7, 2017

@enj it does. But we don't call that code: https://github.com/kubernetes/kubernetes/blob/55042b0ba9d9452366c2091a73d0bc4d7c50bf09/pkg/registry/cachesize/cachesize.go#L74. This PR is about the default cache size for those resources not listed there in that function. We had a size of 1000, kube uses 100.

deads2k
deads2k reviewed May 8, 2017
View reviewed changes
pkg/util/restoptions/configgetter.go
capacity := requestedSize
if capacity == UseConfiguredCacheSize {
capacity = configuredCacheSize
// use the origin default cache size, not the one in registry.StorageWithCacher
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the capacity variable, since after line 130 here, it's no longer requestedSize. It's "the right size".

@deads2k
Copy link
Contributor

deads2k commented May 8, 2017

nit on variable name, then lgtm. Approving bug fix.

@deads2k deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2017
@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch from b8427a6 to a59a577 Compare May 9, 2017 05:14
@sttts
Copy link
Contributor Author

sttts commented May 9, 2017

Addressed comment.

@sttts
Copy link
Contributor Author

sttts commented May 9, 2017

[merge]

@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch 3 times, most recently from 4d530e4 to ff5e280 Compare May 9, 2017 11:33
@deads2k
Copy link
Contributor

deads2k commented May 9, 2017

[merge]

@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch 3 times, most recently from 4fdf8a4 to 9045da0 Compare May 10, 2017 10:37
@sttts
Copy link
Contributor Author

sttts commented May 10, 2017

Flake #14122

@sttts
Copy link
Contributor Author

sttts commented May 10, 2017

re[test]

@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch 3 times, most recently from 93f0d45 to f376d04 Compare May 11, 2017 13:32
@sttts sttts mentioned this pull request May 15, 2017
Closed
32 tasks
@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch from f376d04 to 68f9b97 Compare May 17, 2017 12:06
@sttts
Copy link
Contributor Author

sttts commented May 18, 2017

Flake #11561

@sttts
Copy link
Contributor Author

sttts commented May 18, 2017

re[test]

@ncdc
Copy link
Contributor

ncdc commented May 22, 2017

Is this still good to go - does it just need a merge retag?

@sttts sttts mentioned this pull request May 22, 2017
Closed
@deads2k
Copy link
Contributor

deads2k commented May 22, 2017

re[merge]

@sttts
Copy link
Contributor Author

sttts commented May 22, 2017

It has a reproducible StatefulSet e2e flake. Checking in #14280 whether it's actually a regression here or just shows up because we fixed another issue before.

@ncdc
Copy link
Contributor

ncdc commented May 23, 2017

[test]
[merge]

@sttts
Unblacklist StateFul e2e tests that should work with correct watch ca…
d0797da 
…che sizes

Leave the one disabled that depends on host volumes to work.
@sttts sttts force-pushed the sttts-tri-state-watch-cache-size branch from 68f9b97 to d0797da Compare May 23, 2017 14:07
@sttts
Copy link
Contributor Author

sttts commented May 23, 2017

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to d0797da

@openshift-bot
Copy link
Contributor

openshift-bot commented May 23, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/759/) (Base Commit: 48e5e40) (Image: devenv-rhel7_6255)

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to d0797da

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1643/) (Base Commit: 48e5e40)

@openshift-bot openshift-bot merged commit c02131d into openshift:master May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

Assignees

@ncdc ncdc

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sttts @ncdc @enj @deads2k @openshift-bot