[GitHub] [incubator-yunikorn-core] yangwwei commented on a change in pull request #192: [YUNIKORN-204][App CRD] Implement basic controller logic

[GitBox <gi...@apache.org>]

yangwwei commented on a change in pull request #192:
URL: https://github.com/apache/incubator-yunikorn-core/pull/192#discussion_r466693023



##########
File path: pkg/cache/application_info.go
##########
@@ -112,6 +127,23 @@ func (ai *ApplicationInfo) IsWaiting() bool {
 // The state machine handles the locking.
 func (ai *ApplicationInfo) HandleApplicationEvent(event applicationEvent) error {
 	err := ai.stateMachine.Event(event.String(), ai)
+	updatedApps := make([]*si.UpdatedApplication, 0)
+	updatedApps = append(updatedApps, &si.UpdatedApplication{
+		ApplicationID: ai.ApplicationID,
+		State:         ai.stateMachine.Current(),
+		StateTransitionTimestamp: time.Now().UnixNano(),
+		Message: fmt.Sprintf("{Status change triggered by the event : %s}",event.String()),
+	})
+
+	if err ==nil && ai.statusChangeEventHandler != nil {
+	ai.statusChangeEventHandler.HandleEvent(
+		&rmevent.RMApplicationUpdateEvent{
+			RmID:                 ai.rmId,
+			AcceptedApplications: make([]*si.AcceptedApplication, 0),
+			RejectedApplications: make([]*si.RejectedApplication, 0),
+			UpdatedApplications: updatedApps,
+		})
+	}

Review comment:
       do not handle this here, we can add this to the callback while state changes. https://github.com/apache/incubator-yunikorn-core/blob/a4a44d7e05d16aa712e1777c85f09e4d55466830/pkg/cache/application_state.go#L102-L108.
   
   This is called whenever there is a state change
   > // 6. enter_state - called after entering all states
   
   Like the rest of callbacks in `application_state.go`, you can create a function in `application_info.go`, such as
   
   ```
   func (ai *ApplicationInfo) onStateChange() {
     log.Logger().Debug("Application state transition",
       zap.String("appID", event.Args[0].(*ApplicationInfo).ApplicationID),
       zap.String("source", event.Src),
       zap.String("destination", event.Dst),
       zap.String("event", event.Event))
   
     // TODO : move the event handling here
   } 
   ```
   

##########
File path: pkg/cache/application_info.go
##########
@@ -52,9 +55,21 @@ type ApplicationInfo struct {
 	stateMachine      *fsm.FSM                   // application state machine
 	stateTimer        *time.Timer                // timer for state time
 
+	//TODO create a struct for it
+	statusChangeEventHandler commonevents.EventHandler

Review comment:
       change this to
   
   ```
   eventHandlers handler.EventHandlers
   ```
   
   just to keep consistency with `cluster_info`, `scheduler` etc




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