YUNIKORN-2175 Add queue headRoom for Rest API querying and improve logs #727
Conversation
zhuqi-lucas
requested review from
pbacsko,
wilfred-s,
FrankYang0529 and
manirajv06
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #727 +/- ##
==========================================
- Coverage 77.87% 77.82% -0.05%
==========================================
Files 80 80
Lines 13344 13400 +56
==========================================
+ Hits 10391 10429 +38
- Misses 2630 2647 +17
- Partials 323 324 +1 ☔ View full report in Codecov by Sentry. |
zhuqi-lucas
changed the title
|
Thank you @manirajv06 for review, add more logs improvement, and also added test cases! |
| @@ -33,6 +33,7 @@ type PartitionQueueDAOInfo struct { | |||
| GuaranteedResource map[string]int64 `json:"guaranteedResource,omitempty"` | |||
| AllocatedResource map[string]int64 `json:"allocatedResource,omitempty"` | |||
| PreemptingResource map[string]int64 `json:"preemptingResource,omitempty"` | |||
| HeadRoom map[string]int64 `json:"headroom,omitempty"` | |||
There was a problem hiding this comment.
It's not tested whether this value appears properly after a web service call. It's not just this field, but quite a few inside PartitionQueueDAOInfo. There's a test case TestGetPartitionQueuesHandler which should be improved to see if values from the schedulerContext are mapped back properly. This can be done in a follow-up JIRA.
There was a problem hiding this comment.
Created https://issues.apache.org/jira/browse/YUNIKORN-2179 so that we can proceed with this one.
There was a problem hiding this comment.
Good catch @pbacsko !
| if !evt.eventSystem.IsEventTrackingEnabled() || !evt.limiter.Allow() { | ||
| return | ||
| } | ||
| message := fmt.Sprintf("Application %s does not fit into %s queue", request.GetApplicationID(), evt.app.queuePath) | ||
| message := fmt.Sprintf("Application %s does not fit into %s queue, request resoure %s can't fit headroom %s", request.GetApplicationID(), evt.app.queuePath, request.GetAllocatedResource().String(), headroom.String()) |
There was a problem hiding this comment.
Minor nit no.1: calling String() is not needed, fmt.Sprintf() can recognize the Stringer interface.
Minor nit no.2: slight rewording: "Application %s does not fit into %s queue (request resoure %s, headroom %s)". The proposed message sounds like two separate problems.
There was a problem hiding this comment.
Thanks, addressed in latest PR.
What is this PR for?
We should support query headroom using rest API.
And from the logs, i can't find any info about the headroom value, when application request can't meet the headroom.
We need to add more logs about this case!
What type of PR is it?
Todos
What is the Jira issue?
[YUNIKORN-2] Gang scheduling interface parametersHow should this be tested?
Screenshots (if appropriate)
Questions: