yangwwei commented on a change in pull request #245:
URL:
https://github.com/apache/incubator-yunikorn-k8shim/pull/245#discussion_r606749104
##########
File path: pkg/cache/application_test.go
##########
@@ -177,6 +263,32 @@ func (ms *mockSchedulerAPI) ReloadConfiguration(rmID
string) error {
return nil
}
+type mockRecorder struct {
Review comment:
Pls rename this struct to `MockedRecorder` and move it to a separate
file and put it into
https://github.com/apache/incubator-yunikorn-k8shim/tree/master/pkg/common/events
package. So in the future we can easily reuse it.
##########
File path: pkg/cache/application_test.go
##########
@@ -177,6 +263,32 @@ func (ms *mockSchedulerAPI) ReloadConfiguration(rmID
string) error {
return nil
}
+type mockRecorder struct {
+ updateFn func()
Review comment:
pls rename updateFn to `OnEventf()`, which means this is to be called
when `Eventf()` is called
##########
File path: pkg/cache/application.go
##########
@@ -506,13 +507,33 @@ func (app *Application) onReservationStateChange(event
*fsm.Event) {
func (app *Application) handleRejectApplicationEvent(event *fsm.Event) {
log.Logger().Info("app is rejected by scheduler", zap.String("appID",
app.applicationID))
// for rejected apps, we directly move them to failed state
- dispatcher.Dispatch(NewFailApplicationEvent(app.applicationID))
+ dispatcher.Dispatch(NewFailApplicationEvent(app.applicationID,
+ fmt.Sprintf("application %s is rejected by scheduler",
app.applicationID)))
}
func (app *Application) handleCompleteApplicationEvent(event *fsm.Event) {
// TODO app lifecycle updates
}
+func (app *Application) handleFailApplicationEvent(event *fsm.Event) {
+ eventArgs := make([]string, 1)
+ if err := events.GetEventArgsAsStrings(eventArgs, event.Args); err !=
nil {
+ log.Logger().Error("fail to parse event arg", zap.Error(err))
+ return
+ }
+ errMess := eventArgs[0]
+ // unallocated task states include New, Pending and Scheduling
+ unalloc := app.getTasks(events.States().Task.New)
+ unalloc = append(unalloc, app.getTasks(events.States().Task.Pending)...)
+ unalloc = append(unalloc,
app.getTasks(events.States().Task.Scheduling)...)
+ // publish pod level event to unallocated pods
+ for _, task := range unalloc {
+ events.GetRecorder().Eventf(task.GetTaskPod(),
v1.EventTypeWarning, "ApplicationFailed",
+ "Application %s scheduling failed, reason: %s",
app.applicationID, errMess)
+ }
+ log.Logger().Warn("failed to schedule app", zap.String("appID",
app.applicationID))
Review comment:
I think we have already logged this, pls remove this line.
##########
File path: pkg/common/events/recorder.go
##########
@@ -31,7 +31,7 @@ import (
"github.com/apache/incubator-yunikorn-k8shim/pkg/conf"
)
-var eventRecorder record.EventRecorder = record.NewFakeRecorder(1024)
+var EventRecorder record.EventRecorder = record.NewFakeRecorder(1024)
Review comment:
we do not need to expose this if we put the mocked class to the same
package, we just need to expose a function to the callers so we can modify the
OnXXX functions.
--
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]
