studio/flamenco
studio/flamenco
12
30
Fork 38
Code Issues 64 Pull Requests 5 Releases Wiki Activity

Support pausing jobs #104313

Manually merged
Sybren A. Stüvel merged 28 commits from David-Zhang-10/flamenco:paused-job-status into main 2024-07-01 17:53:44 +02:00
Conversation 1 Commits 28 Files Changed 12 +36 -31
1 changed files with 36 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 8f067da901 - Show all commits

67
internal/manager/task_state_machine/task_state_machine.go
View File

@ -166,11 +166,26 @@ func (sm *StateMachine) jobStatusIfAThenB(
return sm.JobStatusChange(ctx, job, thenStatus, reason)
}
func (sm *StateMachine) shouldJobBePaused(ctx context.Context, logger zerolog.Logger, job *persistence.Job) (bool, error) {
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-06-26 11:16:17 +02:00
Outdated
Review
Copy Link

I think the function name can be improved. The name shouldJobBePaused describes what should be done by the caller. I always try to pick a function name that matches what the function does. So for this one, I feel that isJobPausingComplete would be better.

Also a documentation comment would be good. In Go, these always start with // {function name} .... It should describe that this returns true when the job status is pause-requested and there are no more active tasks.

I think the function name can be improved. The name `shouldJobBePaused` describes what should be done by the caller. I always try to pick a function name that matches what the function does. So for this one, I feel that `isJobPausingComplete` would be better. Also a documentation comment would be good. In Go, these always start with `// {function name} ...`. It should describe that this returns true when the job status is `pause-requested` and there are no more active tasks.
if job.Status == api.JobStatusPauseRequested {
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-06-26 11:12:17 +02:00
Outdated
Review
Copy Link

Right now, the structure of the function is such that the entire function body is contained in a conditional. This is not a good idea, as it unnecessarily increases the complexity. Flipping the condition of the first if will un-indent the remaining code:

	if job.Status != api.JobStatusPauseRequested {
		return false, nil
	}

  // rest of the code
Right now, the structure of the function is such that the entire function body is contained in a conditional. This is not a good idea, as it unnecessarily increases the complexity. Flipping the condition of the first `if` will un-indent the remaining code: ```go if job.Status != api.JobStatusPauseRequested { return false, nil } // rest of the code ```
numActive, _, err := sm.persist.CountTasksOfJobInStatus(ctx, job, api.TaskStatusActive)
if err != nil {
return false, err
}
if numActive == 0 {
// There is no active task, and the job is in pause-requested status, so we can pause the job.
logger.Info().Msg("No more active tasks, job is paused")
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-06-26 11:13:37 +02:00
Outdated
Review
Copy Link

I think it would be be better to leave the logging to the caller of this function. Also what is logged here is not entirely true -- the job is still in state pause-requested, not in state paused, so logging that it is paused is incorrect.

By removing the logging, you can reduce the final return to just return numActive == 0, nil.

I think it would be be better to leave the logging to the caller of this function. Also what is logged here is not entirely true -- the job is still in state `pause-requested`, not in state `paused`, so logging that it **is** paused is incorrect. By removing the logging, you can reduce the final `return` to just `return numActive == 0, nil`.
return true, nil
}
}
return false, nil
}
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-06-17 15:35:21 +02:00
Outdated
Review
Copy Link

This code block feels a bit out of place. If I understand correctly, this is the flow that this code would handle:

  1. User cancels a task.
  2. There are other tasks that are still runnable but not active (i.e. status in {queued, soft-failed, paused}).
  3. Job is in status pause-requested.

To me this doesn't look like a status in which the pausing was complete.

This logic is also repeated quite a bit below. I think it's better to make a new function responsible for answering the question "is the job pausing, and is that process complete now?", and call that function from the places that need this.

Finally, some reordering can be done to optimise the code. Currently every check will count the tasks in the job, even when job.Status != api.JobStatusPauseRequested. It's good practice to do the cheapest check first, and the most expensive check last.

This code block feels a bit out of place. If I understand correctly, this is the flow that this code would handle: 1. User cancels a task. 2. There are other tasks that are still runnable but not active (i.e. status in `{queued, soft-failed, paused}`). 3. Job is in status `pause-requested`. To me this doesn't look like a status in which the pausing was complete. This logic is also repeated quite a bit below. I think it's better to make a new function responsible for answering the question "_is the job pausing, and is that process complete now?_", and call that function from the places that need this. Finally, some reordering can be done to optimise the code. Currently every check will count the tasks in the job, even when `job.Status != api.JobStatusPauseRequested`. It's good practice to do the cheapest check first, and the most expensive check last.
David Zhang commented 2024-06-18 22:03:23 +02:00
Outdated
Review
Copy Link

The comment makes sense to me! The only thing I am confused about is what you meant by "the pausing was complete".

The comment makes sense to me! The only thing I am confused about is what you meant by "the pausing was complete".
Sybren A. Stüvel commented 2024-06-19 10:32:31 +02:00
Outdated
Review
Copy Link

The only thing I am confused about is what you meant by "the pausing was complete".

"Pausing is complete" for me means that the work that should be done when the job goes to pause-requested is done, and it can be moved to state paused.

> The only thing I am confused about is what you meant by "the pausing was complete". "Pausing is complete" for me means that the work that should be done when the job goes to `pause-requested` is done, and it can be moved to state `paused`.
David Zhang commented 2024-06-19 17:45:08 +02:00
Outdated
Review
Copy Link

So for the pausing to be complete, we need all tasks to be in paused state, including tasks that are runnable but not in active state? In other words, instead of checking the number of active tasks here, we should check if the job has any tasks other than paused?

So for the pausing to be complete, we need all tasks to be in `paused` state, including tasks that are runnable but not in `active` state? In other words, instead of checking the number of active tasks here, we should check if the job has any tasks other than `paused`?
// updateJobOnTaskStatusCanceled conditionally escalates the cancellation of a task to cancel the job.
func (sm *StateMachine) updateJobOnTaskStatusCanceled(ctx context.Context, logger zerolog.Logger, job *persistence.Job) error {
// If no more tasks can run, cancel the job.
numRunnable, _, err := sm.persist.CountTasksOfJobInStatus(ctx, job,
api.TaskStatusActive, api.TaskStatusQueued, api.TaskStatusSoftFailed, api.TaskStatusPaused)
api.TaskStatusActive, api.TaskStatusQueued, api.TaskStatusSoftFailed)
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-06-17 15:42:03 +02:00
Outdated
Review
Copy Link

This log entry is giving the wrong information. If updateJobOnTaskStatusCanceled() is called, a task was cancelled and thus the log message cannot be correct. Same for the job status change reason "all tasks completed".

The same is true for other copies of this code. Don't make your code lie; in a task-failure-handling part of the code, don't make it log "all tasks completed".

This log entry is giving the wrong information. If `updateJobOnTaskStatusCanceled()` is called, a task was cancelled and thus the log message cannot be correct. Same for the job status change reason "all tasks completed". The same is true for other copies of this code. Don't make your code lie; in a task-failure-handling part of the code, don't make it log "all tasks completed".
if err != nil {
return err
}
@ -181,16 +196,12 @@ func (sm *StateMachine) updateJobOnTaskStatusCanceled(ctx context.Context, logge
}
// Deal with the special case when the job is in pause-requested status.
if job.Status == api.JobStatusPauseRequested {
numActive, _, err := sm.persist.CountTasksOfJobInStatus(ctx, job, api.TaskStatusActive)
if err != nil {
return err
}
if numActive == 0 {
// There is no active task, and the job is in pause-requested status, so we can pause the job.
logger.Info().Msg("No more active tasks, job is paused")
return sm.JobStatusChange(ctx, job, api.JobStatusPaused, "all tasks completed")
}
toBePaused, err := sm.shouldJobBePaused(ctx, logger, job)
if err != nil {
return err
}
if toBePaused {
return sm.JobStatusChange(ctx, job, api.JobStatusPaused, "no more active tasks after task cancellation")
}
return nil
@ -218,16 +229,13 @@ func (sm *StateMachine) updateJobOnTaskStatusFailed(ctx context.Context, logger
// If the job didn't fail, this failure indicates that at least the job is active.
failLogger.Info().Msg("task failed, but not enough to fail the job")
if job.Status == api.JobStatusPauseRequested {
numActive, _, err := sm.persist.CountTasksOfJobInStatus(ctx, job, api.TaskStatusActive)
if err != nil {
return err
}
if numActive == 0 {
// There is no active task, and the job is in pause-requested status, so we can pause the job.
failLogger.Info().Msg("No more active tasks, job is paused")
return sm.JobStatusChange(ctx, job, api.JobStatusPaused, "all tasks completed")
}
// Deal with the special case when the job is in pause-requested status.
toBePaused, err := sm.shouldJobBePaused(ctx, logger, job)
if err != nil {
return err
}
if toBePaused {
return sm.JobStatusChange(ctx, job, api.JobStatusPaused, "no more active tasks after task failure")
}
return sm.jobStatusIfAThenB(ctx, logger, job, api.JobStatusQueued, api.JobStatusActive,
@ -245,16 +253,13 @@ func (sm *StateMachine) updateJobOnTaskStatusCompleted(ctx context.Context, logg
return sm.JobStatusChange(ctx, job, api.JobStatusCompleted, "all tasks completed")
}
if job.Status == api.JobStatusPauseRequested {
numActive, _, err := sm.persist.CountTasksOfJobInStatus(ctx, job, api.TaskStatusActive)
if err != nil {
return err
}
if numActive == 0 {
// There is no active task, and the job is in pause-requested status, so we can pause the job.
logger.Info().Msg("No more active tasks, job is paused")
return sm.JobStatusChange(ctx, job, api.JobStatusPaused, "all tasks completed")
}
// Deal with the special case when the job is in pause-requested status.
toBePaused, err := sm.shouldJobBePaused(ctx, logger, job)
if err != nil {
return err
}
if toBePaused {
return sm.JobStatusChange(ctx, job, api.JobStatusPaused, "no more active tasks after task completion")
}
logger.Info().