wilfred-s commented on code in PR #1122:
URL: https://github.com/apache/yunikorn-core/pull/1122#discussion_r3772067906
##########
pkg/scheduler/objects/application.go:
##########
@@ -877,11 +933,19 @@ func (sa *Application) deallocateAsk(ask *Allocation)
(*resources.Resource, erro
return nil, fmt.Errorf("unable to deallocate pending ask %s on
app %s", ask.GetAllocationKey(), sa.ApplicationID)
}
- askPriority := ask.GetPriority()
- if askPriority > sa.askMaxPriority {
- // increase app priority
- sa.askMaxPriority = askPriority
- sa.queue.UpdateApplicationPriority(sa.ApplicationID,
askPriority)
+ // The ask returns to the pending set, but only if it still IS this
application's ask: an ask that
+ // has already been dropped from sa.requests must not be counted again.
That is reachable today:
+ // removeAsksInternal("") wipes sa.requests while leaving
sa.allocations intact until the shim
+ // confirms the releases, and a release arriving in that window reaches
RollbackAllocation, which
+ // finds the entry in sa.allocations and deallocates it. The identity
comparison (not just a
+ // presence check) also covers a stale ask object that has since been
replaced by a new ask under
+ // the same key.
+ // This matches the converged behaviour of the full rescan this change
replaced:
+ // updateAskMaxPriority derived the max by scanning sa.requests, so an
ask absent from sa.requests
+ // never influenced it. Without the guard that pre-existing accounting
drift would turn into a
+ // permanent leak in the incremental histogram instead.
+ if sa.requests[ask.GetAllocationKey()] == ask {
+ sa.incPendingPriority(ask.GetPriority())
}
Review Comment:
> The odd one out seems to be RollbackAllocation, which takes its ask from
sa.allocations and never consults sa.requests, so the invariant isn't enforced
there the way it is elsewhere - is that something we need to worry about?
we should check that the key is in requests in `RolbackAllocation`. The
current behaviour is that as long as the pod exists on K8s the reference is
maintained in `requests`. However we should not blindly assume that as it would
cause the pod to be ignored in late scheduling cycles.
Filed a jira
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
