[GitHub] [incubator-yunikorn-core] chia7712 commented on a change in pull request #349: [YUNIKORN-981] cleanup queue comments and logging

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

chia7712 commented on a change in pull request #349:
URL: https://github.com/apache/incubator-yunikorn-core/pull/349#discussion_r769277194



##########
File path: pkg/scheduler/objects/queue.go
##########
@@ -129,8 +130,7 @@ func NewDynamicQueue(name string, leaf bool, parent *Queue) (*Queue, error) {
 	}
 	// name might not be checked do it here
 	if !configs.QueueNameRegExp.MatchString(name) {
-		return nil, fmt.Errorf("invalid queue name %s, a name must only have alphanumeric characters,"+
-			" - or _, and be no longer than 64 characters", name)
+		return nil, fmt.Errorf("invalid queue name '%s', a name must only have alphanumeric characters, - or _, and be no longer than 64 characters", name)

Review comment:
       Is this line too long?

##########
File path: pkg/scheduler/objects/queue.go
##########
@@ -783,17 +808,22 @@ func (sq *Queue) IncAllocatedResource(alloc *resources.Resource, nodeReported bo
 	newAllocated := resources.Add(sq.allocatedResource, alloc)
 	if !nodeReported {
 		if !sq.maxResource.FitInMaxUndef(newAllocated) {
-			return fmt.Errorf("allocation (%v) puts queue %s over maximum allocation (%v)",
-				alloc, sq.QueuePath, sq.maxResource)
+			return fmt.Errorf("allocation (%v) puts queue '%s' over maximum allocation (%v), current usage (%v)",
+				alloc, sq.QueuePath, sq.maxResource, sq.allocatedResource)
 		}
 	}
 	// check the parent: need to pass before updating
 	if sq.parent != nil {
 		if err := sq.parent.IncAllocatedResource(alloc, nodeReported); err != nil {
-			log.Logger().Error("parent queue exceeds maximum resource",
-				zap.Any("allocationId", alloc),
-				zap.String("maxResource", sq.maxResource.String()),
-				zap.Error(err))
+			// only log the warning if we get to the leaf

Review comment:
       The main reason is that we don't want to see looped warnings, right? If so, could we add the main reason to the comment? Otherwise, it seems to me the comment is not more clear than the code below.




-- 
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