wilfred-s commented on code in PR #838:
URL: https://github.com/apache/yunikorn-core/pull/838#discussion_r1569874933
##########
pkg/scheduler/partition.go:
##########
@@ -339,26 +339,29 @@ func (pc *PartitionContext) AddApplication(app
*objects.Application) error {
return fmt.Errorf("failed to find queue %s for application %s",
queueName, appID)
}
- // add the app to the queue to set the quota on the queue if needed
- queue.AddApplication(app)
+ // set resources based on tags, but only if the queue is dynamic
(unmanaged)
+ if queue.IsManaged() {
+ log.Log(log.SchedQueue).Warn("Trying to set resources on a
queue that is not an unmanaged leaf",
+ zap.String("queueName", queue.QueuePath))
+ } else {
+ queue.SetResources(app.GetGuaranteedResource(),
app.GetMaxResource())
+ }
// check only for gang request
// - make sure the taskgroup request fits in the maximum set for the
queue hierarchy
// - task groups should only be used in FIFO queues
// if the check fails remove the app from the queue again
if placeHolder := app.GetPlaceholderAsk();
!resources.IsZero(placeHolder) {
// check the queue sorting
if !queue.SupportTaskGroup() {
- queue.RemoveApplication(app)
return fmt.Errorf("queue %s cannot run application %s
with task group request: unsupported sort type", queueName, appID)
}
- // retrieve the max set
if maxQueue := queue.GetMaxQueueSet(); maxQueue != nil {
if !maxQueue.FitInMaxUndef(placeHolder) {
- queue.RemoveApplication(app)
return fmt.Errorf("queue %s cannot fit
application %s: task group request %s larger than max queue allocation %s",
queueName, appID, placeHolder.String(), maxQueue.String())
}
}
}
+ queue.AddApplication(app)
Review Comment:
Can we move this till after the queue and callback are set on the app. That
means the app object cannot be reached by the scheduler via the queue or
partition lowering any change of lock order issues in the future.
It would also allow the removal of the locking in both `SetQueue` and
`SetTerminatedCallback`.
##########
pkg/scheduler/partition_test.go:
##########
@@ -2804,14 +2804,22 @@ func TestAddTGApplication(t *testing.T) {
partition, err := newLimitedPartition(limit)
assert.NilError(t, err, "partition create failed")
// add a app with TG that does not fit in the queue
- var tgRes *resources.Resource
- tgRes, err = resources.NewResourceFromConf(map[string]string{"vcore":
"10"})
+ tgRes, err := resources.NewResourceFromConf(map[string]string{"vcore":
"10"})
Review Comment:
NIT: this change introduces shadowing for the err variable. Not needed
##########
pkg/scheduler/partition.go:
##########
@@ -339,26 +339,29 @@ func (pc *PartitionContext) AddApplication(app
*objects.Application) error {
return fmt.Errorf("failed to find queue %s for application %s",
queueName, appID)
}
- // add the app to the queue to set the quota on the queue if needed
- queue.AddApplication(app)
+ // set resources based on tags, but only if the queue is dynamic
(unmanaged)
+ if queue.IsManaged() {
+ log.Log(log.SchedQueue).Warn("Trying to set resources on a
queue that is not an unmanaged leaf",
+ zap.String("queueName", queue.QueuePath))
+ } else {
+ queue.SetResources(app.GetGuaranteedResource(),
app.GetMaxResource())
+ }
// check only for gang request
// - make sure the taskgroup request fits in the maximum set for the
queue hierarchy
// - task groups should only be used in FIFO queues
// if the check fails remove the app from the queue again
if placeHolder := app.GetPlaceholderAsk();
!resources.IsZero(placeHolder) {
// check the queue sorting
if !queue.SupportTaskGroup() {
- queue.RemoveApplication(app)
return fmt.Errorf("queue %s cannot run application %s
with task group request: unsupported sort type", queueName, appID)
}
- // retrieve the max set
if maxQueue := queue.GetMaxQueueSet(); maxQueue != nil {
if !maxQueue.FitInMaxUndef(placeHolder) {
- queue.RemoveApplication(app)
return fmt.Errorf("queue %s cannot fit
application %s: task group request %s larger than max queue allocation %s",
queueName, appID, placeHolder.String(), maxQueue.String())
}
}
}
+ queue.AddApplication(app)
// all is OK update the app and add it to the partition
app.SetQueue(queue)
Review Comment:
This has not changed but the call to set the queue makes an unneeded call to
`Queue.UpdateApplicationPriority()` There are no asks defined on the
application yet and thus the priority will default to `configs.MinPriority`. To
be overridden again as soon as the first ask gets added. Cheaper to just wait
until the first ask gets added.
--
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]
