craigcondit commented on code in PR #944:
URL: https://github.com/apache/yunikorn-core/pull/944#discussion_r1725549610
##########
pkg/scheduler/objects/preemption.go:
##########
@@ -827,6 +828,18 @@ func (qps *QueuePreemptionSnapshot)
GetRemainingGuaranteedResource() *resources.
if qps.AskQueue.QueuePath == qps.QueuePath &&
!remainingGuaranteed.IsEmpty() {
return resources.MergeIfNotPresent(remainingGuaranteed,
parent)
}
+ // Queue (potential victim queue path) being processed
currently sharing common ancestors or parent with ask queue should not
propagate its
+ // actual remaining guaranteed to rest of queue's in the queue
hierarchy downwards to let them use their own remaining guaranteed only if
guaranteed
+ // has been set. Otherwise, propagating the remaining
guaranteed downwards would give wrong perception and those queues might not be
chosen
+ // as victims for sibling ( who is under guaranteed and
starving for resources) in the same level.
+ // Overall, this increases the chance of choosing victims for
preemptor from siblings without causing preemption storm or loop.
+ askQueueRemainingGuaranteed :=
qps.AskQueue.GuaranteedResource.Clone()
+ askQueueUsed := qps.AskQueue.AllocatedResource.Clone()
+ askQueueUsed = resources.SubOnlyExisting(askQueueUsed,
qps.AskQueue.PreemptingResource)
+ askQueueRemainingGuaranteed =
resources.SubOnlyExisting(askQueueRemainingGuaranteed, askQueueUsed)
Review Comment:
The Clone() calls are unnecessary here, as SubOnlyExisting() returns a copy
already.
##########
pkg/scheduler/objects/preemption_queue_test.go:
##########
@@ -24,6 +24,35 @@ import (
"github.com/apache/yunikorn-core/pkg/common/resources"
)
+var smallestRes =
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 5})
+var childRes =
resources.NewResourceFromMap(map[string]resources.Quantity{"second": 5})
+var smallestResPlusChildRes = resources.Add(smallestRes, childRes)
+var smallestResDouble = resources.Multiply(smallestRes, 2)
+var smallestResDoublePlusChildRes =
resources.Add(resources.Multiply(smallestRes, 2), childRes)
+var smallestResMultiplyByZero =
resources.NewResourceFromMap(map[string]resources.Quantity{"first": 0})
+var smallestResMultiplyByMinusOne = resources.Multiply(smallestRes, -1)
+var smallestResMultiplyByMinusOnePlusChildRes =
resources.Add(resources.Multiply(smallestRes, -1), childRes)
+var childResMultiplyByZero =
resources.NewResourceFromMap(map[string]resources.Quantity{"second": 0})
+var childResMultiplyByMinusOne = resources.Multiply(childRes, -1)
+var childResDouble = resources.Multiply(childRes, 2)
+var smallestResDoublePlusChildResDouble =
resources.Add(resources.Multiply(smallestRes, 2), childResDouble)
+var smallestResPlusChildResDouble = resources.Add(smallestRes, childResDouble)
+var smallestResMultiplyByZeroPluschildResMultiplyByZero =
resources.Add(smallestResMultiplyByZero, childResMultiplyByZero)
+var smallestResMultiplyByZeroPluschildResMultiplyByMinusOne =
resources.Add(smallestResMultiplyByZero, childResMultiplyByMinusOne)
Review Comment:
Agree with this, please simplify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
