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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
