[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #230: [YUNIKORN-484] Added waiting state timeout

[GitBox]

yangwwei commented on pull request #230:
URL: 
https://github.com/apache/incubator-yunikorn-core/pull/230#issuecomment-755907468


   > We can only ever have one state timer in an application there is no need 
for multiple state timers as an application can only be in one state at a time. 
The timer is an app instance variable so we always need to call using the app 
instance.
   
   This is a good point. I was saying the same too. We can store a common timer 
instance in the `StateTimers` struct. And then all the impl details can be 
hidden in this struct. I was suggesting this way because I think this is 1) 
easier to read; 2) common impl and can be further re-used by other state 
machines. However, it is totally up to you and @kingamarton which way to 
implement this. I should be fine with either approach. Thanks.



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:
us...@infra.apache.org

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #230: [YUNIKORN-484] Added waiting state timeout

[GitBox]

yangwwei commented on pull request #230:
URL: 
https://github.com/apache/incubator-yunikorn-core/pull/230#issuecomment-755833895


   hi @kingamarton  thanks for the update. I think this can work,  but we maybe 
able to improve this further.
   The idea is to abstract the common things into a struct, something like:
   
   We can define a `StateTimers` and `StateTimer` type like the following:
   ```
   type StateTimers map[applicationState]StateTimer
   type StateTimer struct {
 timeout time.Duration
 callback func() // executes when it times out
   }
   
   func (s *StateTimer) Start() {}
   func (s *StateTimer) Stop() {}
   
   ```
   
   then we can init the stateTimers in each app like
   
   ```
   Application{
 ...
 stateTimers : StateTimers{
Waiting : {
timeout: 3*time.Second,
callback: func() {
// do something
},
},
}
   }
   ```
   
   and then in the state machine, we can simplify the code to something like:
   
   ```
   // this gets executed in every state transition
   fsm.Callbacks{
 "enter_state": func(event *fsm.Event) {
 // blablabla...
 if st, ok := stateTimers[currentState]; ok {
 // a state timer was set
 st.start()
 }
 },
   }
   ```
   
   similarly to the "leave_state" callbacks.
   If we can do this, this can be further utilized by other state machines, 
which gives us a general way to handle state timeouts. Do you think this makes 
sense to you? 
   



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:
us...@infra.apache.org

[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #230: [YUNIKORN-484] Added waiting state timeout

[GitBox]

yangwwei commented on pull request #230:
URL: 
https://github.com/apache/incubator-yunikorn-core/pull/230#issuecomment-754132731


   hi @kingamarton  is this still a draft? could you pls update this. you can 
run some tests with your changes and see if these works end to end. The shim 
changes should have done in https://issues.apache.org/jira/browse/YUNIKORN-478.



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:
us...@infra.apache.org