wilfred-s commented on a change in pull request #260:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/260#discussion_r596645108
##########
File path: pkg/scheduler/objects/application_state_test.go
##########
@@ -39,10 +42,11 @@ func TestAcceptStateTransition(t *testing.T) {
assert.Assert(t, err != nil, "error expected accepted to rejected")
assert.Equal(t, app.CurrentState(), Accepted.String())
- // accepted to killed
+ // accepted to failed
err = app.HandleApplicationEvent(FailApplication)
assert.NilError(t, err, "no error expected accepted to killed")
Review comment:
message should be failed
##########
File path: pkg/webservice/handlers_test.go
##########
@@ -690,9 +692,9 @@ func TestPartitions(t *testing.T) {
assert.Assert(t, err, "no error expected new to accepted")
// app4: accepted to wait
- err = app4.HandleApplicationEvent(objects.WaitApplication)
+ err = app4.HandleApplicationEvent(objects.CompleteApplication)
Review comment:
We have `Application.SetState()` instead of moving from state to state
we can just set whatever we want/need. Simplifies this codes and we're not
dependent on the state ordering.
##########
File path: pkg/scheduler/objects/application_state_test.go
##########
@@ -87,7 +91,8 @@ func TestStartStateTransition(t *testing.T) {
// start to killed
err = appInfo.HandleApplicationEvent(FailApplication)
assert.NilError(t, err, "no error expected starting to killed")
Review comment:
comment and message should be updated to failed
multiple references to killed left, not tagged all of them
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -1303,3 +1281,52 @@ func (sa *Application)
notifyRMAllocationAskReleased(rmID string, released []*Al
}
sa.rmEventHandler.HandleEvent(releaseEvent)
}
+
+func (sa *Application) failAppIfPossible() {
+ if sa.isApplicationEmpty(false) {
+ // we need to run this in a different go routine to avoid
deadlock in sm
+ // nolint: errcheck
+ go sa.HandleApplicationEvent(FailApplication)
+ }
+}
+
+func (sa *Application) isApplicationEmpty(skipPlaceholders bool) bool {
+ if skipPlaceholders {
+ return resources.IsZero(sa.pending) &&
resources.IsZero(sa.allocatedResource)
+ }
+ return resources.IsZero(sa.pending) &&
resources.IsZero(sa.allocatedResource) &&
resources.IsZero(sa.allocatedPlaceholder)
+}
+
+// lock free call, this should be called when holding a lock on the
application object
+func (sa *Application) handleStateChange() {
Review comment:
This looks really complex. We have discussed this off-line and come to
a nicer solution.
Please update with the discussed changes.
##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -148,6 +152,9 @@ func NewAppState() *fsm.FSM {
app.executeTerminatedCallback()
app.clearPlaceholderTimer()
},
+ fmt.Sprintf("enter_%s", Failing.String()): func(event
*fsm.Event) {
+ event.Args[0].(*Application).failAppIfPossible()
+ },
fmt.Sprintf("enter_%s", Failed.String()): func(event
*fsm.Event) {
app := setTimer(completedTimeout, event,
ExpireApplication)
Review comment:
rename to `terminatedTimeout` or something like that as it is not just
used for completed but also failed.
##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -127,7 +131,7 @@ func NewAppState() *fsm.FSM {
fmt.Sprintf("enter_%s", Starting.String()): func(event
*fsm.Event) {
setTimer(startingTimeout, event, RunApplication)
},
- fmt.Sprintf("enter_%s", Waiting.String()): func(event
*fsm.Event) {
+ fmt.Sprintf("enter_%s", Completing.String()):
func(event *fsm.Event) {
setTimer(waitingTimeout, event,
CompleteApplication)
Review comment:
rename to completingTimeout (also in the application)
----------------------------------------------------------------
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]
