wilfred-s commented on a change in pull request #261:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/261#discussion_r599167454
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -266,12 +266,29 @@ func (sa *Application) timeoutPlaceholderProcessing() {
defer sa.Unlock()
switch {
// Case 1: if all app's placeholders are allocated, only part of them
gets replaced, just delete the remaining placeholders
- case (sa.IsRunning() || sa.IsStarting()) &&
!resources.IsZero(sa.allocatedPlaceholder):
+ case (sa.IsRunning() || sa.IsStarting() || sa.IsCompleting()) &&
!resources.IsZero(sa.allocatedPlaceholder):
Review comment:
> If the application is in completing state that means that the
completing timer is started, so if it will timeout right after the placeholder
timer, we might send the same allocation release request to the shim twice.
There is no reason to assume that the timeouts will . The placeholder timer
could time out 1 sec after entering the completing state.
What this check does is that it only removes placeholders that have not been
replaced yet. If we move into the _Completing_ state we can do that with
placeholder still allocated on the app. Any of those placeholders that have
been marked as `released` have been communicated to the shim already for a
replacement. They have a real allocation linked to it which is not confirmed
yet as the core is waiting for the shim to confirm the release. If the
replacement comes back from the shim we add the real allocation to the
application. At that point we move back out of _Completing_ and we move back
into the _Running_ state. As soon as that happens the timing out of the state
_Completing_ will become a noop as the state is not the expected state.
That is the specific case we're trying to solve. The case 1 comment should
be updated to cover all the use cases properly.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
