[YUNIKORN-2724] Improve the signature of methods notifyTaskComplete() and ensureAppAndTaskCreated() #873
Conversation
… and ensureAppAndTaskCreated()
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #873 +/- ##
==========================================
- Coverage 68.08% 68.07% -0.01%
==========================================
Files 70 70
Lines 7577 7575 -2
==========================================
- Hits 5159 5157 -2
Misses 2203 2203
Partials 215 215 ☔ View full report in Codecov by Sentry. |
|
@ryankert01 please rebase the PR with the latest master, thx. |
|
Nevermind, I updated the PR with master, please check if it's OK. |
brandboat
changed the title
|
gentle ping @ryankert01, could you please fix the conflicts ? Thanks. |
|
Just finished the PR, ready for review now! |
pkg/cache/context.go
Outdated
| } | ||
| return | ||
| } | ||
| log.Log(log.ShimContext).Debug("notifyTaskComplete and release allocation", |
There was a problem hiding this comment.
Maybe we can keep origin message? release allocation
There was a problem hiding this comment.
I change the message to "notifyTaskComplete and release allocation" because I combine 2 logs to make it only log once. Perhaps I can change it to " release allocation in notifyTaskComplete"?
There was a problem hiding this comment.
Got it. It is fine to keep current message :)
What is this PR for?
From the review #864
Change notifyTaskComplete(string, string) to notifyTaskComplete(*Application, string). It removes a number of extra getApplication() calls we really do not need.
Similar for ensureAppAndTaskCreated() which is only ever called from this function. Add a parameter to it to make it: ensureAppAndTaskCreated(*v1.Pod, *Application) and only execute application creation if app == nil.
What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2724
How should this be tested?
github ci
Screenshots (if appropriate)
Questions: