craigcondit commented on code in PR #935:
URL: https://github.com/apache/yunikorn-core/pull/935#discussion_r1727443366
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1258,6 +1258,48 @@ func (sq *Queue) GetMaxResource() *resources.Resource {
return sq.internalGetMax(limit)
}
+// GetFairMaxResource computes the fair max resources for a given queue.
+// Starting with the root, descend down to the target queue allowing children
to override Resource values .
+// If the root includes an explicit 0 value for a Resource, do not include it
in the accumulator and treat it as missing.
+// If no children provide a maximum capacity override, the resulting value
will be the value found on the Root.
+// It is useful for fair-scheduling to allow a ratio to be produced
representing the rough utilization % of a given queue.
+func (sq *Queue) GetFairMaxResource() *resources.Resource {
+ var limit *resources.Resource
+ if sq.parent == nil {
+ cleaned := resources.NewResource()
+ if sq.maxResource == nil {
+ return cleaned
+ }
+
+ for k, v := range sq.maxResource.Resources {
+ if v != 0 {
+ cleaned.Resources[k] = v
+ }
+ }
Review Comment:
This block can be replaced by:
```
return sq.GetMaxResource().Clone()
```
As root queue resources are now always in a pruned state.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1258,6 +1258,48 @@ func (sq *Queue) GetMaxResource() *resources.Resource {
return sq.internalGetMax(limit)
}
+// GetFairMaxResource computes the fair max resources for a given queue.
+// Starting with the root, descend down to the target queue allowing children
to override Resource values .
+// If the root includes an explicit 0 value for a Resource, do not include it
in the accumulator and treat it as missing.
+// If no children provide a maximum capacity override, the resulting value
will be the value found on the Root.
+// It is useful for fair-scheduling to allow a ratio to be produced
representing the rough utilization % of a given queue.
+func (sq *Queue) GetFairMaxResource() *resources.Resource {
+ var limit *resources.Resource
+ if sq.parent == nil {
+ cleaned := resources.NewResource()
+ if sq.maxResource == nil {
+ return cleaned
+ }
+
+ for k, v := range sq.maxResource.Resources {
+ if v != 0 {
+ cleaned.Resources[k] = v
+ }
+ }
+
+ return cleaned
+ }
+
+ limit = sq.parent.GetFairMaxResource()
+ return sq.internalGetFairMaxResource(limit)
+}
+
+func (sq *Queue) internalGetFairMaxResource(limit *resources.Resource)
*resources.Resource {
+ sq.RLock()
+ defer sq.RUnlock()
+
+ if sq.maxResource.IsEmpty() || limit.IsEmpty() {
+ return limit
+ }
+
+ // perform merge. child wins every resources collision
+ for k, v := range sq.maxResource.Resources {
+ limit.Resources[k] = v
+ }
+
+ return limit
Review Comment:
Here, do your clone of `limit` and only update the clone, otherwise you're
mutating an object which was passed into the function.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1258,6 +1258,48 @@ func (sq *Queue) GetMaxResource() *resources.Resource {
return sq.internalGetMax(limit)
}
+// GetFairMaxResource computes the fair max resources for a given queue.
+// Starting with the root, descend down to the target queue allowing children
to override Resource values .
+// If the root includes an explicit 0 value for a Resource, do not include it
in the accumulator and treat it as missing.
+// If no children provide a maximum capacity override, the resulting value
will be the value found on the Root.
+// It is useful for fair-scheduling to allow a ratio to be produced
representing the rough utilization % of a given queue.
+func (sq *Queue) GetFairMaxResource() *resources.Resource {
+ var limit *resources.Resource
+ if sq.parent == nil {
+ cleaned := resources.NewResource()
+ if sq.maxResource == nil {
+ return cleaned
+ }
+
+ for k, v := range sq.maxResource.Resources {
+ if v != 0 {
+ cleaned.Resources[k] = v
+ }
+ }
+
+ return cleaned
+ }
+
+ limit = sq.parent.GetFairMaxResource()
+ return sq.internalGetFairMaxResource(limit)
+}
+
+func (sq *Queue) internalGetFairMaxResource(limit *resources.Resource)
*resources.Resource {
+ sq.RLock()
+ defer sq.RUnlock()
+
+ if sq.maxResource.IsEmpty() || limit.IsEmpty() {
+ return limit
Review Comment:
This should be `return limit.Clone()`. `Resource` objects are not immutable,
so it's better to clone since we're returning one.
--
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]
