[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #267: [YUNIKORN-686] [Regression] Placeholders for completed applications are recreated during recovery

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

wilfred-s commented on a change in pull request #267:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/267#discussion_r639357852



##########
File path: pkg/cache/application.go
##########
@@ -456,6 +456,29 @@ func (app *Application) handleRecoverApplicationEvent(event *fsm.Event) {
 	}
 }
 
+func (app *Application) skipReservationStage() bool {
+	// no task groups defined, skip reservation
+	if len(app.taskGroups) == 0 {
+		log.Logger().Debug("Skip reservation stage: no task groups defined")

Review comment:
       Add the appID to the log to allow tracking the flow correctly. There might be multiple apps doing the same thing.

##########
File path: pkg/cache/application.go
##########
@@ -456,6 +456,29 @@ func (app *Application) handleRecoverApplicationEvent(event *fsm.Event) {
 	}
 }
 
+func (app *Application) skipReservationStage() bool {
+	// no task groups defined, skip reservation
+	if len(app.taskGroups) == 0 {
+		log.Logger().Debug("Skip reservation stage: no task groups defined")
+		return true
+	}
+
+	// if there is any task already passed New state,
+	// that means the scheduler has already tried to schedule it
+	// in this case, we should skip the reservation stage
+	if len(app.taskMap) > 0 {
+		for _, task := range app.taskMap {
+			if task.GetTaskState() != events.States().Task.New {
+				log.Logger().Debug("Skip reservation stage: found task already has been scheduled before.",
+					zap.String("taskID", task.GetTaskID()),

Review comment:
       We should have the appID also in this log entry. 

##########
File path: pkg/cache/application.go
##########
@@ -456,6 +456,29 @@ func (app *Application) handleRecoverApplicationEvent(event *fsm.Event) {
 	}
 }
 
+func (app *Application) skipReservationStage() bool {
+	// no task groups defined, skip reservation
+	if len(app.taskGroups) == 0 {
+		log.Logger().Debug("Skip reservation stage: no task groups defined")
+		return true
+	}
+
+	// if there is any task already passed New state,
+	// that means the scheduler has already tried to schedule it
+	// in this case, we should skip the reservation stage
+	if len(app.taskMap) > 0 {
+		for _, task := range app.taskMap {
+			if task.GetTaskState() != events.States().Task.New {
+				log.Logger().Debug("Skip reservation stage: found task already has been scheduled before.",

Review comment:
       The application has _at least_ one task that has been scheduled before. The exact task does not matter and since it is a range over a map it will log a random taskID. The same input will generate different log entries.




-- 
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