[YUNIKORN-2042] REST API for specific queue #687
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #687 +/- ##
==========================================
+ Coverage 79.22% 79.23% +0.01%
==========================================
Files 82 82
Lines 11346 11372 +26
==========================================
+ Hits 8989 9011 +22
- Misses 2037 2039 +2
- Partials 320 322 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I was expecting an API that would return a specific queue with its details. In this case we return a subtree rooted at a certain point. I would have expected just the one queue to be returned as a lightweight object without the whole subtree attached. There is no difference for a leaf queue but further up the hierarchy there is.
There was a problem hiding this comment.
@steinsgateted nice patch. there are some minor comments, and please take a look. thanks!
| for _, child := range childes { | ||
| queueInfo.ChildrenNames = append(queueInfo.ChildrenNames, child.Name) | ||
| } | ||
| // we have held the read lock so following method should not take lock again. |
There was a problem hiding this comment.
those code is almost equal to GetPartitionQueueDAOInfo. Could you do a bit refactor for them?
pkg/scheduler/objects/queue.go
Outdated
| func (sq *Queue) GetSingleQueueDAOInfo() dao.PartitionQueueDAOInfo { | ||
| queueInfo := dao.PartitionQueueDAOInfo{} | ||
| childes := sq.GetCopyOfChildren() | ||
| queueInfo.ChildrenNames = make([]string, 0, len(childes)) |
There was a problem hiding this comment.
maybe GetPartitionQueueDAOInfo could fill up this field also?
|
@wilfred-s @chia7712
A few changes:
Also, I would like some clarifications. |
pkg/scheduler/objects/queue.go
Outdated
| children := sq.GetCopyOfChildren() | ||
| queueInfo.Children = make([]dao.PartitionQueueDAOInfo, 0, len(children)) | ||
| if exclude { | ||
| for _, child := range children { | ||
| queueInfo.ChildrenNames = append(queueInfo.ChildrenNames, child.QueuePath) | ||
| } | ||
| } else { | ||
| for _, child := range children { | ||
| queueInfo.Children = append(queueInfo.Children, child.GetPartitionQueueDAOInfo(false)) | ||
| } |
There was a problem hiding this comment.
refactor: only need to get a copy of children if we need to recurse into them:
if !exclude {
children := sq.GetCopyOfChildren()
queueInfo.Children = make([]dao.PartitionQueueDAOInfo, 0, len(children))
for _, child := range children {
queueInfo.Children = append(queueInfo.Children, child.GetPartitionQueueDAOInfo(false))
}
}
Leave adding the child path names to the locked code below
There was a problem hiding this comment.
Add this:
for _, child := range children {
queueInfo.ChildrenNames = append(queueInfo.ChildrenNames, child.QueuePath)
}
around properties for loop line 660
pkg/webservice/handlers.go
Outdated
| // The default is to exclude children | ||
| queueDao := queue.GetPartitionQueueDAOInfo(true) | ||
| // exclude children | ||
| if exclude := strings.ToLower(r.URL.Query().Get("exclude")); exclude != "" { | ||
| if exclude != "children" { | ||
| buildJSONErrorResponse(w, "Only following exclude is allowed: children", http.StatusBadRequest) | ||
| return | ||
| } | ||
| } | ||
| // include children | ||
| if include := strings.ToLower(r.URL.Query().Get("include")); include != "" { | ||
| if include == "children" { | ||
| queueDao = queue.GetPartitionQueueDAOInfo(false) | ||
| } else { | ||
| buildJSONErrorResponse(w, "Only following include is allowed: children", http.StatusBadRequest) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
A simple subtree query parameter is a much more elegant solution.
If the parameter is there we call GetPartitionQueueDAOInfo(true) otherwise GetPartitionQueueDAOInfo(false) The default, and not present, is false so code wise it would look a bit like this:
queueDao := queue.GetPartitionQueueDAOInfo(r.URL.Query().Has("subtree"))
It checks for the presence of the key subtree
|
@wilfred-s Thank you for your review. I modified. |
Expose a REST API for specific queue: /ws/v1/partition/%s/queue/%s /ws/v1/partition/%s/queue/%s?aubtree The call takes one query parameter "subtree" if provided the whole tree of queues rooted at the level requested in the call will be returned. If "subtree" is not set only the queue requested will be returned Closes: #687 Signed-off-by: Wilfred Spiegelenburg <[email protected]> (cherry picked from commit e9805c9)
What is this PR for?
Expose a REST API for specific queue:
/ws/v1/partition/%s/queue/%s/Other modifications:
1
Modify the test items, which were not actually tested originally.
pkg/scheduler/objects/queue_test.goTestGetPartitionQueueDAOInfofunctionWhat type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/projects/YUNIKORN/issues/YUNIKORN-2042
How should this be tested?
example:http://localhost:9080/ws/v1/partition/default/queue/root.namespaces.level1.level2
Screenshots (if appropriate)
Questions: