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/11/07 05:38:41 UTC
[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #441: [YUNIKORN-1328] Handle application state changes and trigger tracker interfaces
wilfred-s commented on code in PR #441:
URL: https://github.com/apache/yunikorn-core/pull/441#discussion_r1015002409
##########
pkg/scheduler/objects/application.go:
##########
@@ -1507,7 +1532,14 @@ func (sa *Application) removeAllocationInternal(uuid string) *Allocation {
var eventWarning string
// update correct allocation tracker
if alloc.IsPlaceholder() {
+ // as and when every ph gets removed (for replacement), resource usage would be reduced.
+ // When real allocation happens as part of replacement, usage would be increased again with real alloc resource
+ removeApp := false
sa.allocatedPlaceholder = resources.Sub(sa.allocatedPlaceholder, alloc.GetAllocatedResource())
+ if resources.IsZero(sa.allocatedPlaceholder) {
+ removeApp = true
Review Comment:
Make sure this is correct: if we call `removeAllocationInternal()` when we time out the placeholders that have not been allocated yet we _might_ remove the app from tracking which is incorrect.
We need to account for the same kind of cases as below with Fail and Run event.
##########
pkg/scheduler/objects/application.go:
##########
@@ -1507,7 +1532,14 @@ func (sa *Application) removeAllocationInternal(uuid string) *Allocation {
var eventWarning string
// update correct allocation tracker
if alloc.IsPlaceholder() {
+ // as and when every ph gets removed (for replacement), resource usage would be reduced.
+ // When real allocation happens as part of replacement, usage would be increased again with real alloc resource
+ removeApp := false
sa.allocatedPlaceholder = resources.Sub(sa.allocatedPlaceholder, alloc.GetAllocatedResource())
+ if resources.IsZero(sa.allocatedPlaceholder) {
Review Comment:
Same check is happening below, please make merge the change into one check
--
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