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

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

@ -166,7 +166,8 @@ 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) {
// 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) {
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 {
numActive, _, err := sm.persist.CountTasksOfJobInStatus(ctx, job, api.TaskStatusActive)
if err != nil {
@ -196,7 +197,7 @@ func (sm *StateMachine) updateJobOnTaskStatusCanceled(ctx context.Context, logge
}
// Deal with the special case when the job is in pause-requested status.
toBePaused, err := sm.shouldJobBePaused(ctx, logger, job)
toBePaused, err := sm.isJobPausingComplete(ctx, logger, job)
if err != nil {
return err
}
@ -230,7 +231,7 @@ func (sm *StateMachine) updateJobOnTaskStatusFailed(ctx context.Context, logger
failLogger.Info().Msg("task failed, but not enough to fail the job")
// Deal with the special case when the job is in pause-requested status.
toBePaused, err := sm.shouldJobBePaused(ctx, logger, job)
toBePaused, err := sm.isJobPausingComplete(ctx, logger, job)
if err != nil {
return err
}
@ -254,7 +255,7 @@ func (sm *StateMachine) updateJobOnTaskStatusCompleted(ctx context.Context, logg
}
// Deal with the special case when the job is in pause-requested status.
toBePaused, err := sm.shouldJobBePaused(ctx, logger, job)
toBePaused, err := sm.isJobPausingComplete(ctx, logger, job)
if err != nil {
return err
}
@ -509,7 +510,7 @@ func (sm *StateMachine) pauseTasks(
}
// If pausing was requested, it has now happened, so the job can transition.
toBePaused, err := sm.shouldJobBePaused(ctx, logger, job)
toBePaused, err := sm.isJobPausingComplete(ctx, logger, job)
if err != nil {
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-06-26 11:23:57 +02:00
Outdated
Review
Copy Link

This hides the underlying error, making things hard to debug. When wrapping an error, always use the %w format specifier to include the underlying error. This will make sure that functions like errors.Is() and errors.As() can do their work. You can find more about this at https://pkg.go.dev/errors

Also, this function shouldn't know what's going on inside of shouldJobBePaused(), so if you want to have "when accessing number of active tasks" in the error message, that should be done inside shouldJobBePaused(), not here.

This hides the underlying error, making things hard to debug. When wrapping an error, always use the `%w` format specifier to include the underlying error. This will make sure that functions like `errors.Is()` and `errors.As()` can do their work. You can find more about this at https://pkg.go.dev/errors Also, this function shouldn't know what's going on inside of `shouldJobBePaused()`, so if you want to have "when accessing number of active tasks" in the error message, that should be done inside `shouldJobBePaused()`, not here.
return "", fmt.Errorf("error when accessing number of active tasks")
}