tigerquoll commented on code in PR #1122:
URL: https://github.com/apache/yunikorn-core/pull/1122#discussion_r3758080213
##########
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:
@wilfred-s Some overly-cautious checks here due to me not being completely
familiar with the code base.
`DeallocateAsk` does the same check, and you're right that this guard is
redundant because of that - it passes the object it just found, so identity
holds by construction. Same for the two revert points in
`tryPlaceholderAllocate`, which source from `sortedRequests`. 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?
I couldn't find a path in production code where the ask is still in
`sa.allocations` when it has been removed from `sa.requests`. The gang-timeout
route I traced is blocked: the outstanding placeholder keeps
`allocatedPlaceholder` non-zero, which holds the app in Resuming, and
`RollbackAllocation` rejects anything that isn't Accepted or Running.
The reason I added it: master bumps `askMaxPriority` up in `deallocateAsk`
with no check either, but there it self-corrects - the value only ever moves up
incrementally, and `updateAskMaxPriority` recomputes it wholesale from
sa.requests whenever the maximum could drop. The histogram has no equivalent
recompute, so anything it accumulates is permanent.
If you're sure this can't be an issue then I will remove the 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
