[YUNIKORN-2638] Simplify finalizeNodes and finalizePods #949
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #949 +/- ##
==========================================
+ Coverage 68.02% 68.04% +0.02%
==========================================
Files 70 70
Lines 9195 9195
==========================================
+ Hits 6255 6257 +2
+ Misses 2733 2731 -2
Partials 207 207 ☔ View full report in Codecov by Sentry. |
| @@ -1621,9 +1621,9 @@ func (ctx *Context) finalizeNodes(existingNodes []*v1.Node) error { | |||
| } | |||
|
|
|||
| // convert the node list into a map | |||
| nodeMap := make(map[string]*v1.Node) | |||
| nodeMap := make(map[string]struct{}, len(nodes)) | |||
There was a problem hiding this comment.
From a readability perspective I would prefer nodeMap := make(map[string]bool, len(nodes)) and set true in 1626
Then at line 1631 the check simply becomes if !nodeMap[node.Name] {
Size (~5% more) and speed wise (not measurable unless the map has millions of entries) it does not make a difference when you consider what we use it for.
There was a problem hiding this comment.
From a readability perspective I would prefer nodeMap := make(map[string]bool, len(nodes)) and set true in 1626
I'm +1 on @wilfred-s's suggestion. I prefer readability over small performance enhancements too. @steinsgateted Could you make change for it?
Same here, I also vote for |
What is this PR for?
Change
map[types.UID]*v1.Pod/map[string]*v1.Nodetomap[types.UID]struct{}.Reduce the memory footprint of map, because struct{} is zero-size and does not occupy any additional space.
Fixed map length to avoid reallocation of memory
What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2638
How should this be tested?
Screenshots (if appropriate)
Questions: