wilfred-s commented on code in PR #935:
URL: https://github.com/apache/yunikorn-core/pull/935#discussion_r1718085818
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1220,6 +1220,50 @@ 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
Review Comment:
No longer needed as the max for root is already pruned.
This direct access can cause a data race or even a panic as we access the
max for the root outside a lock and it can be changed to a nil while we iterate
via `SetMaxResource()`
Need to call `GetMaxResource()` that gives us the cloned resource while
locking the queue to work on.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1220,6 +1220,50 @@ 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(parent *resources.Resource)
*resources.Resource {
+ sq.RLock()
+ defer sq.RUnlock()
+
+ us := sq.maxResource
+ if us == nil {
+ return parent.Clone()
+ }
Review Comment:
* should be using `sq.maxResource` directly no need to alias to a new pointer
* should use `IsEmpty()` to check for an empty resource, no types means no
work
* no need to call clone on the parent the parent limit must never be the
resource set on the queue. See previous comment also.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1220,6 +1220,50 @@ 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(parent *resources.Resource)
*resources.Resource {
Review Comment:
Suggest to rename `parent` to `limit` as used in the caller
##########
pkg/scheduler/objects/queue_test.go:
##########
@@ -869,6 +869,114 @@ func TestMaxHeadroomMax(t *testing.T) {
assert.Assert(t, resources.Equals(res, headRoom), "leaf2 queue head
room not as expected %v, got: %v", res, headRoom)
}
+func TestGetFairMaxResource(t *testing.T) {
+ tests := []struct {
Review Comment:
Add a name field to the structure to show which test is running, wrap the
tests in a Run:
```
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
... test code here
})
}
```
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1220,6 +1220,50 @@ 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(parent *resources.Resource)
*resources.Resource {
+ sq.RLock()
+ defer sq.RUnlock()
+
+ us := sq.maxResource
+ if us == nil {
+ return parent.Clone()
+ }
+
+ out := parent.Clone()
+
+ // perform merge. child wins every resources collision
+ for k, v := range us.Resources {
+ out.Resources[k] = v
+ }
Review Comment:
If the parent limit is nil then we panic here. That can happen if we have
not registered any nodes yet. `Clone()` can return a nil object.
Can account for that by adding it to the check earlier:
```
if sq.maxResource.IsEmpty() || limit.IsEmpty() {
return limit
}
```
Besides that we should directly manipulate the limit object, no cloning.
##########
pkg/scheduler/objects/queue_test.go:
##########
@@ -869,6 +869,114 @@ func TestMaxHeadroomMax(t *testing.T) {
assert.Assert(t, resources.Equals(res, headRoom), "leaf2 queue head
room not as expected %v, got: %v", res, headRoom)
}
+func TestGetFairMaxResource(t *testing.T) {
+ tests := []struct {
+ RootResource map[string]string
+ ParentResource map[string]string
+ Tier0Resource map[string]string
+ Tier0Expectation map[string]string
+ Tier1Resource map[string]string
+ Tier1Expectation map[string]string
+ }{
Review Comment:
Missing nil input tests, for root, parent and child or some combination...
Missing parent max with type different than child
Missing parent explicit 0 limit for type not set in child
--
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]
