kingamarton commented on a change in pull request #261:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/261#discussion_r599471071
##########
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:
Even if there is no reason to assume that the timeouts will follow
quickly, it can easily happen.
- Let's say we timeout the placeholders, and send the placeholder release
request to the shim. As we know it can take a longer period for the shim to
remove them, so in the core we will still have the placeholders marked as
removed, until we have an answer from the shim.
- Right after that, the completing state times out(before we can get the
replacement fro the shim). Since there we don't have this check for the already
released placeholders, the core will send the remaining placeholders to the
shim again.
So I think we need to mark the placeholders as released when we timeout the
Completing state as well, and also we need to clear the completing state timer
if we timeout the placeholders. There is no reason to keep it running until it
will timeout.
--
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]
