craigcondit commented on code in PR #935:
URL: https://github.com/apache/yunikorn-core/pull/935#discussion_r1708093769
##########
pkg/common/resources/resources.go:
##########
@@ -455,6 +455,74 @@ func (r *Resource) fitIn(smaller *Resource, skipUndef
bool) bool {
return true
}
+// Denominator Resources can be either guaranteed Resources or fairmax
Resources.
+// If the quanity is explicitly 0 or negative, we will check usage. If usage
>= 0, the share will be set to 1.0. Otherwise, it will be set 0.0.
+func getShareFairForDenominator(resourceType string, allocated Quantity,
denominatorResources *Resource) (float64, bool) {
+ if denominatorResources == nil {
+ return 0.0, false
+ }
+
+ denominator, ok := denominatorResources.Resources[resourceType]
+
+ switch {
+ case ok && denominator <= 0:
+ if denominator < 0 {
+ log.Log(log.Resources).Debug("denominator is negative.
will not compute share",
+ zap.String("resource key", resourceType),
+ zap.Int64("resource quantity",
int64(denominator)))
+ }
+
+ if allocated <= 0 {
+ //explicit 0 or negative value with NO usage
+ return float64(0.0), true
Review Comment:
I don't believe the cast is necessary here.
##########
pkg/common/resources/resources.go:
##########
@@ -455,6 +455,74 @@ func (r *Resource) fitIn(smaller *Resource, skipUndef
bool) bool {
return true
}
+// Denominator Resources can be either guaranteed Resources or fairmax
Resources.
+// If the quanity is explicitly 0 or negative, we will check usage. If usage
>= 0, the share will be set to 1.0. Otherwise, it will be set 0.0.
+func getShareFairForDenominator(resourceType string, allocated Quantity,
denominatorResources *Resource) (float64, bool) {
+ if denominatorResources == nil {
+ return 0.0, false
+ }
+
+ denominator, ok := denominatorResources.Resources[resourceType]
+
+ switch {
+ case ok && denominator <= 0:
+ if denominator < 0 {
Review Comment:
Can we remove the logging? This is hot-path code, even evaluating whether
log level is active is pretty expensive. Also, if you have n queues to compare
you're going to emit ~ n log(n) debug calls per sort() invocation. Take that
times every scheduling cycle, and you're going to emit millions of log entries
very quickly if someone is unfortunate enough to turn this on.
##########
pkg/common/resources/resources.go:
##########
@@ -455,6 +455,74 @@ func (r *Resource) fitIn(smaller *Resource, skipUndef
bool) bool {
return true
}
+// Denominator Resources can be either guaranteed Resources or fairmax
Resources.
+// If the quanity is explicitly 0 or negative, we will check usage. If usage
>= 0, the share will be set to 1.0. Otherwise, it will be set 0.0.
+func getShareFairForDenominator(resourceType string, allocated Quantity,
denominatorResources *Resource) (float64, bool) {
+ if denominatorResources == nil {
+ return 0.0, false
+ }
+
+ denominator, ok := denominatorResources.Resources[resourceType]
+
+ switch {
+ case ok && denominator <= 0:
+ if denominator < 0 {
+ log.Log(log.Resources).Debug("denominator is negative.
will not compute share",
+ zap.String("resource key", resourceType),
+ zap.Int64("resource quantity",
int64(denominator)))
+ }
+
+ if allocated <= 0 {
+ //explicit 0 or negative value with NO usage
+ return float64(0.0), true
+
+ } else {
+ //explicit 0 or negative value with usage
+ return float64(1.0), true
+
+ }
+ case denominator > 0:
+ return (float64(allocated) / float64(denominator)), true
+
+ default:
+ //no denominator. ie. no guarantee or fairmax for resourceType
+ return 0.0, false
+ }
+}
+
+// For a given queue, produce a ratio which represents it's current 'fair'
share usage.
+// Iterate over all of the allocated resource types. For each, compute the
ratio, ultimately returning the max ratio encountered.
+// The numerator will be the allocated usage.
+// If guarantees are present, they will be used for the denominator, otherwise
we will fallback to the 'maxfair' capacity of the cluster.
+func getFairShare(allocated, guaranteed, fair *Resource) float64 {
Review Comment:
Nit: Can we rename 'fair' to 'max' to align with how this is used elsewhere
in the code?
##########
pkg/common/resources/resources.go:
##########
@@ -455,6 +455,74 @@ func (r *Resource) fitIn(smaller *Resource, skipUndef
bool) bool {
return true
}
+// Denominator Resources can be either guaranteed Resources or fairmax
Resources.
+// If the quanity is explicitly 0 or negative, we will check usage. If usage
>= 0, the share will be set to 1.0. Otherwise, it will be set 0.0.
+func getShareFairForDenominator(resourceType string, allocated Quantity,
denominatorResources *Resource) (float64, bool) {
+ if denominatorResources == nil {
+ return 0.0, false
+ }
+
+ denominator, ok := denominatorResources.Resources[resourceType]
+
+ switch {
+ case ok && denominator <= 0:
+ if denominator < 0 {
+ log.Log(log.Resources).Debug("denominator is negative.
will not compute share",
+ zap.String("resource key", resourceType),
+ zap.Int64("resource quantity",
int64(denominator)))
+ }
+
+ if allocated <= 0 {
+ //explicit 0 or negative value with NO usage
+ return float64(0.0), true
+
+ } else {
+ //explicit 0 or negative value with usage
+ return float64(1.0), true
+
+ }
+ case denominator > 0:
+ return (float64(allocated) / float64(denominator)), true
+
+ default:
+ //no denominator. ie. no guarantee or fairmax for resourceType
+ return 0.0, false
+ }
+}
+
+// For a given queue, produce a ratio which represents it's current 'fair'
share usage.
+// Iterate over all of the allocated resource types. For each, compute the
ratio, ultimately returning the max ratio encountered.
+// The numerator will be the allocated usage.
+// If guarantees are present, they will be used for the denominator, otherwise
we will fallback to the 'maxfair' capacity of the cluster.
+func getFairShare(allocated, guaranteed, fair *Resource) float64 {
+ if allocated == nil || len(allocated.Resources) == 0 {
+ return float64(0)
Review Comment:
Unnecessary cast.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1220,6 +1220,63 @@ func (sq *Queue) GetMaxResource() *resources.Resource {
return sq.internalGetMax(limit)
}
+// 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 {
+ cleanedRootResources := resources.NewResource()
+ if sq.maxResource == nil {
+ return cleanedRootResources
+ }
+
+ for k, v := range sq.maxResource.Resources {
+ if v != 0 {
+ cleanedRootResources.Resources[k] = v
+ }
+ }
+
+ return cleanedRootResources
+ }
+
+ 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 parent == nil && us == nil {
+ return nil
+ }
+ if parent == nil {
+ return us.Clone()
+ }
+ if us == nil {
+ return parent.Clone()
+ }
+
+ out := resources.NewResource()
+ for k, v := range parent.Resources {
+ out.Resources[k] = v
+ }
+
+ //child wins every resources collision
+ for k, v := range us.Resources {
+ log.Log(log.SchedQueue).Info("child overwriting parent",
Review Comment:
Again, skip the logging here. It will overwhelm the rest of the system.
##########
pkg/common/resources/resources.go:
##########
@@ -455,6 +455,74 @@ func (r *Resource) fitIn(smaller *Resource, skipUndef
bool) bool {
return true
}
+// Denominator Resources can be either guaranteed Resources or fairmax
Resources.
+// If the quanity is explicitly 0 or negative, we will check usage. If usage
>= 0, the share will be set to 1.0. Otherwise, it will be set 0.0.
+func getShareFairForDenominator(resourceType string, allocated Quantity,
denominatorResources *Resource) (float64, bool) {
+ if denominatorResources == nil {
+ return 0.0, false
+ }
+
+ denominator, ok := denominatorResources.Resources[resourceType]
+
+ switch {
+ case ok && denominator <= 0:
+ if denominator < 0 {
+ log.Log(log.Resources).Debug("denominator is negative.
will not compute share",
+ zap.String("resource key", resourceType),
+ zap.Int64("resource quantity",
int64(denominator)))
+ }
+
+ if allocated <= 0 {
+ //explicit 0 or negative value with NO usage
+ return float64(0.0), true
+
+ } else {
+ //explicit 0 or negative value with usage
+ return float64(1.0), true
+
+ }
+ case denominator > 0:
+ return (float64(allocated) / float64(denominator)), true
+
+ default:
+ //no denominator. ie. no guarantee or fairmax for resourceType
+ return 0.0, false
+ }
+}
+
+// For a given queue, produce a ratio which represents it's current 'fair'
share usage.
+// Iterate over all of the allocated resource types. For each, compute the
ratio, ultimately returning the max ratio encountered.
+// The numerator will be the allocated usage.
+// If guarantees are present, they will be used for the denominator, otherwise
we will fallback to the 'maxfair' capacity of the cluster.
+func getFairShare(allocated, guaranteed, fair *Resource) float64 {
+ if allocated == nil || len(allocated.Resources) == 0 {
+ return float64(0)
+ }
+
+ var maxShare float64
+ for k, v := range allocated.Resources {
+ var nextShare float64
+
+ //if usage <= 0, resource has no share
+ if allocated.Resources[k] < 0 {
+ continue
+ }
+
+ nextShare, found := getShareFairForDenominator(k, v, guaranteed)
+ if !found {
+ nextShare, found = getShareFairForDenominator(k, v,
fair)
+ }
+ if found {
+ if nextShare > maxShare {
+ maxShare = nextShare
+ }
+ }
Review Comment:
This whole expression can be simplified:
```
if !found {
nextShare, found = getShareFairForDenominator(k, v, fair)
} else if nextShare > maxShare {
maxShare = nextShare
}
```
##########
pkg/common/resources/resources.go:
##########
@@ -455,6 +455,74 @@ func (r *Resource) fitIn(smaller *Resource, skipUndef
bool) bool {
return true
}
+// Denominator Resources can be either guaranteed Resources or fairmax
Resources.
+// If the quanity is explicitly 0 or negative, we will check usage. If usage
>= 0, the share will be set to 1.0. Otherwise, it will be set 0.0.
+func getShareFairForDenominator(resourceType string, allocated Quantity,
denominatorResources *Resource) (float64, bool) {
+ if denominatorResources == nil {
+ return 0.0, false
+ }
+
+ denominator, ok := denominatorResources.Resources[resourceType]
+
+ switch {
+ case ok && denominator <= 0:
+ if denominator < 0 {
+ log.Log(log.Resources).Debug("denominator is negative.
will not compute share",
+ zap.String("resource key", resourceType),
+ zap.Int64("resource quantity",
int64(denominator)))
+ }
+
+ if allocated <= 0 {
+ //explicit 0 or negative value with NO usage
+ return float64(0.0), true
+
+ } else {
+ //explicit 0 or negative value with usage
+ return float64(1.0), true
Review Comment:
Also cast not required.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1220,6 +1220,63 @@ func (sq *Queue) GetMaxResource() *resources.Resource {
return sq.internalGetMax(limit)
}
+// 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.
Review Comment:
Add the function name as the first word in the description per godoc
conventions. I.e. "GetFairMaxResource computes the fair maximum resources ...."
##########
pkg/scheduler/objects/sorters.go:
##########
@@ -65,8 +65,10 @@ func sortQueuesByPriorityAndFairness(queues []*Queue) {
if lPriority < rPriority {
return false
}
- comp :=
resources.CompUsageRatioSeparately(l.GetAllocatedResource(),
l.GetGuaranteedResource(),
- r.GetAllocatedResource(), r.GetGuaranteedResource())
+
Review Comment:
I think this function needs some work as long as we're rewriting it. We are
doing the expensive recursive computation of resources on every comparison.
Since we already have a slice of queues, it would be better to precompute a
slice of resources computed on each one, and then use that in the comparison
function so that we only create the resource maps once per sort run instead of
once per comparison.
--
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]
