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 +11 -10
1 changed files with 11 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 95f3472e6d - Show all commits

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

@ -168,16 +168,17 @@ func (sm *StateMachine) jobStatusIfAThenB(
// isJobPausingComplete returns true when the job status is pause-requested and there are no more active tasks. // isJobPausingComplete returns true when the job status is pause-requested and there are no more active tasks.
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.
func (sm *StateMachine) isJobPausingComplete(ctx context.Context, logger zerolog.Logger, job *persistence.Job) (bool, error) { func (sm *StateMachine) isJobPausingComplete(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: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 ```
if job.Status == api.JobStatusPauseRequested { if job.Status != api.JobStatusPauseRequested {
numActive, _, err := sm.persist.CountTasksOfJobInStatus(ctx, job, api.TaskStatusActive) return false, nil
if err != nil { }
return false, err numActive, _, err := sm.persist.CountTasksOfJobInStatus(ctx, job, api.TaskStatusActive)
} if err != nil {
if numActive == 0 { return false, err
// There is no active task, and the job is in pause-requested status, so we can pause the job. }
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`.
logger.Info().Msg("No more active tasks, job is paused") if numActive == 0 {
return true, nil // 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 true, 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`?
return false, nil return false, nil
} }