[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #538: [YUNIKORN-1607] Configuration processing should support limit hierarc…

["wilfred-s (via GitHub)" <gi...@apache.org>]

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


##########
pkg/common/configs/configvalidator.go:
##########
@@ -153,6 +211,56 @@ func checkQueueMaxApplications(cur QueueConfig) error {
 	return nil
 }
 
+func checkLimitMaxApplications(cur QueueConfig, parent *QueueConfig, users map[string]uint64, groups map[string]uint64) error {
+	// Collect user and group limits of parent queue
+	if parent != nil {
+		for _, limit := range parent.Limits {
+			parentLimitMaxApplications := limit.MaxApplications
+			for _, user := range limit.Users {
+				users[user] = parentLimitMaxApplications
+			}
+			for _, group := range limit.Groups {
+				groups[group] = parentLimitMaxApplications
+			}
+		}
+	}
+
+	// compare user & group limit setting between the current queue and parent queue
+	for _, limit := range cur.Limits {
+		limitMaxApplications := limit.MaxApplications
+		for _, user := range limit.Users {
+			// Is user limit setting exists?
+			if userMaxApplications, ok := users[user]; ok {
+				if userMaxApplications != 0 && (userMaxApplications < limitMaxApplications || limitMaxApplications == 0) {
+					return fmt.Errorf("user %s max applications %v of queue %s is greater than immediate or ancestor parent max applications %v", user, limitMaxApplications, cur.Name, userMaxApplications)
+				}
+				users[user] = uint64(math.Min(float64(limitMaxApplications), float64(userMaxApplications)))

Review Comment:
   Introduce a simple util function in our code: `Min(x, y uint64) uint64` that returns one or the other



##########
pkg/common/configs/configvalidator.go:
##########
@@ -153,6 +211,56 @@ func checkQueueMaxApplications(cur QueueConfig) error {
 	return nil
 }
 
+func checkLimitMaxApplications(cur QueueConfig, parent *QueueConfig, users map[string]uint64, groups map[string]uint64) error {
+	// Collect user and group limits of parent queue
+	if parent != nil {
+		for _, limit := range parent.Limits {
+			parentLimitMaxApplications := limit.MaxApplications
+			for _, user := range limit.Users {
+				users[user] = parentLimitMaxApplications
+			}
+			for _, group := range limit.Groups {
+				groups[group] = parentLimitMaxApplications
+			}
+		}
+	}
+
+	// compare user & group limit setting between the current queue and parent queue
+	for _, limit := range cur.Limits {
+		limitMaxApplications := limit.MaxApplications
+		for _, user := range limit.Users {
+			// Is user limit setting exists?
+			if userMaxApplications, ok := users[user]; ok {
+				if userMaxApplications != 0 && (userMaxApplications < limitMaxApplications || limitMaxApplications == 0) {
+					return fmt.Errorf("user %s max applications %v of queue %s is greater than immediate or ancestor parent max applications %v", user, limitMaxApplications, cur.Name, userMaxApplications)

Review Comment:
   use %d for integers not %v



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