kingamarton commented on a change in pull request #261:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/261#discussion_r598700252
##########
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:
Why do we need here the `sa.IsCompleting()` check?
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.
If we want to keep that check here, the state timer must be cleared.
##########
File path: pkg/scheduler/objects/application_test.go
##########
@@ -19,16 +19,49 @@
package objects
import (
+ "math"
"strconv"
"testing"
"time"
+ "github.com/apache/incubator-yunikorn-core/pkg/common/security"
+ "github.com/apache/incubator-yunikorn-core/pkg/rmproxy/rmevent"
+ "github.com/apache/incubator-yunikorn-scheduler-interface/lib/go/si"
"gotest.tools/assert"
"github.com/apache/incubator-yunikorn-core/pkg/common"
"github.com/apache/incubator-yunikorn-core/pkg/common/resources"
)
+// basic app creating with timeout checks
+func TestNewApplication(t *testing.T) {
Review comment:
Please rename this test case to reflect what actually is tested, instead
of adding explanations in the comment: something like
TestAppPlaceholderTimeoutSetting
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1308,14 +1334,3 @@ func (sa *Application)
notifyRMAllocationAskReleased(rmID string, released []*Al
}
sa.rmEventHandler.HandleEvent(releaseEvent)
}
-
-// Auto progress the application when it enters the Failing state if there is
nothing to clean up.
-// Since this is called by the _locked_ state machine while processing an
event we cannot call back
-// into the statemachine directly and we need a go routine to avoid a deadlock.
-func (sa *Application) failAppIfPossible() {
- if resources.IsZero(sa.pending) &&
resources.IsZero(sa.allocatedResource) &&
resources.IsZero(sa.allocatedPlaceholder) {
- // The event handling cannot fail
- //nolint: errcheck
- go sa.HandleApplicationEvent(FailApplication)
- }
-}
Review comment:
I think we will need this change, because we want to fail the apps if
there is a partition removed. In this case we might have an application with no
allocations and asks, so if we are not moving it from Failing to Failed, it
will be stuck into Failing state.
##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -152,9 +152,6 @@ func NewAppState() *fsm.FSM {
app.executeTerminatedCallback()
app.clearPlaceholderTimer()
},
- fmt.Sprintf("enter_%s", Failing.String()): func(event
*fsm.Event) {
- event.Args[0].(*Application).failAppIfPossible()
- },
Review comment:
We need this for handling the case when there is a partition removed.
(see my previous comment)
##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -99,7 +99,7 @@ func NewAppState() *fsm.FSM {
Dst: Completed.String(),
}, {
Name: FailApplication.String(),
- Src: []string{Accepted.String(), New.String(),
Running.String(), Starting.String(), Completing.String()},
+ Src: []string{New.String(), Accepted.String(),
Starting.String(), Running.String()},
Review comment:
I think we need to allow the transition from Completing to Failed as
well. If there is a partition removed, we are failing all the apps from that
partition. We can have some apps in Completing state as well.
--
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]
