yangwwei commented on a change in pull request #244:
URL:
https://github.com/apache/incubator-yunikorn-k8shim/pull/244#discussion_r596605678
##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -88,6 +88,27 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response
*si.UpdateResponse)
}
}
+ // handle status changes
+ for _, updated := range response.UpdatedApplications {
+ log.Logger().Debug("status update callback received",
+ zap.String("appId", updated.ApplicationID),
+ zap.String("new status", updated.State))
+ // delete application from context
+ if updated.State == events.States().Application.Completed {
+ err :=
callback.context.RemoveApplicationInternal(updated.ApplicationID)
+ if err != nil {
+ log.Logger().Error("failed to delete
application", zap.Error(err))
+ }
+ } else {
+ if updated.State == "Failing" || updated.State ==
events.States().Application.Failed {
+ ev :=
cache.NewFailApplicationEvent(updated.ApplicationID)
+ dispatcher.Dispatch(ev)
+ }
+ // handle status update
+
dispatcher.Dispatch(cache.NewApplicationStatusChangeEvent(updated.ApplicationID,
events.AppStateChange, updated.State))
+ }
+ }
+
Review comment:
I do not think this order matters, can we move it back?
##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -88,6 +88,27 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response
*si.UpdateResponse)
}
}
+ // handle status changes
+ for _, updated := range response.UpdatedApplications {
+ log.Logger().Debug("status update callback received",
+ zap.String("appId", updated.ApplicationID),
+ zap.String("new status", updated.State))
+ // delete application from context
+ if updated.State == events.States().Application.Completed {
+ err :=
callback.context.RemoveApplicationInternal(updated.ApplicationID)
+ if err != nil {
+ log.Logger().Error("failed to delete
application", zap.Error(err))
+ }
+ } else {
+ if updated.State == "Failing" || updated.State ==
events.States().Application.Failed {
Review comment:
do we need to handle the Failing state in the shim side? If I understand
correctly, when the core side puts an app into Failing, it will no longer
schedule any of its asks. Why we need this change?
----------------------------------------------------------------
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]
