You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2022/08/11 22:10:26 UTC
[GitHub] [yunikorn-core] lixmgl opened a new pull request, #429: [YUNIKORN-790] Implement MaxApplications enforcement
lixmgl opened a new pull request, #429:
URL: https://github.com/apache/yunikorn-core/pull/429
### What is this PR for?
Adding maxApplication implementation for Yunikorn queue.
This PR includes and updates existing wip https://github.com/apache/yunikorn-core/pull/384
### What type of PR is it?
* [ ] - Bug Fix
* [ *] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
NA
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-790
### How should this be tested?
1. local test passed by running "make test" (refer to screenshot)
2. Verified by deploying change to test k8s cluster, I set maxapplication = 3 for queue1, and submitted 5 spark applications to queue1. Only 3 applications were in RUNNING state, the other 2 applications were in PENDING state. End to end functionality is working as expected. (refer to screenshot)
### Screenshots (if appropriate)
Local test result:
End to end test result:
### Questions:
NA
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1026994795
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) canRun() bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.canRun()
+ }
+ return sq.internalCanRun(ok)
+}
+
+func (sq *Queue) internalCanRun(ok bool) bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ return ok && sq.runningApps < sq.maxRunningApps
+}
Review Comment:
@craigcondit Actually I removed canRun() method, this file is outdated.
Current canRun logic is inside this method: https://github.com/apache/yunikorn-core/pull/429/commits/030c1b44a69ec8302fc566f57af7489b26d5cc0e#diff-27632d48eb925e150a33bc92370ceaa66c31048018d11ca7a53a0b50ab7250acR1345
Please review and lmk if you have any comments.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1026994482
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
@craigcondit Thanks for the info, it make sense.
I updated by merging them into one method. Please review.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
Review Comment:
Updated.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r990563963
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
reverted to unsigned int type. cc @wilfred-s
related discussion is here: https://github.com/apache/yunikorn-site/pull/188#discussion_r990446069
--
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
[GitHub] [yunikorn-core] lixmgl commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1236404670
> I have some concerns regarding some pieces of the code, please take a look at those, thanks.
Thanks for reviewing the code. I have updated them and addressed all your comments. Please take another look.
--
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
[GitHub] [yunikorn-core] wilfred-s commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1330442670
> Can we merge this PR to master first then add your PR?
This PR formed the basis under my port. We can merge the ported PR in one go. @craigcondit will handle the proper attribution to you when we commit the change. I just did some cleanup and the port :-)
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r954326263
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) canRun() bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.canRun()
+ }
+ return sq.internalCanRun(ok)
+}
+
+func (sq *Queue) internalCanRun(ok bool) bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ return ok && sq.runningApps < sq.maxRunningApps
+}
Review Comment:
same reason as before, also I updated logic in canRun https://github.com/apache/yunikorn-core/pull/435
--
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
[GitHub] [yunikorn-core] craigcondit commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
craigcondit commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1331096828
Closed in favor of #455.
--
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
[GitHub] [yunikorn-core] lixmgl commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1320706826
>
Thanks @craigcondit for reviewing the PR.
I updated your comments about combining the methods.
_canRun_ code is outdate, please review _incAllocatingAcceptedAppsIfCanRun_ method
--
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
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1028290813
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1164,13 +1175,16 @@ func (sq *Queue) TryReservedAllocate(iterator func() NodeIterator) *Allocation {
zap.String("appID", appID))
return nil
}
- alloc := app.tryReservedAllocate(headRoom, iterator)
- if alloc != nil {
- log.Logger().Debug("reservation found for allocation found on queue",
- zap.String("queueName", sq.QueuePath),
- zap.String("appID", appID),
- zap.String("allocation", alloc.String()))
- return alloc
+ if app.IsStarting() || app.IsRunning() || app.IsAccepted() {
+ alloc := app.tryReservedAllocate(headRoom, iterator)
+ if alloc != nil {
+ log.Logger().Debug("Rainie test accepted apps: reservation found for allocation found on queue",
Review Comment:
Can you remove your name from this log statement?
--
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
[GitHub] [yunikorn-core] lixmgl commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1322637144
>
Thank you @craigcondit. I addressed comments.
@wilfred-s @pbacsko please take a look when you have time, thanks!
--
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
[GitHub] [yunikorn-core] lixmgl commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1329587935
> @lixmgl / @pbacsko / @manirajv06 / @craigcondit Please check PR #455. I ported the changes to master and moved the logic into the queue code. I added unit tests and one full cycle test into the partition. I think I have covered all the cases.
PR https://github.com/apache/yunikorn-core/pull/455 LGTM.
Thanks for the refactoring and full cycle tests @wilfred-s
Happy late Thanksgiving :)
Can you merge my PR first then add your 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
[GitHub] [yunikorn-core] lixmgl commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1233327152
> I am not sure which PR is the one you are working on. The email pointed back to this PR: config validation code for the resources of a queue compared to its parent is here https://github.com/apache/yunikorn-core/blob/master/pkg/common/configs/configvalidator.go#L79
I merged two PRs together. Please only review this one.
--
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
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r945412946
##########
pkg/scheduler/objects/queue.go:
##########
@@ -657,6 +660,9 @@ func (sq *Queue) addChildQueue(child *Queue) error {
if sq.IsDraining() {
return fmt.Errorf("cannot add a child queue when queue is marked for deletion: %s", sq.QueuePath)
}
+ if sq.maxRunningApps != 0 && sq.maxRunningApps < child.maxRunningApps {
+ return fmt.Errorf("parent maxRunningApps must be larger than child maxRunningApps")
+ }
Review Comment:
This belongs in the config validations check, like we do for the "other" resources. Otherwise we could fail during the config change and leave the system in a strange state.
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -143,6 +143,7 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
+ app.queue.incRunningApps()
Review Comment:
This is the correct point (see the [usage tracking doc](https://docs.google.com/document/d/19FYOL1Uw7tSf74qyuVN40Fv-GdWd2KrBcHJpUQLTBZ4/edit#heading=h.481iug6rohb3)) but we need to make sure the metrics are updated in the same place as the queue itself.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
Review Comment:
This can be folded into the `decRunningApps()`
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) canRun() bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.canRun()
+ }
+ return sq.internalCanRun(ok)
+}
+
+func (sq *Queue) internalCanRun(ok bool) bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ return ok && sq.runningApps < sq.maxRunningApps
+}
+
+func (sq *Queue) GetRunningApps() uint64 {
+ sq.RLock()
+ defer sq.RUnlock()
+ return sq.runningApps
+}
+
+func (sq *Queue) SetMaxRunningApps(maxApps uint64) {
Review Comment:
Add a comment to the exported function to show it is test only
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) canRun() bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.canRun()
+ }
+ return sq.internalCanRun(ok)
+}
+
+func (sq *Queue) internalCanRun(ok bool) bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ return ok && sq.runningApps < sq.maxRunningApps
+}
Review Comment:
I had to really look twice to grasp what was going on with the `ok` flag. Isn't this a much simpler flow and faster, as we fail fast if a parent has failed:
```
func (sq *Queue) canRun() bool {
if sq == nil {
return false
}
// check the parent(s)
if sq.parent != nil {
ok := sq.parent.canRun()
if !ok {
return false
}
}
// check this queue only if all parents allowed it
sq.RLock()
defer sq.RUnlock()
if sq.maxRunningApps == 0 {
return true
}
return sq.runningApps < sq.maxRunningApps
}
```
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
This can be folded into the `incRunningApps()`
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r990563987
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
reverted to unsigned int type. cc @wilfred-s
related discussion is here: https://github.com/apache/yunikorn-site/pull/188#discussion_r990446069
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
same reason as before
--
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
[GitHub] [yunikorn-core] craigcondit closed pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
craigcondit closed pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
URL: https://github.com/apache/yunikorn-core/pull/429
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1028317321
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1164,13 +1175,16 @@ func (sq *Queue) TryReservedAllocate(iterator func() NodeIterator) *Allocation {
zap.String("appID", appID))
return nil
}
- alloc := app.tryReservedAllocate(headRoom, iterator)
- if alloc != nil {
- log.Logger().Debug("reservation found for allocation found on queue",
- zap.String("queueName", sq.QueuePath),
- zap.String("appID", appID),
- zap.String("allocation", alloc.String()))
- return alloc
+ if app.IsStarting() || app.IsRunning() || app.IsAccepted() {
+ alloc := app.tryReservedAllocate(headRoom, iterator)
+ if alloc != nil {
+ log.Logger().Debug("Rainie test accepted apps: reservation found for allocation found on queue",
Review Comment:
good catch, removed :)
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,71 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ success := true
+ if sq.parent != nil {
+ success = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(success, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(success bool, sa *Application) bool {
Review Comment:
Sure, combined.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1028317655
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,71 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ success := true
+ if sq.parent != nil {
+ success = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(success, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(success bool, sa *Application) bool {
Review Comment:
make sense, combined.
--
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
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1026683207
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) canRun() bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.canRun()
+ }
+ return sq.internalCanRun(ok)
+}
+
+func (sq *Queue) internalCanRun(ok bool) bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ return ok && sq.runningApps < sq.maxRunningApps
+}
Review Comment:
Wilfred is correct, this can be merged together. It can even be simplified further:
```
func (sq *Queue) canRun() bool {
if sq == nil {
return false
}
// check the parent(s)
if sq.parent != nil && !sq.parent.canRun() {
return false
}
// check this queue only if all parents allowed it
sq.RLock()
defer sq.RUnlock()
if sq.maxRunningApps == 0 {
return true
}
return sq.runningApps < sq.maxRunningApps
}
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961055046
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
Does `maxApps` have to be an `uint64`? I mean I understand that it's more logical, but now we have these weird casts here and in queue.go:1359. Simple `int` is the most portable data type.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961038813
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,83 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(ok, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(ok bool, sa *Application) bool {
+ sq.Lock()
+ defer sq.Unlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ result := ok && (sq.runningApps+uint64(len(sq.allocatingAcceptedApps)) < sq.maxRunningApps)
+ if result {
+ _, contains := sq.allocatingAcceptedApps[sa.ApplicationID]
Review Comment:
Here, `contains` is typically named `ok`.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961038108
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,83 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(ok, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(ok bool, sa *Application) bool {
Review Comment:
Can we have something else instead of `ok`?
`ok` is basically an idiomatic variable for various things such as map lookup results.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r954318873
##########
pkg/scheduler/objects/queue.go:
##########
@@ -657,6 +660,9 @@ func (sq *Queue) addChildQueue(child *Queue) error {
if sq.IsDraining() {
return fmt.Errorf("cannot add a child queue when queue is marked for deletion: %s", sq.QueuePath)
}
+ if sq.maxRunningApps != 0 && sq.maxRunningApps < child.maxRunningApps {
+ return fmt.Errorf("parent maxRunningApps must be larger than child maxRunningApps")
+ }
Review Comment:
Can you point me where is other resources config validations check?
If you are referring this method: https://github.com/apache/yunikorn-core/blob/branch-1.0/pkg/scheduler/objects/queue.go#L207
I don't think it's a good idea to add this child maxRunningApps check into this method since we need to iterate each child.
--
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
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1026679609
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
Review Comment:
And as above, this is unnecessary, and these can be combined.
--
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
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1028296754
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,71 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ success := true
+ if sq.parent != nil {
+ success = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(success, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(success bool, sa *Application) bool {
Review Comment:
This can be combined into incAllocatingAppsIfCanRun() as well, since it is a tail call. I think that improves the visibility.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r959941574
##########
pkg/scheduler/objects/queue.go:
##########
@@ -657,6 +660,9 @@ func (sq *Queue) addChildQueue(child *Queue) error {
if sq.IsDraining() {
return fmt.Errorf("cannot add a child queue when queue is marked for deletion: %s", sq.QueuePath)
}
+ if sq.maxRunningApps != 0 && sq.maxRunningApps < child.maxRunningApps {
+ return fmt.Errorf("parent maxRunningApps must be larger than child maxRunningApps")
+ }
Review Comment:
Updated, added check into configValidator file.
--
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
[GitHub] [yunikorn-core] lixmgl commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1233332563
>
>
updated, please review.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r954316181
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -143,6 +143,7 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
+ app.queue.incRunningApps()
Review Comment:
Updated in https://github.com/apache/yunikorn-core/pull/435
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r989537935
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
@pbacsko what i did is first find the parent recursively [method](https://github.com/apache/yunikorn-core/pull/429/files#diff-27632d48eb925e150a33bc92370ceaa66c31048018d11ca7a53a0b50ab7250acR1315 )
Then lock this parent sq only and incRunningApps. It's not single threaded.
This way we can avoid locking all the parents during the recursion.
I recall @wilfred-s pointed out this issue here https://github.com/apache/yunikorn-core/pull/384/files#r843616529 for combining into one method.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
@pbacsko what i did is first find the parent recursively [code](https://github.com/apache/yunikorn-core/pull/429/files#diff-27632d48eb925e150a33bc92370ceaa66c31048018d11ca7a53a0b50ab7250acR1315 )
Then lock this parent sq only and incRunningApps. It's not single threaded.
This way we can avoid locking all the parents during the recursion.
I recall @wilfred-s pointed out this issue here https://github.com/apache/yunikorn-core/pull/384/files#r843616529 for combining into one method.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961042934
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,83 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(ok, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(ok bool, sa *Application) bool {
+ sq.Lock()
+ defer sq.Unlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
Review Comment:
just `return ok`
--
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
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1026679388
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
Inlining `internalIncRunningApps()` will not change the behavior in any way, because the unlock will happen prior to returning from `incRunningApps()` in either case. They are functionally equivalent. However, do be sure that the lock and defer still come immediately before incrementing `runningApps`. They should not be relocated to the top of `incRunningApps()`
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r989537935
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
@pbacsko what i did is first find the parent recursively [method](https://github.com/apache/yunikorn-core/pull/429/files#diff-27632d48eb925e150a33bc92370ceaa66c31048018d11ca7a53a0b50ab7250acR1310 )
Then lock this parent sq only and incRunningApps. It's not single threaded.
This way we can avoid locking all the parents during the recursion.
I recall @wilfred-s pointed out this issue here https://github.com/apache/yunikorn-core/pull/384/files#r843616529 for combining into one method.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961038108
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,83 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(ok, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(ok bool, sa *Application) bool {
Review Comment:
Can we have something else instead of "ok"?
"ok" is basically an idiomatic variable for various things such as map lookup results.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961041448
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -143,6 +143,10 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
+ app.queue.incRunningApps()
+ app.queue.decAllocatingAcceptedApps(app)
+ metrics.GetQueueMetrics(app.queuePath).IncQueueApplicationsRunning()
Review Comment:
We have this call at two places now: `enter_Starting` and `enter_Running`.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961050728
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
Does `maxApps` have to be an `uint64`? I mean I understand that it's more logical, but now we have these weird casts here and in line queue.go:1359. Simple `int` is the most portable data type.
--
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
[GitHub] [yunikorn-core] lixmgl commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1314906173
Hi @manirajv06 Can you take a look? I saw @wilfred-s requested a review from you 11 days ago.
--
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
[GitHub] [yunikorn-core] wilfred-s commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1326050765
@lixmgl / @pbacsko / @manirajv06 / @craigcondit
Please check PR #455. I ported the changes to master and moved the logic into the queue code.
I added unit tests and one full cycle test into the partition.
I think I have covered all the cases.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r954326263
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) canRun() bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.canRun()
+ }
+ return sq.internalCanRun(ok)
+}
+
+func (sq *Queue) internalCanRun(ok bool) bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ return ok && sq.runningApps < sq.maxRunningApps
+}
Review Comment:
same reason as before
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r954316181
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -143,6 +143,7 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
+ app.queue.incRunningApps()
Review Comment:
Updated in https://github.com/apache/yunikorn-core/pull/435
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -143,6 +143,7 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
+ app.queue.incRunningApps()
Review Comment:
Updated
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961046263
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
Does `maxApps` have to be an `uint64`? I mean I understand that it's more logical, but now we have these weird casts here and in queue.go:1359. Simple `int` is the most portable data type.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961041864
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -143,6 +143,10 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
+ app.queue.incRunningApps()
+ app.queue.decAllocatingAcceptedApps(app)
+ metrics.GetQueueMetrics(app.queuePath).IncQueueApplicationsRunning()
+ metrics.GetSchedulerMetrics().IncTotalApplicationsRunning()
Review Comment:
Same for this, line 174 and 175.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r954326483
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) canRun() bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.canRun()
+ }
+ return sq.internalCanRun(ok)
+}
+
+func (sq *Queue) internalCanRun(ok bool) bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ return ok && sq.runningApps < sq.maxRunningApps
+}
+
+func (sq *Queue) GetRunningApps() uint64 {
+ sq.RLock()
+ defer sq.RUnlock()
+ return sq.runningApps
+}
+
+func (sq *Queue) SetMaxRunningApps(maxApps uint64) {
Review Comment:
deleted this method since no longer needed.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r954318873
##########
pkg/scheduler/objects/queue.go:
##########
@@ -657,6 +660,9 @@ func (sq *Queue) addChildQueue(child *Queue) error {
if sq.IsDraining() {
return fmt.Errorf("cannot add a child queue when queue is marked for deletion: %s", sq.QueuePath)
}
+ if sq.maxRunningApps != 0 && sq.maxRunningApps < child.maxRunningApps {
+ return fmt.Errorf("parent maxRunningApps must be larger than child maxRunningApps")
+ }
Review Comment:
Can you point me where is other resources config validations check?
If you are referring this method: https://github.com/apache/yunikorn-core/blob/branch-1.0/pkg/scheduler/objects/queue.go#L207
I don't think it's a good idea to add this child maxRunningApps check into this method since we need to iterate each child.
--
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
[GitHub] [yunikorn-core] wilfred-s commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1226913247
I am not sure which PR is the one you are working on. The email pointed back to this PR:
config validation code for the resources of a queue compared to its parent is here https://github.com/apache/yunikorn-core/blob/master/pkg/common/configs/configvalidator.go#L79
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r979863391
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
@lixmgl I read the stackoverflow topic and that's related to goroutine scheduling. If what you're saying is correct that would mean that sequantial consistency is broken. What you're doing here is single threaded, meaning everything runs on a single goroutine. To me it would be extremely suprirising if merging `internalIncRunningApps()` into `incRunningApps()` would result in incorrect behavior.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961047820
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
Does `maxApps` have to be an `uint64`? I mean I understand that it's more logical, but now we have these weird casts here and in queue.go:1359. Simple `int` is the most portable data type.
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
Does `maxApps` have to be an `uint64`? I mean I understand that it's more logical, but now we have these weird casts here and in line queue.go:1359. Simple `int` is the most portable data type.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961046263
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
Does `maxApps` have to be an `uint64`? I mean I understand that it's more logical, but now we have these weird casts here and in line queue.go:1359. Simple `int` is the most portable data type.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961036509
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
I want to have some deeper understanding with this.
Will get back to this code change later.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r961036509
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
I want to have some deeper understanding with this.
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r954325378
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
The reason I separated this logic into a new method instead of merging into incRunningApps() is:
Based on my understanding, instruction order is not ensured in golang even with lock (refer: https://stackoverflow.com/questions/60340382/can-i-use-lock-to-ensure-instruction-order)
If we merge into incRunningApps(), sq.Lock() could get executed before this recursion. Each parent could be locked during recursion.
if sq.parent != nil {
ok = sq.parent.canRun()
}
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
Review Comment:
same reason as before.
--
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
[GitHub] [yunikorn-core] pbacsko commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r979863391
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
Review Comment:
@lixmgl I read the stackoverflow topic and that's related to goroutine scheduling. If what you're saying is correct that would mean that sequential consistency is broken. What you're doing here is single threaded, meaning everything runs on a single goroutine. To me it would be extremely suprirising if merging `internalIncRunningApps()` into `incRunningApps()` would result in incorrect behavior.
--
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
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1026683207
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) canRun() bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.canRun()
+ }
+ return sq.internalCanRun(ok)
+}
+
+func (sq *Queue) internalCanRun(ok bool) bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ return ok && sq.runningApps < sq.maxRunningApps
+}
Review Comment:
Wilfred is correct, this can be merged together. It can even be simplified further:
```func (sq *Queue) canRun() bool {
if sq == nil {
return false
}
// check the parent(s)
if sq.parent != nil && !sq.parent.canRun() {
return false
}
// check this queue only if all parents allowed it
sq.RLock()
defer sq.RUnlock()
if sq.maxRunningApps == 0 {
return true
}
return sq.runningApps < sq.maxRunningApps
}
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r1026994795
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1304,67 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) canRun() bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.canRun()
+ }
+ return sq.internalCanRun(ok)
+}
+
+func (sq *Queue) internalCanRun(ok bool) bool {
+ sq.RLock()
+ defer sq.RUnlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ return ok && sq.runningApps < sq.maxRunningApps
+}
Review Comment:
@craigcondit Actually I removed canRun() method, this file is outdated.
Current canRun logic is inside this method: incAllocatingAcceptedAppsIfCanRun https://github.com/apache/yunikorn-core/pull/429/files#diff-27632d48eb925e150a33bc92370ceaa66c31048018d11ca7a53a0b50ab7250acR1334
Please review and lmk if you have any comments.
--
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
[GitHub] [yunikorn-core] lixmgl commented on pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#issuecomment-1330872155
> > Can we merge this PR to master first then add your PR?
>
> This PR formed the basis under my port. We can merge the ported PR in one go. @craigcondit will handle the proper attribution to you when we commit the change. I just did some cleanup and the port :-)
I see, sounds good. Thanks @wilfred-s and @craigcondit !
--
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
[GitHub] [yunikorn-core] lixmgl commented on a diff in pull request #429: [YUNIKORN-790] Implement MaxApplications enforcement
Posted by GitBox <gi...@apache.org>.
lixmgl commented on code in PR #429:
URL: https://github.com/apache/yunikorn-core/pull/429#discussion_r962361186
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,83 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(ok, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(ok bool, sa *Application) bool {
+ sq.Lock()
+ defer sq.Unlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
Review Comment:
updated
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -143,6 +143,10 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
+ app.queue.incRunningApps()
+ app.queue.decAllocatingAcceptedApps(app)
+ metrics.GetQueueMetrics(app.queuePath).IncQueueApplicationsRunning()
Review Comment:
good catch, removed the one in enter_Starting, kept the one in enter_Running
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,83 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(ok, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(ok bool, sa *Application) bool {
+ sq.Lock()
+ defer sq.Unlock()
+ if sq.maxRunningApps == 0 {
+ return true && ok
+ }
+ result := ok && (sq.runningApps+uint64(len(sq.allocatingAcceptedApps)) < sq.maxRunningApps)
+ if result {
+ _, contains := sq.allocatingAcceptedApps[sa.ApplicationID]
Review Comment:
updated, using ok instead of contains
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
same
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
updated to int64
##########
pkg/scheduler/objects/application_state.go:
##########
@@ -143,6 +143,10 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
app := event.Args[0].(*Application) //nolint:errcheck
+ app.queue.incRunningApps()
+ app.queue.decAllocatingAcceptedApps(app)
+ metrics.GetQueueMetrics(app.queuePath).IncQueueApplicationsRunning()
+ metrics.GetSchedulerMetrics().IncTotalApplicationsRunning()
Review Comment:
same
##########
pkg/scheduler/objects/queue.go:
##########
@@ -1292,3 +1306,83 @@ func (sq *Queue) String() string {
return fmt.Sprintf("{QueuePath: %s, State: %s, StateTime: %x, MaxResource: %s}",
sq.QueuePath, sq.stateMachine.Current(), sq.stateTime, sq.maxResource)
}
+
+func (sq *Queue) incRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.incRunningApps()
+ }
+ sq.internalIncRunningApps()
+}
+
+func (sq *Queue) internalIncRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps++
+}
+
+func (sq *Queue) decRunningApps() {
+ if sq == nil {
+ return
+ }
+ if sq.parent != nil {
+ sq.parent.decRunningApps()
+ }
+ sq.internalDecRunningApps()
+}
+
+func (sq *Queue) internalDecRunningApps() {
+ sq.Lock()
+ defer sq.Unlock()
+ sq.runningApps--
+}
+
+func (sq *Queue) incAllocatingAcceptedAppsIfCanRun(sa *Application) bool {
+ if sq == nil {
+ return false
+ }
+ ok := true
+ if sq.parent != nil {
+ ok = sq.parent.incAllocatingAcceptedAppsIfCanRun(sa)
+ }
+ return sq.InternalIncAllocatingAcceptedAppsIfCanRun(ok, sa)
+}
+
+func (sq *Queue) InternalIncAllocatingAcceptedAppsIfCanRun(ok bool, sa *Application) bool {
Review Comment:
updated, using success instead of ok
##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -49,11 +49,20 @@ func createManagedQueue(parentSQ *Queue, name string, parent bool, maxRes map[st
// create managed queue with props set
func createManagedQueueWithProps(parentSQ *Queue, name string, parent bool, maxRes, props map[string]string) (*Queue, error) {
+ return createManagedQueuePropsMaxApps(parentSQ, name, parent, maxRes, props, uint64(0))
Review Comment:
same
--
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