[YUNIKORN-2883] Improve sorters & queue_tracker funtion's test coverage #971
Conversation
| @@ -410,11 +410,7 @@ func (qt *QueueTracker) canRunApp(hierarchy []string, applicationID string, trac | |||
| func (qt *QueueTracker) canBeRemoved() bool { | |||
| for _, childQT := range qt.childQueueTrackers { | |||
| // quick check to avoid further traversal | |||
| if childQT.canBeRemovedInternal() { | |||
| if !childQT.canBeRemoved() { | |||
There was a problem hiding this comment.
Isn't it meant to do it recursively?
There was a problem hiding this comment.
That place seems unreachable.
There was a problem hiding this comment.
Could you verify it by adding test case with tree that depth >= 3? I think it's for tree traversal
There was a problem hiding this comment.
Did you mean len(qt.childQueueTrackers) == 0 on L426 cause this?
There was a problem hiding this comment.
I see how this is going. This function can only remove tree that height=2.
There was a problem hiding this comment.
I've already discussion about this code with @manirajv06 please involve him as well.
There was a problem hiding this comment.
gentle ping @manirajv06 , are you available to give us some insight?
There was a problem hiding this comment.
The check works even for deeper trees. What is not supported is a recursive delete so that is what it prevents from happening.
root -> parent1 .to. parent5 -> child1 .to. child5 -> leaf1 .to. leaf5
The .to. signifies a repeat of the queues. So we have 5 parents under the root. Each with 5 child queues etc.
When we check for a removal we traverse the tree and would do a depth first check. To remove parent1 it has to be empty and have no quota set, that is the original call. Before we go all the way to the leaf at any point we check if the child queues at the current level can be removed.
If any child has a quota set, even without any usage, we cannot remove parent and would return false.
I think the check len(qt.childQueueTrackers) == 0 in canBeRemovedInternal() should not be there as well as it stops going down the tree. I think @SP12893678 and @ryankert01 made the correct remarks to start of the conversation. I think we need to fix that vunction and make it work recursively as it should.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #971 +/- ##
=========================================
Coverage ? 81.08%
=========================================
Files ? 97
Lines ? 12522
Branches ? 0
=========================================
Hits ? 10154
Misses ? 2099
Partials ? 269 ☔ View full report in Codecov by Sentry. |
|
@manirajv06 can you take a look at this? Some of this logic looks suspect to me... |
What is this PR for?
Improve the following funtion's test coverage:
What type of PR is it?
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2883