[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #430: [YUNIKORN-1282] Resource metrics are not tracked correctly for all queue types

[GitBox <gi...@apache.org>]

wilfred-s commented on code in PR #430:
URL: https://github.com/apache/yunikorn-core/pull/430#discussion_r948579426


##########
pkg/scheduler/objects/queue.go:
##########
@@ -269,12 +269,14 @@ func (sq *Queue) setResources(resource configs.Resources) error {
 	} else {
 		log.Logger().Debug("max resources setting ignored: cannot set zero max resources")
 	}
+	sq.updateMaxResourceMetrics()

Review Comment:
   I think we need this inside the `if` clause with the check for `StrictlyGreaterThanZero`. If there is no max set the object is a non nil pointer with no content.  We do not allow 0 values without at least 1 value larger than 0. That means this call would not result in any metrics being generated. The for loop inside the  `updateMaxResourceMetrics` never executes. It thus is a call to nowhere.
   
   Same for guaranteed resources.



##########
pkg/scheduler/objects/queue.go:
##########
@@ -1017,6 +1019,8 @@ func (sq *Queue) SetMaxResource(max *resources.Resource) {
 		zap.String("current max", sq.maxResource.String()),
 		zap.String("new max", max.String()))
 	sq.maxResource = max.Clone()
+	sq.updateGuaranteedResourceMetrics()

Review Comment:
   We only set max so we should not set the guaranteed metric.



-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org