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

Remove nodeName from predicate signature. #24968

Merged
merged 1 commit into from
May 6, 2016
Merged

Remove nodeName from predicate signature. #24968

merged 1 commit into from
May 6, 2016
+150 −112

Conversation

wojtek-t
Copy link
Member

With this approach, I'm getting the initial throughput (in empty cluster) in 1000-node cluster of ~95pods/s.
Which is ~30% improvement.

@kubernetes/sig-scalability

@wojtek-t wojtek-t added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 29, 2016
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 29, 2016
@hongchaodeng
Copy link
Contributor

On top of my head, I can think of a call to Store.Get()/List() which might be causing contention.
Do you have any other thoughts about the improvement?
One way or another, it is great to clean up nodeName which is duplicate.

@wojtek-t
Copy link
Member Author

@hongchaodeng - it's not about contention this time. It's about unnecessary allocating ( and copying) this node name for every predicate function & for every node (which is 6000 in 1000-node cluster).

@davidopp
Copy link
Member

LGTM

Very nice! I love those rare cases where simplifying something also speeds it up!

@davidopp davidopp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 29, 2016
@davidopp
Copy link
Member

@k8s-bot e2e test this issue: #IGNORE

@wojtek-t
Copy link
Member Author

@mwielgus - FYI

@smarterclayton smarterclayton added the sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. label May 1, 2016
@wojtek-t wojtek-t force-pushed the remove_node_name branch from bc8c3b6 to 25debaf Compare May 4, 2016 08:24
@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 4, 2016
@wojtek-t wojtek-t force-pushed the remove_node_name branch from 25debaf to a51f266 Compare May 6, 2016 09:23
@wojtek-t
Copy link
Member Author

wojtek-t commented May 6, 2016

Simple rebase after #22985 - reapplying lgtm

@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 6, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented May 6, 2016

GCE e2e build/test passed for commit a51f266.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 66ef873 into kubernetes:master May 6, 2016
@wojtek-t wojtek-t deleted the remove_node_name branch May 17, 2016 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@davidopp davidopp

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wojtek-t @hongchaodeng @davidopp @k8s-github-robot @k8s-bot @smarterclayton @googlebot