[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #525: [YUNIKORN-1668] Check there must be one more limit setting for group with specific name defined in the config when use wildcard group limit

["zhuqi-lucas (via GitHub)" <gi...@apache.org>]

zhuqi-lucas commented on code in PR #525:
URL: https://github.com/apache/yunikorn-core/pull/525#discussion_r1161580287


##########
pkg/common/configs/configvalidator.go:
##########
@@ -86,6 +86,14 @@ type placementStaticPath struct {
 
 type placementPathCheckResult int
 
+type LimitCheckHelper struct {

Review Comment:
   Good suggestion, addressed in latest PR.



##########
pkg/common/configs/configvalidator.go:
##########
@@ -320,13 +328,17 @@ func checkLimit(limit Limit, currIdx int, userWildCardIdx, groupWildCardIdx *int
 		// It means the wildcard for user should be the last item for limits object list which including the username,
 		// and we should only set one wildcard user for all limits
 		if name == "*" {
-			if *userWildCardIdx != -1 && currIdx > *userWildCardIdx {
+			if limitCheckHelper.userWildCardIdx != -1 && currIdx > limitCheckHelper.userWildCardIdx {
 				return fmt.Errorf("should not set more than one wildcard user")
 			}
-			*userWildCardIdx = currIdx
-		} else if *userWildCardIdx != -1 && currIdx > *userWildCardIdx {
+			limitCheckHelper.userWildCardIdx = currIdx
+		} else if limitCheckHelper.userWildCardIdx != -1 && currIdx > limitCheckHelper.userWildCardIdx {
 			return fmt.Errorf("should not set no wildcard user %s after wildcard user limit", name)
 		}
+		if limitCheckHelper.existedUserName[name] {
+			return fmt.Errorf("duplicated user name %s , already existed", name)
+		}
+		limitCheckHelper.existedUserName[name] = true

Review Comment:
   Good suggestion, addressed in latest PR.



##########
pkg/common/configs/configvalidator.go:
##########
@@ -336,14 +348,30 @@ func checkLimit(limit Limit, currIdx int, userWildCardIdx, groupWildCardIdx *int
 		// It means the wildcard for group should be the last item for limits object list which including the group name,
 		// and we should only set one wildcard group for all limits
 		if name == "*" {
-			if *groupWildCardIdx != -1 && currIdx > *groupWildCardIdx {
+			if limitCheckHelper.groupWildCardIdx != -1 && currIdx > limitCheckHelper.groupWildCardIdx {
 				return fmt.Errorf("should not set more than one wildcard group")
 			}
-			*groupWildCardIdx = currIdx
-		} else if *groupWildCardIdx != -1 && currIdx > *groupWildCardIdx {
+			limitCheckHelper.groupWildCardIdx = currIdx
+		} else if limitCheckHelper.groupWildCardIdx != -1 && currIdx > limitCheckHelper.groupWildCardIdx {
 			return fmt.Errorf("should not set no wildcard group %s after wildcard group limit", name)
+		} else {
+			limitCheckHelper.groupSpecificIdx = currIdx
+		}
+		if limitCheckHelper.existedGroupName[name] {
+			return fmt.Errorf("duplicated group name %s , already existed", name)
 		}
+		limitCheckHelper.existedGroupName[name] = true
 	}
+
+	// Specifying a wildcard for the group limit sets a cumulative limit for all users in that queue.
+	// If there is no specific group mentioned the wildcard group limit would thus be the same as the queue limit.
+	// For that reason we do not allow specifying only one group limit that is using the wildcard.
+	// There must be at least one limit with a group name defined.
+	if limitCheckHelper.groupWildCardIdx != -1 && limitCheckHelper.groupSpecificIdx == -1 {
+		return fmt.Errorf("should not specify only one group limit that is using the wildcard. " +
+			"There must be at least one limit with a group name defined ")
+	}
+

Review Comment:
   Good suggestion, addressed in latest PR.



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