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

scheduler: calculate priority in parallel. #18413

Merged
merged 1 commit into from Dec 21, 2015

Conversation

xiang90
Copy link
Contributor

@xiang90 xiang90 commented Dec 9, 2015

To improve the throughput of current scheduler, we can do
a simple optimization by calculating priorities in parallel.

This doubles the throughput of density test, which has the default
config with 3 priority funcs (the spreading one does not actually
consume any computation time). It matches the expectation.

/cc @hongchaodeng @davidopp @HaiyangDING @kevin-wangzefeng

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 9, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@HaiyangDING
Copy link

Great work. If we could have the performance test, the result would have more credit.

@kevin-wangzefeng
Copy link
Member

/cc @HardySimpson

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2015
@wojtek-t
Copy link
Member

@xiang90 - can you please rebase this PR?
I'm happy to review it once this is rebased (it seems to have significant impact on scheduler throughput).

@davidopp
Copy link
Member

Yes, please rebase.

Assigning to @wojtek-t who seems to be offering to review it. :)

@davidopp davidopp assigned wojtek-t and unassigned davidopp Dec 18, 2015
@wojtek-t
Copy link
Member

@xiang90 - can you please rebase?

@@ -170,40 +170,72 @@ func PrioritizeNodes(pod *api.Pod, podLister algorithm.PodLister, priorityConfig
return EqualPriority(pod, podLister, nodeLister)
}

var (
mu sync.Mutex
Copy link
Member

Choose a reason for hiding this comment

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

personally I perfer:

mu := sync.Mutex{}

@wojtek-t
Copy link
Member

@xiang90 - I also added some comments - please apply them when rebasing.

@xiang90
Copy link
Contributor Author

xiang90 commented Dec 18, 2015

@wojtek-t Rebased. Fixed the issues you mentioned. PTAL.

@wojtek-t
Copy link
Member

LGTM - thanks!

@wojtek-t wojtek-t added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 18, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 18, 2015

GCE e2e test build/test passed for commit 13d29440edac214d7a1b9452e89a323084b8ded2.

@wojtek-t
Copy link
Member

LGTM - thanks!

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Dec 21, 2015

GCE e2e test build/test passed for commit 7f4f754.

@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 Dec 21, 2015

GCE e2e test build/test passed for commit 7f4f754.

@wojtek-t
Copy link
Member

@k8s-bot unit test this please

@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 Dec 21, 2015

GCE e2e test build/test passed for commit 7f4f754.

@wojtek-t
Copy link
Member

@k8s-bot unit test this please

4 similar comments
@wojtek-t
Copy link
Member

@k8s-bot unit test this please

@wojtek-t
Copy link
Member

@k8s-bot unit test this please

@wojtek-t
Copy link
Member

@k8s-bot unit test this please

@wojtek-t
Copy link
Member

@k8s-bot unit test this please

@wojtek-t
Copy link
Member

@xiang90 - can you please rebase this PR - it's now constantly failing with the following error:

git checkout -f fc1808411418f9a12f7b68e523287b57aef3df3d
FATAL: Could not checkout fc1808411418f9a12f7b68e523287b57aef3df3d
hudson.plugins.git.GitException: Could not checkout fc1808411418f9a12f7b68e523287b57aef3df3d

I hope rebase will help...

@wojtek-t
Copy link
Member

@k8s-bot unit test this please

1 similar comment
@wojtek-t
Copy link
Member

@k8s-bot unit test this please

@xiang90
Copy link
Contributor Author

xiang90 commented Dec 21, 2015

@wojtek-t Do I still need to rebase? Seems to be fine?

@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 Dec 21, 2015

GCE e2e build/test failed for commit 7f4f754.

@wojtek-t
Copy link
Member

@xiang90 - no, it seems it's fixed now

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 21, 2015

GCE e2e test build/test passed for commit 7f4f754.

@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 Dec 21, 2015

GCE e2e test build/test passed for commit 7f4f754.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Dec 21, 2015
@k8s-github-robot k8s-github-robot merged commit 2975431 into kubernetes:master Dec 21, 2015
@xiang90 xiang90 deleted the p_schedule branch December 21, 2015 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet