chia7712 commented on code in PR #838:
URL: https://github.com/apache/yunikorn-core/pull/838#discussion_r1563647058
##########
pkg/scheduler/objects/application.go:
##########
@@ -186,11 +189,57 @@ func NewApplication(siApp *si.AddApplicationRequest, ugi
security.UserGroup, eve
app.user = ugi
app.rmEventHandler = eventHandler
app.rmID = rmID
+ app.setResourcesFromTags()
app.appEvents = newApplicationEvents(app, events.GetEventSystem())
app.appEvents.sendNewApplicationEvent()
return app
}
+func (sa *Application) setResourcesFromTags() {
+ quota := sa.GetTag(siCommon.AppTagNamespaceResourceQuota)
+ // get the tag with the guaranteed resource
+ guaranteed := sa.GetTag(siCommon.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
+ return
+ }
+
+ var quotaRes, guaranteedRes *resources.Resource
Review Comment:
maybe we can use short declaration?
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1281,6 +1238,37 @@ func (sq *Queue) internalGetMax(parentLimit
*resources.Resource) *resources.Reso
return resources.ComponentWiseMin(parentLimit, sq.maxResource)
}
+// GetProposedMaxQueueResource calculates the maximum resource for the queue
based on a proposed new value.
+// This is similar to GetMaxQueueSet but uses a value that has not been
applied as a maxResource for the queue.
+func (sq *Queue) GetProposedMaxQueueResource(maxRes *resources.Resource)
*resources.Resource {
+ // get the limit for the parent first and check against the queue's own
+ var limit *resources.Resource
Review Comment:
maybe we don't need to declare `limit`.
`sq.internalGetProposedMax(sq.parent.GetMaxQueueSet(), maxRes)`
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1281,6 +1238,37 @@ func (sq *Queue) internalGetMax(parentLimit
*resources.Resource) *resources.Reso
return resources.ComponentWiseMin(parentLimit, sq.maxResource)
}
+// GetProposedMaxQueueResource calculates the maximum resource for the queue
based on a proposed new value.
+// This is similar to GetMaxQueueSet but uses a value that has not been
applied as a maxResource for the queue.
+func (sq *Queue) GetProposedMaxQueueResource(maxRes *resources.Resource)
*resources.Resource {
+ // get the limit for the parent first and check against the queue's own
+ var limit *resources.Resource
+ if sq.parent == nil {
+ return nil
+ }
+ limit = sq.parent.GetMaxQueueSet()
+ return sq.internalGetProposedMax(limit, maxRes)
+}
+
+// internalGetProposedMax does the real max calculation.
+func (sq *Queue) internalGetProposedMax(parentLimit, maxRes
*resources.Resource) *resources.Resource {
+ sq.RLock()
Review Comment:
the resource from `GetMaxQueueSet` is a new object, and so it seems we don't
need to hold the lock here.
##########
pkg/scheduler/objects/application.go:
##########
@@ -186,11 +189,57 @@ func NewApplication(siApp *si.AddApplicationRequest, ugi
security.UserGroup, eve
app.user = ugi
app.rmEventHandler = eventHandler
app.rmID = rmID
+ app.setResourcesFromTags()
Review Comment:
Personally, I prefer to use function in the initialization. for example:
```go
app.maxResource =
quotaFromTags(app.GetTag(siCommon.AppTagNamespaceResourceQuota))
func quotaFromTags(quota string) *resources.Resource {
if quota == "" {
return nil
}
// need to set a quota: convert json string to resource
quotaRes, quotaErr := resources.NewResourceFromString(quota)
if quotaErr != nil {
log.Log(log.SchedQueue).Warn("application resource quota
conversion failure",
zap.String("json quota string", quota),
zap.Error(quotaErr))
return nil
} else if !resources.StrictlyGreaterThanZero(quotaRes) {
log.Log(log.SchedQueue).Warn("Max resource quantities should be
greater than zero: cannot set queue max resource",
zap.Stringer("maxResource", quotaRes))
return nil // Skip setting quota if it has a value <= 0
}
return quotaRes
}
```
it includes following benefits.
1. function is more easy to test. we don't need to create complete structure
2. it does not require lock of structure
##########
pkg/scheduler/partition.go:
##########
@@ -339,26 +339,28 @@ 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)
// 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 {
+ // the new maxResource is going to be either
app.GetMaxResource() from the tags or the maxResource which already set on the
queue
+ maxRes := queue.GetMaxResource()
+ if app.GetMaxResource() != nil {
Review Comment:
`app.GetMaxResource()` do the deep clone, and so we can reverse the
assignment to avoid duplicate clone.
```go
maxRes := app.GetMaxResource()
if maxRes == nil {
maxRes = queue.GetMaxResource()
}
```
--
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]
