-
Notifications
You must be signed in to change notification settings - Fork 134
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
[YUNIKORN-1842] improve updatePVCRefCounts #629
Conversation
Codecov Report
@@ Coverage Diff @@
## master #629 +/- ##
==========================================
+ Coverage 70.84% 71.07% +0.23%
==========================================
Files 50 50
Lines 8194 8177 -17
==========================================
+ Hits 5805 5812 +7
+ Misses 2186 2164 -22
+ Partials 203 201 -2
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
b18afcc
to
2ea7cb7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit - avoid calling node.Node().Name multiple times.
maxGeneration = node.Generation | ||
func (cache *SchedulerCache) updatePVCRefCounts(node *framework.NodeInfo, removeNode bool) { | ||
for k, v := range cache.pvcRefCounts { | ||
delete(v, node.Node().Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Save node.Node().Name to a variable at the top of the function so it can be re-used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it. Thank you!
Signed-off-by: Frank Yang <[email protected]>
2ea7cb7
to
3b3234e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM.
What is this PR for?
In the previous version, we call
updatePVCRefCounts
inupdateNode
,updatePod
, andremovePod
. These functions callNodeInfo.SetNode
,NodeInfo.AddPod
, orNodeInfo.RemovePod
. All theseNodeInfo
functions updateGeneration
, so I think we don't need to usepvcGeneration
to determine whether we need to updatepvcRefCounts
. We can just givenodeInfo
object toupdatePVCRefCounts
and updatepvcRefCounts
inSchedulerCache
.What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1842
How should this be tested?
Add unit tests for
updatePVCRefCounts
Screenshots (if appropriate)
Questions: