wilfred-s commented on a change in pull request #230:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/230#discussion_r557073131
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -169,37 +183,41 @@ func (sa *Application) OnStateChange(event *fsm.Event) {
// Set the starting timer to make sure the application will not get stuck in a
starting state too long.
// This prevents an app from not progressing to Running when it only has 1
allocation.
// Called when entering the Starting state by the state machine.
-func (sa *Application) SetStartingTimer() {
- log.Logger().Debug("Application Starting state timer initiated",
+func (sa *Application) SetStateTimer(timeout time.Duration, currentState
string, event applicationEvent) {
+ log.Logger().Debug("Application state timer initiated",
zap.String("appID", sa.ApplicationID),
- zap.Duration("timeout", startingTimeout))
- sa.stateTimer = time.AfterFunc(startingTimeout, sa.timeOutStarting)
+ zap.String("state", sa.stateMachine.Current()),
+ zap.Any("timeout", timeout))
+
+ sa.stateTimer = time.AfterFunc(timeout, sa.timeoutTimer(currentState,
event))
+}
+
+func (sa *Application) timeoutTimer(expectedState string, event
applicationEvent) func() {
+ return func() {
+ // make sure we are still in the right state
+ // we could have been killed or something might have happened
while waiting for a lock
+ if expectedState == sa.stateMachine.Current() {
+ log.Logger().Debug("Application state: auto progress",
+ zap.String("applicationID", sa.ApplicationID),
+ zap.String("state", sa.stateMachine.Current()))
+
+ //nolint: errcheck
+ _ = sa.HandleApplicationEvent(event)
+ }
+ }
}
// Clear the starting timer. If the application has progressed out of the
starting state we need to stop the
// timer and clean up.
// Called when leaving the Starting state by the state machine.
-func (sa *Application) ClearStartingTimer() {
+func (sa *Application) ClearStateTimer() {
Review comment:
We should not need to export this, seems wrong in the old code already.
##########
File path: pkg/scheduler/objects/application.go
##########
@@ -169,37 +183,41 @@ func (sa *Application) OnStateChange(event *fsm.Event) {
// Set the starting timer to make sure the application will not get stuck in a
starting state too long.
// This prevents an app from not progressing to Running when it only has 1
allocation.
// Called when entering the Starting state by the state machine.
-func (sa *Application) SetStartingTimer() {
- log.Logger().Debug("Application Starting state timer initiated",
+func (sa *Application) SetStateTimer(timeout time.Duration, currentState
string, event applicationEvent) {
Review comment:
We should not need to export this, seems wrong in the old code already.
##########
File path: pkg/scheduler/partition.go
##########
@@ -49,6 +49,7 @@ type PartitionContext struct {
// Private fields need protection
root *objects.Queue // start of the
queue hierarchy
applications map[string]*objects.Application // applications
assigned to this partition
+ completedApplications map[string]*objects.Application // completed
applications from this partition
Review comment:
We have had a long discussion on this and we have come to the conclusion
that my request to split up the map was not correct.
However I am missing something to make this work: when we enter the
`Completed` state we should disconnect the queue from app object and the app
from the queue object. This can be driven from the state machine.
##########
File path: pkg/scheduler/partition.go
##########
@@ -1112,3 +1123,25 @@ func (pc *PartitionContext) removeAllocationAsk(appID
string, allocationKey stri
}
}
}
+
+// Move all the completed apps into the completedApp list
+// Delete all the applications marked for removal
+func (pc *PartitionContext) cleanupApps() {
Review comment:
This we can now drive from the state change since we remove the second
map.
##########
File path: pkg/scheduler/partition.go
##########
@@ -922,6 +923,16 @@ func (pc *PartitionContext) GetApplications()
[]*objects.Application {
return appList
}
+func (pc *PartitionContext) GetCompletedApplications() []*objects.Application {
+ pc.RLock()
+ defer pc.RUnlock()
+ var appList []*objects.Application
+ for _, app := range pc.completedApplications {
+ appList = append(appList, app)
Review comment:
this needs to become a filtering loop as we only have one map: we can
directly filter on the `Expired` state if I am correct
##########
File path: pkg/scheduler/objects/application_state.go
##########
@@ -134,7 +144,15 @@ func NewAppState() *fsm.FSM {
},
fmt.Sprintf("enter_%s", Completed.String()): func(event
*fsm.Event) {
metrics.GetSchedulerMetrics().IncTotalApplicationsCompleted()
+ setTimer(completedTimeout, event,
deleteApplication)
Review comment:
We also need to disconnect the app from the queue and vice versa:
```
app.unSetQueue()
```
And in the app have unSetQueue() as:
```
// remove the leaf queue the application runs in, used when completing the
app
func (sa *Application) unSetQueue() {
sa.queue.RemoveApplication()
sa.Lock()
defer sa.Unlock()
sa.queue = nil
}
```
This should be done in combination with the removal of completedApplications
in the PartitionContext
----------------------------------------------------------------
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]
