wilfred-s commented on code in PR #927:
URL: https://github.com/apache/yunikorn-core/pull/927#discussion_r1705117355
##########
pkg/scheduler/objects/application.go:
##########
@@ -2157,6 +2157,11 @@ func (sa *Application) GetMaxResource()
*resources.Resource {
return sa.getResourceFromTags(siCommon.AppTagNamespaceResourceQuota)
}
+// GetMaxApps returns the max apps that is set in the application tags
+func (sa *Application) GetMaxApps() string {
Review Comment:
Need to line up with `getResourceFromTags()` we should return the converted
value not the tag value, convert to uint64 here
##########
pkg/scheduler/partition.go:
##########
@@ -342,13 +342,14 @@ func (pc *PartitionContext) AddApplication(app
*objects.Application) error {
guaranteedRes := app.GetGuaranteedResource()
maxRes := app.GetMaxResource()
- if !isRecoveryQueue && (guaranteedRes != nil || maxRes != nil) {
+ maxApps := app.GetMaxApps()
Review Comment:
change maxApps to a uint64...
##########
pkg/scheduler/objects/queue.go:
##########
@@ -444,12 +445,25 @@ func (sq *Queue) setResources(guaranteedResource,
maxResource *resources.Resourc
log.Log(log.SchedQueue).Debug("guaranteed resources setting
ignored: cannot set zero guaranteed resources",
zap.String("queue", sq.QueuePath))
}
+
+ // For shim side we already make sure the max apps > 0 and valid parsing
+ maxAppsInt, err := strconv.ParseInt(maxApps, 10, 64)
+ maxAppsUint := uint64(maxAppsInt)
+ if err == nil {
+ log.Log(log.SchedQueue).Debug("setting max running apps",
+ zap.String("queue", sq.QueuePath),
+ zap.Uint64("current", sq.maxRunningApps),
+ zap.Uint64("new", maxAppsUint))
+ }
+ sq.maxRunningApps = maxAppsUint
+
+ //nolint:godox //TODO, we need max apps metrics and event change?
}
-func (sq *Queue) SetResources(guaranteedResource, maxResource
*resources.Resource) {
+func (sq *Queue) SetResources(guaranteedResource, maxResource
*resources.Resource, maxApps string) {
Review Comment:
I would not change the signature, leave as is
##########
pkg/scheduler/objects/queue.go:
##########
@@ -444,12 +445,25 @@ func (sq *Queue) setResources(guaranteedResource,
maxResource *resources.Resourc
log.Log(log.SchedQueue).Debug("guaranteed resources setting
ignored: cannot set zero guaranteed resources",
zap.String("queue", sq.QueuePath))
}
+
+ // For shim side we already make sure the max apps > 0 and valid parsing
+ maxAppsInt, err := strconv.ParseInt(maxApps, 10, 64)
+ maxAppsUint := uint64(maxAppsInt)
+ if err == nil {
+ log.Log(log.SchedQueue).Debug("setting max running apps",
+ zap.String("queue", sq.QueuePath),
+ zap.Uint64("current", sq.maxRunningApps),
+ zap.Uint64("new", maxAppsUint))
+ }
+ sq.maxRunningApps = maxAppsUint
Review Comment:
See first comment this belongs in the application code, which should return
an uint64
##########
pkg/scheduler/objects/queue.go:
##########
@@ -388,11 +388,12 @@ func (sq *Queue) setResourcesFromConf(resource
configs.Resources) error {
return err
}
- sq.setResources(guaranteedResource, maxResource)
+ //nolint:godox //TODO, do we support setting maxApps for static queue?
+ sq.setResources(guaranteedResource, maxResource, "")
return nil
}
-func (sq *Queue) setResources(guaranteedResource, maxResource
*resources.Resource) {
+func (sq *Queue) setResources(guaranteedResource, maxResource
*resources.Resource, maxApps string) {
Review Comment:
I would not change the signature, leave as is
##########
pkg/scheduler/objects/queue.go:
##########
@@ -388,11 +388,12 @@ func (sq *Queue) setResourcesFromConf(resource
configs.Resources) error {
return err
}
- sq.setResources(guaranteedResource, maxResource)
+ //nolint:godox //TODO, do we support setting maxApps for static queue?
Review Comment:
We should not add a TODO here.
And yes we allow setting maxApps on any queue. Updating seems to be
currently broken as only `NewConfiguredQueue()` sets the maxRunningApps. It
should be moved into `sq.applyConf()` to fix that.
##########
pkg/scheduler/partition.go:
##########
@@ -342,13 +342,14 @@ func (pc *PartitionContext) AddApplication(app
*objects.Application) error {
guaranteedRes := app.GetGuaranteedResource()
maxRes := app.GetMaxResource()
- if !isRecoveryQueue && (guaranteedRes != nil || maxRes != nil) {
+ maxApps := app.GetMaxApps()
+ if !isRecoveryQueue && (guaranteedRes != nil || maxRes != nil ||
maxApps != "") {
// 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(guaranteedRes, maxRes)
+ queue.SetResources(guaranteedRes, maxRes, maxApps)
Review Comment:
add a call `queue.SetMaxRunningApps()` update the signature to take a unit64
and update the code and comment for that function
--
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]
