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/03/10 11:09:01 UTC

[GitHub] [incubator-yunikorn-core] chungen0126 opened a new pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

chungen0126 opened a new pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384


   ### What is this PR for?
   Add the variable for running application and max running application, and make sure that running application will not more than  max running application.
   
   ### What type of PR is it?
   * [ ] - Bug Fix
   * [x] - Improvement
   * [ ] - Feature
   * [ ] - Documentation
   * [ ] - Hot Fix
   * [ ] - Refactoring
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   https://issues.apache.org/jira/browse/YUNIKORN-790
   
   ### How should this be tested?
   
   ### Screenshots (if appropriate)
   
   ### Questions:
   * [ ] - The licenses files need update.
   * [ ] - There is breaking changes for older versions.
   * [ ] - It needs documentation.
   


-- 
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1064874312


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#384](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (33e9753) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/2f7ee5bd43e7cd1f5bd86118be778982b42f8d01?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2f7ee5b) will **decrease** coverage by `0.03%`.
   > The diff coverage is `53.84%`.
   
   > :exclamation: Current head 33e9753 differs from pull request most recent head 4d8a6b1. Consider uploading reports for the commit 4d8a6b1 to get more accurate results
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #384      +/-   ##
   ==========================================
   - Coverage   69.40%   69.37%   -0.04%     
   ==========================================
     Files          66       66              
     Lines        9475     9485      +10     
   ==========================================
   + Hits         6576     6580       +4     
   - Misses       2655     2659       +4     
   - Partials      244      246       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.64% <0.00%> (-0.21%)` | :arrow_down: |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.44% <25.00%> (-0.22%)` | :arrow_down: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `100.00% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2f7ee5b...4d8a6b1](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [incubator-yunikorn-core] pbacsko commented on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1064925779


   You accidentally committed an empty file `yunikorn_state.txt`. That should be removed from the 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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824536474



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -171,6 +175,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("leave_%s", Running.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps--

Review comment:
       Same here, we need a new `DecRunningApps()`
   
   ```
   func (sq *Queue) DecRunningApps() {
   	sq.Lock()
   	defer sq.Unlock()
   	sq.runningApps--
   	if sq.parent != nil {
   		sq.parent.DecRunningApps()
   	}
   }
   ```




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824539305



##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1264,6 +1264,9 @@ func (sa *Application) AddAllocation(info *Allocation) {
 // Add the Allocation to the application
 // No locking must be called while holding the lock
 func (sa *Application) addAllocationInternal(info *Allocation) {
+	if sa.queue != nil && sa.queue.maxRunningApps != 0 && sa.stateMachine.Is(Accepted.String()) && sa.queue.maxRunningApps <= sa.queue.runningApps {
+		return

Review comment:
       I don't think we can just return.
   
   The application cannot continue its transition because `maxRunningApps` has been reached.
   
   Therefore, it has to be saved to a slice along with the current timestamp so that when `queue.runningApps < queue.maxRunningApps` we can manually trigger the state transition.
   
   The timestamp is needed because we want to trigger the app which has been waiting for the longest amount of time.




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824564495



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -171,6 +175,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("leave_%s", Running.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps--

Review comment:
       At this point, we have to update the pending applications and start the one which was blocked first.
   
   Just a very POC-like code:
   ```
   pendingApps []*Applications
   ...
          
   	if app.queue.CanRun() { // is runningApps < maxRunningApps in the hierarchy?
   		sort.Slice(pendingApps, func(i, j int) bool {
   			if pendingApps[i].SubmissionTime.Before(pendingApps[j].SubmissionTime) {
   				return true
   			}
   			return false
   		})
   
   		selected := pendingApps[0]
   		selected.stateMachine.Event(RunApplication.String(), selected)
   	}
   ...
   ```
   
   This is really just a rough idea, need to be tested thoroughly.




-- 
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1064874312


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#384](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (33e9753) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/2f7ee5bd43e7cd1f5bd86118be778982b42f8d01?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2f7ee5b) will **decrease** coverage by `0.03%`.
   > The diff coverage is `53.84%`.
   
   > :exclamation: Current head 33e9753 differs from pull request most recent head 4a62123. Consider uploading reports for the commit 4a62123 to get more accurate results
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #384      +/-   ##
   ==========================================
   - Coverage   69.40%   69.37%   -0.04%     
   ==========================================
     Files          66       66              
     Lines        9475     9485      +10     
   ==========================================
   + Hits         6576     6580       +4     
   - Misses       2655     2659       +4     
   - Partials      244      246       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.64% <0.00%> (-0.21%)` | :arrow_down: |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.44% <25.00%> (-0.22%)` | :arrow_down: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `100.00% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2f7ee5b...4a62123](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824539305



##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1264,6 +1264,9 @@ func (sa *Application) AddAllocation(info *Allocation) {
 // Add the Allocation to the application
 // No locking must be called while holding the lock
 func (sa *Application) addAllocationInternal(info *Allocation) {
+	if sa.queue != nil && sa.queue.maxRunningApps != 0 && sa.stateMachine.Is(Accepted.String()) && sa.queue.maxRunningApps <= sa.queue.runningApps {
+		return

Review comment:
       I don't think we can just return.
   
   The application cannot continue its transition because maxRunningApps is exceeded.
   
   Therefore, it has to be saved to a slice along with the current timestamp so that when `queue.runningApps < queue.maxRunningApps` we can manually trigger the state transition.
   
   The timestamp is needed because we want to trigger the app which has been waiting for the longest amount of time.




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824530694



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -142,6 +143,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps++
+				}

Review comment:
       We have to update the counter in the parent as well.
   




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824578541



##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1264,6 +1264,9 @@ func (sa *Application) AddAllocation(info *Allocation) {
 // Add the Allocation to the application
 // No locking must be called while holding the lock
 func (sa *Application) addAllocationInternal(info *Allocation) {
+	if sa.queue != nil && sa.queue.maxRunningApps != 0 && sa.stateMachine.Is(Accepted.String()) && sa.queue.maxRunningApps <= sa.queue.runningApps {
+		return

Review comment:
       As @wilfred-s mentioned in the JIRA, we have to have a `CanRun()` method in the queue, something like:
   
   ```
   func (sq *Queue) CanRun() {
   	sq.RLock()
   	defer sq.RUnlock()
   	
           if sq.maxRunningApps == 0 {
             return true
           }
   
   	ok := sq.runningApps < sq.maxRunningApps
   	if sq.parent != nil {
   		return ok && sq.parent.CanRun()
   	} else {
   		return ok
   	}
   }
   ```
   
   so then it becomes simpler:
   ```
   if sa.queue != nil && sa.stateMachine.Is(Accepted.String()) && sa.queue.CanRun() {
   ...
   } else {
     // maxRunningApps reached, save the Application
     ...
   }
   ```




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824578541



##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1264,6 +1264,9 @@ func (sa *Application) AddAllocation(info *Allocation) {
 // Add the Allocation to the application
 // No locking must be called while holding the lock
 func (sa *Application) addAllocationInternal(info *Allocation) {
+	if sa.queue != nil && sa.queue.maxRunningApps != 0 && sa.stateMachine.Is(Accepted.String()) && sa.queue.maxRunningApps <= sa.queue.runningApps {
+		return

Review comment:
       As @wilfred-s mentioned in the JIRA, we have to have a `CanRun()` method in the queue, something like:
   
   ```
   func (sq *Queue) CanRun() {
   	sq.RLock()
   	defer sq.RUnlock()
   	
   	ok := sq.runningApps < sq.maxRunningApps
   	if sq.parent != nil {
   		return ok && sq.parent.CanRun()
   	} else {
   		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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1064874312


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#384](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c34491a) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/2f7ee5bd43e7cd1f5bd86118be778982b42f8d01?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2f7ee5b) will **decrease** coverage by `0.03%`.
   > The diff coverage is `53.84%`.
   
   > :exclamation: Current head c34491a differs from pull request most recent head 33e9753. Consider uploading reports for the commit 33e9753 to get more accurate results
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #384      +/-   ##
   ==========================================
   - Coverage   69.40%   69.37%   -0.04%     
   ==========================================
     Files          66       66              
     Lines        9475     9485      +10     
   ==========================================
   + Hits         6576     6580       +4     
   - Misses       2655     2659       +4     
   - Partials      244      246       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.64% <0.00%> (-0.21%)` | :arrow_down: |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.44% <25.00%> (-0.22%)` | :arrow_down: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `100.00% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2f7ee5b...33e9753](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1064874312


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#384](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a62123) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/b0906e9f24cdaedfc0777a1340685be732afb92a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b0906e9) will **decrease** coverage by `0.20%`.
   > The diff coverage is `56.41%`.
   
   > :exclamation: Current head 4a62123 differs from pull request most recent head dff4c63. Consider uploading reports for the commit dff4c63 to get more accurate results
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #384      +/-   ##
   ==========================================
   - Coverage   69.52%   69.31%   -0.21%     
   ==========================================
     Files          67       66       -1     
     Lines        9522     9511      -11     
   ==========================================
   - Hits         6620     6593      -27     
   - Misses       2657     2669      +12     
   - Partials      245      249       +4     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.64% <0.00%> (-0.21%)` | :arrow_down: |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.05% <53.33%> (-0.68%)` | :arrow_down: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `100.00% <100.00%> (ø)` | |
   | [pkg/events/event\_publisher.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2V2ZW50cy9ldmVudF9wdWJsaXNoZXIuZ28=) | `92.59% <0.00%> (-7.41%)` | :arrow_down: |
   | [pkg/webservice/handlers.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3dlYnNlcnZpY2UvaGFuZGxlcnMuZ28=) | `75.81% <0.00%> (-0.11%)` | :arrow_down: |
   | [pkg/common/resources/quantity.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcXVhbnRpdHkuZ28=) | | |
   | [pkg/common/resources/resources.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9yZXNvdXJjZXMvcmVzb3VyY2VzLmdv) | `98.19% <0.00%> (+0.50%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b0906e9...dff4c63](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824564495



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -171,6 +175,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("leave_%s", Running.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps--

Review comment:
       At this point, we have to update the pending applications and start the one which was blocked first.
   
   Just a very POC-like code:
   ```
   pendingApps []*Applications
   ...
          
   	if app.queue.CanRun() { // is runningApps < maxRunningApps in the hierarchy?
                   // TODO: not optimal, a SubmissionTime-based min-Heap is better
   		sort.Slice(pendingApps, func(i, j int) bool {
   			if pendingApps[i].SubmissionTime.Before(pendingApps[j].SubmissionTime) {
   				return true
   			}
   			return false
   		})
   
   		selected := pendingApps[0]
   		selected.stateMachine.Event(RunApplication.String(), selected)
   	}
   ...
   ```
   
   This is really just a rough idea, need to be tested thoroughly.




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824564495



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -171,6 +175,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("leave_%s", Running.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps--

Review comment:
       At this point, we have to update the pending applications and start the one which was blocked first.
   
   Just a very POC-like code:
   ```
   pendingApps []*Applications
   ...
           // TODO: not optimal, a SubmissionTime-based min-Heap is better
   	sort.Slice(pendingApps, func(i, j int) bool {
   		if pendingApps[i].SubmissionTime.Before(pendingApps[j].SubmissionTime) {
   			return true
   		}
   		return false
   	})
   
   	selected := pendingApps[0]
   	selected.stateMachine.Event(RunApplication.String(), selected) // trigger app to run
   ...
   ```
   
   This is really just a rough idea, need to be tested thoroughly.




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824535715



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -142,6 +143,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps++

Review comment:
       We have to update the counter in the parent as well, all the way up to root.
   
   So we need an `incrementRunningApps()` method like:
   ```
   func (sq *Queue) IncrementRunningApps() {
   	sq.Lock()
   	defer sq.Unlock()
   	sq.runningApps++
   	if sq.parent != nil {
   		sq.parent.IncrementRunningApps()		
   	}
   }
   ```




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824535715



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -142,6 +143,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("enter_%s", Starting.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps++

Review comment:
       We have to update the counter in the parent as well, all the way up to root.
   
   So we need an `IncRunningApps()` method like:
   ```
   func (sq *Queue) IncRunningApps() {
   	sq.Lock()
   	defer sq.Unlock()
   	sq.runningApps++
   	if sq.parent != nil {
   		sq.parent.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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r833600994



##########
File path: pkg/scheduler/objects/queue.go
##########
@@ -1292,3 +1299,35 @@ 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() {
+	sq.Lock()
+	defer sq.Unlock()
+	sq.runningApps++
+	if sq.parent != nil {
+		sq.parent.incRunningApps()
+	}
+}
+
+func (sq *Queue) decRunningApps() {
+	sq.Lock()
+	defer sq.Unlock()
+	sq.runningApps--
+	if sq.parent != nil {
+		sq.parent.decRunningApps()
+	}
+}
+
+func (sq *Queue) canRun() bool {
+	sq.RLock()
+	defer sq.RUnlock()
+
+	if sq.maxRunningApps == 0 {
+		return true
+	}
+	ok := sq.runningApps < sq.maxRunningApps
+	if sq.parent != nil {

Review comment:
       Just an idea:
   If `ok` is false, it means that the limit is reached and printing a message is useful.
   I suggest this change:
   
   ```
   ok := sq.runningApps < sq.maxRunningApps
   if !ok {
     log.Logger().Info("Maximum number of runnable apps reached", zap.String("queuePath", sq.QueuePath))
   }
   ```
   
   or maybe we can return earlier:
   ```
   ok := sq.runningApps < sq.maxRunningApps
   if !ok {
     log.Logger().Info("Maximum number of runnable apps reached", zap.String("queuePath", sq.QueuePath))
     return false
   }
   if sq.parent != nil {
     return sq.parent.canRun()
   }
   return true
   ```




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r833600994



##########
File path: pkg/scheduler/objects/queue.go
##########
@@ -1292,3 +1299,35 @@ 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() {
+	sq.Lock()
+	defer sq.Unlock()
+	sq.runningApps++
+	if sq.parent != nil {
+		sq.parent.incRunningApps()
+	}
+}
+
+func (sq *Queue) decRunningApps() {
+	sq.Lock()
+	defer sq.Unlock()
+	sq.runningApps--
+	if sq.parent != nil {
+		sq.parent.decRunningApps()
+	}
+}
+
+func (sq *Queue) canRun() bool {
+	sq.RLock()
+	defer sq.RUnlock()
+
+	if sq.maxRunningApps == 0 {
+		return true
+	}
+	ok := sq.runningApps < sq.maxRunningApps
+	if sq.parent != nil {

Review comment:
       Just an idea:
   If `ok` is false, it means that the limit is reached and printing a message is useful.
   I suggest this change:
   
   ```
   ok := sq.runningApps < sq.maxRunningApps
   if !ok {
     log.Logger().Info("Maximum number of runnable apps reached", zap.String("queue", sq.QueuePath))
   }
   ```
   
   or maybe we can return earlier:
   ```
   ok := sq.runningApps < sq.maxRunningApps
   if !ok {
     log.Logger().Info("Maximum number of runnable apps reached", zap.String("queuePath", sq.QueuePath))
     return false
   }
   if sq.parent != nil {
     return sq.parent.canRun()
   }
   return true
   ```




-- 
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] [incubator-yunikorn-core] codecov[bot] commented on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1064874312


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#384](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c34491a) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/2f7ee5bd43e7cd1f5bd86118be778982b42f8d01?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2f7ee5b) will **decrease** coverage by `0.03%`.
   > The diff coverage is `53.84%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #384      +/-   ##
   ==========================================
   - Coverage   69.40%   69.37%   -0.04%     
   ==========================================
     Files          66       66              
     Lines        9475     9485      +10     
   ==========================================
   + Hits         6576     6580       +4     
   - Misses       2655     2659       +4     
   - Partials      244      246       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.64% <0.00%> (-0.21%)` | :arrow_down: |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.44% <25.00%> (-0.22%)` | :arrow_down: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `100.00% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2f7ee5b...c34491a](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r824539305



##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1264,6 +1264,9 @@ func (sa *Application) AddAllocation(info *Allocation) {
 // Add the Allocation to the application
 // No locking must be called while holding the lock
 func (sa *Application) addAllocationInternal(info *Allocation) {
+	if sa.queue != nil && sa.queue.maxRunningApps != 0 && sa.stateMachine.Is(Accepted.String()) && sa.queue.maxRunningApps <= sa.queue.runningApps {
+		return

Review comment:
       I don't think we can just return.
   
   The application cannot continue its transition because `maxRunningApps` has been reached.
   
   Therefore, it has to be saved to a slice along with the current timestamp (probably `sa.SubmissionTime` is good for us) so that when `queue.runningApps < queue.maxRunningApps` we can manually trigger the state transition.
   
   The timestamp is needed because we want to trigger the app which has been waiting for the longest amount of time, but luckily we have that info inside the `Application` struct.




-- 
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1064874312


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#384](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4a62123) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/2f7ee5bd43e7cd1f5bd86118be778982b42f8d01?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2f7ee5b) will **decrease** coverage by `0.08%`.
   > The diff coverage is `56.41%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #384      +/-   ##
   ==========================================
   - Coverage   69.40%   69.31%   -0.09%     
   ==========================================
     Files          66       66              
     Lines        9475     9511      +36     
   ==========================================
   + Hits         6576     6593      +17     
   - Misses       2655     2669      +14     
   - Partials      244      249       +5     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.64% <0.00%> (-0.21%)` | :arrow_down: |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.05% <53.33%> (-0.61%)` | :arrow_down: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `100.00% <100.00%> (ø)` | |
   | [pkg/events/event\_publisher.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2V2ZW50cy9ldmVudF9wdWJsaXNoZXIuZ28=) | `92.59% <0.00%> (-7.41%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2f7ee5b...4a62123](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r833600994



##########
File path: pkg/scheduler/objects/queue.go
##########
@@ -1292,3 +1299,35 @@ 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() {
+	sq.Lock()
+	defer sq.Unlock()
+	sq.runningApps++
+	if sq.parent != nil {
+		sq.parent.incRunningApps()
+	}
+}
+
+func (sq *Queue) decRunningApps() {
+	sq.Lock()
+	defer sq.Unlock()
+	sq.runningApps--
+	if sq.parent != nil {
+		sq.parent.decRunningApps()
+	}
+}
+
+func (sq *Queue) canRun() bool {
+	sq.RLock()
+	defer sq.RUnlock()
+
+	if sq.maxRunningApps == 0 {
+		return true
+	}
+	ok := sq.runningApps < sq.maxRunningApps
+	if sq.parent != nil {

Review comment:
       Just an idea:
   If `ok` is false, it means that the limit is reached and printing a message is useful.
   I suggest this change:
   
   ```
   ok := sq.runningApps < sq.maxRunningApps
   if !ok {
     log.Logger().Info("Maximum number of runnable apps reached", zap.String("queue", sq.QueuePath))
   }
   ```
   
   or maybe we can return earlier:
   ```
   ok := sq.runningApps < sq.maxRunningApps
   if !ok {
     log.Logger().Info("Maximum number of runnable apps reached", zap.String("queue", sq.QueuePath))
     return false
   }
   if sq.parent != nil {
     return sq.parent.canRun()
   }
   return true
   ```




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r834340153



##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1264,6 +1264,9 @@ func (sa *Application) AddAllocation(info *Allocation) {
 // Add the Allocation to the application
 // No locking must be called while holding the lock
 func (sa *Application) addAllocationInternal(info *Allocation) {

Review comment:
       @chungen0126 
   
   Today I had a discussion and code-checking session with @wilfred-s and our conclusion is that this method is not the right place for this check.
   
   We think that you should put this into `Queue.TryAllocate()` like that:
   
   ```
   func (sq *Queue) TryAllocate(iterator func() NodeIterator) *Allocation {
           if !sq.CanRun() {   // maxApplications reached
               return
           }
   	if sq.IsLeafQueue() {
   		// get the headroom
   		headRoom := sq.getHeadRoom()
   		// process the apps (filters out app without pending requests)
         }
   ...
   ```
   If you can't run at any level, you just leave and return immediately. That way you won't calculate allocations and update data structures.
   
   However, this has an effect on `CanRun()` because in `TryAllocate()` we're walking down the hierarchy from "root", rather than checking from a particular leaf queue.
   
   So I think we need something like:
   ```
   func (sq *Queue) CanRun() bool {
   	sq.RLock()
   	defer sq.RUnlock()
   
   	if sq.maxRunningApps == 0 {
   		return true
   	}
   	
   	return sq.runningApps < sq.maxRunningApps
   }
   ```
   
   So we don't need to check the parent all the way back to "root", because at any given level, we already know that. Eg if we're at "root.users.dev", we already know that "root.users" can accept new applications, because we're updating the counters constantly.
   
   I hope this makes sense.




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r834340153



##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1264,6 +1264,9 @@ func (sa *Application) AddAllocation(info *Allocation) {
 // Add the Allocation to the application
 // No locking must be called while holding the lock
 func (sa *Application) addAllocationInternal(info *Allocation) {

Review comment:
       @chungen0126 
   
   Today I had a discussion and code-checking session with @wilfred-s and our conclusion is that this method is not the right place for this check.
   
   We think that you should put this into `Queue.TryAllocate()` like that:
   
   ```
   func (sq *Queue) TryAllocate(iterator func() NodeIterator) *Allocation {
           if !sq.CanRun() {   // maxApplications reached
               return
           }
   	if sq.IsLeafQueue() {
   		// get the headroom
   		headRoom := sq.getHeadRoom()
   		// process the apps (filters out app without pending requests)
   ...
   ```
   If you can't run at any level, you just leave and return immediately. That way you won't calculate allocations and update data structures.
   
   However, this has an effect on `CanRun()` because in `TryAllocate()` we're walking down the hierarchy from "root", rather than checking from a particular leaf queue.
   
   So I think we need something like:
   ```
   func (sq *Queue) CanRun() bool {
   	sq.RLock()
   	defer sq.RUnlock()
   
   	if sq.maxRunningApps == 0 {
   		return true
   	}
   	
   	return sq.runningApps < sq.maxRunningApps
   }
   ```
   
   So we don't need to check the parent all the way back to "root", because at any given level, we already know that. Eg if we're at "root.users.dev", we already know that "root.users" can accept new applications, because we're updating the counters constantly.
   
   I hope this makes sense.




-- 
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1064874312


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#384](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (33e9753) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/2f7ee5bd43e7cd1f5bd86118be778982b42f8d01?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2f7ee5b) will **decrease** coverage by `0.03%`.
   > The diff coverage is `53.84%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/graphs/tree.svg?width=650&height=150&src=pr&token=SB9NrIi3Hy&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #384      +/-   ##
   ==========================================
   - Coverage   69.40%   69.37%   -0.04%     
   ==========================================
     Files          66       66              
     Lines        9475     9485      +10     
   ==========================================
   + Hits         6576     6580       +4     
   - Misses       2655     2659       +4     
   - Partials      244      246       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.64% <0.00%> (-0.21%)` | :arrow_down: |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.44% <25.00%> (-0.22%)` | :arrow_down: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `100.00% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2f7ee5b...33e9753](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [incubator-yunikorn-core] chungen0126 commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
chungen0126 commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r826667483



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -171,6 +175,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("leave_%s", Running.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps--

Review comment:
       I think that it just needs to be blocked and doesn't need to start here. It will still be allocated first by the rmEventHandler.




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r834340153



##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1264,6 +1264,9 @@ func (sa *Application) AddAllocation(info *Allocation) {
 // Add the Allocation to the application
 // No locking must be called while holding the lock
 func (sa *Application) addAllocationInternal(info *Allocation) {

Review comment:
       @chungen0126 
   
   Today I had a discussion and code-checking session with @wilfred-s and our conclusion is that this method is not the right place for this check.
   
   We think that you should put this into `Queue.TryAllocate()` like that:
   
   ```
   func (sq *Queue) TryAllocate(iterator func() NodeIterator) *Allocation {
           if !sq.CanRun() {   // maxApplications reached
               log.Logger().Info("maxApplications reached", zap.String("queuePath", sq.GetQueuePath()))
               return
           }
   	if sq.IsLeafQueue() {
   		// get the headroom
   		headRoom := sq.getHeadRoom()
   		// process the apps (filters out app without pending requests)
   ...
   ```
   If you can't run at any level, you just leave and return immediately. That way you won't calculate allocations and update data structures.
   
   However, this has an effect on `CanRun()` because in `TryAllocate()` we're walking down the hierarchy from "root", rather than checking from a particular leaf queue.
   
   So I think we need something like:
   ```
   func (sq *Queue) CanRun() bool {
   	sq.RLock()
   	defer sq.RUnlock()
   
   	if sq.maxRunningApps == 0 {
   		return true
   	}
   	
   	return sq.runningApps < sq.maxRunningApps
   }
   ```
   
   So we don't need to check the parent all the way back to "root", because at any given level, we already know that. Eg if we're at "root.users.dev", we already know that "root.users" can accept new applications, because we're updating the counters constantly.
   
   I hope this makes sense.




-- 
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] [incubator-yunikorn-core] codecov[bot] edited a comment on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1064874312


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#384](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dff4c63) into [master](https://codecov.io/gh/apache/incubator-yunikorn-core/commit/b0906e9f24cdaedfc0777a1340685be732afb92a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b0906e9) will **decrease** coverage by `0.06%`.
   > The diff coverage is `56.41%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #384      +/-   ##
   ==========================================
   - Coverage   69.52%   69.46%   -0.07%     
   ==========================================
     Files          67       67              
     Lines        9522     9558      +36     
   ==========================================
   + Hits         6620     6639      +19     
   - Misses       2657     2670      +13     
   - Partials      245      249       +4     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/scheduler/objects/application.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uLmdv) | `57.64% <0.00%> (-0.21%)` | :arrow_down: |
   | [pkg/scheduler/objects/queue.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `70.12% <53.33%> (-0.61%)` | :arrow_down: |
   | [pkg/scheduler/objects/application\_state.go](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2FwcGxpY2F0aW9uX3N0YXRlLmdv) | `100.00% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [b0906e9...dff4c63](https://codecov.io/gh/apache/incubator-yunikorn-core/pull/384?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r833598542



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -171,6 +175,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("leave_%s", Running.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps--

Review comment:
       @chungen0126 Have you tried it?
   
   I think these changes first need to be tested on sample setup with KIND/Minikube to see what happens.
   
   Could you do that? I would like to know what happens.




-- 
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] [incubator-yunikorn-core] pbacsko commented on a change in pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on a change in pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#discussion_r833598542



##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -171,6 +175,9 @@ func NewAppState() *fsm.FSM {
 			},
 			fmt.Sprintf("leave_%s", Running.String()): func(event *fsm.Event) {
 				app := event.Args[0].(*Application) //nolint:errcheck
+				if app.queue != nil {
+					app.queue.runningApps--

Review comment:
       Have you tried it?
   
   I think these changes first need to be tested on sample setup with KIND/Minikube to see what happens.
   
   Could you do that? I would like to know what happens.




-- 
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] [incubator-yunikorn-core] pbacsko commented on pull request #384: [YUNIKORN-790] Implement MaxApplications enforcement

Posted by GitBox <gi...@apache.org>.
pbacsko commented on pull request #384:
URL: https://github.com/apache/incubator-yunikorn-core/pull/384#issuecomment-1076683976


   Next steps:
   
   1) Try out this PR on a test cluster. Just submit sleep pods to a queue with a limit.
   2) Fix issues if there's any.
   3) Write unit tests.


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