Change storeToNodeConditionLister to return []*api.Node instead of api.NodeList for performance#28781
Conversation
pkg/client/cache/listers.go
Outdated
There was a problem hiding this comment.
This is the main change in this PR (most of other change are test fixes and stuff like that).
This has significant performance impact on scheduler, but before I finish fixing tests, I would like to get your opinion on it.
In particular, I think that:
- ideally api.XxxList should all be []*api.Xxx instead of []api.Xxx (e.g. api.NodeList should be []*api.Node instead of []api.Node (plus all the metadata that we currently have))
- however, since this seems to be backward-incompatible change, we may consider changing all listers here to api like I'm changing it here.
Thoughts?
There was a problem hiding this comment.
I think I'm OK with this. It was originally this way to try and match the client interface but I think it's already diverged, so I'd say go for it.
35cea5f to
f5e2548
Compare
…i.NodeList for performance
f5e2548 to
d14fe0f
Compare
|
I just update the PR - it should be ready for review now. |
|
GCE e2e build/test passed for commit d14fe0f. |
| return true | ||
| } | ||
|
|
||
| func includeNodeFromNodeList(node *api.Node) bool { |
|
One nit. LGTM otherwise once SchedulerPredicates tests pass. |
|
SchedulerPredicates tests passed. I will apply the nit in a following PR - applying lgtm label. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit d14fe0f. |
|
Automatic merge from submit-queue |
Currently copies that are made while copying/creating api.NodeList are significant part of scheduler profile, and a bunch of them are made in places, that are not-parallelizable.
Ref #28590