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

11
web/app/src/stores/tasks.js
View File

@ -23,10 +23,17 @@ export const useTasks = defineStore('tasks', {
const jobs = useJobs(); const jobs = useJobs();
const activeJob = jobs.activeJob; const activeJob = jobs.activeJob;
// Check if the job status is 'pause-requested'. if (!activeJob) {
if (activeJob && activeJob.status === 'pause-requested') { console.warn('no active job, unable to determine whether the active task is cancellable');
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-06-17 15:27:39 +02:00
Outdated
Review
Copy Link

There's generally two reasons to test something with an if statement:

  • checking preconditions (for example to avoid errors), and
  • actual business logic.

In this case the check for activeJob is there to prevent errors when accessing activeJob.status in case there is no active job. This mixes the two above reasons, though.

Another way to look at this, is asking yourself: does it make sense to even answer the query ("can I cancel the active task?") if there is no active job? If too little information is given, in this particular case we know the task but not the job, is that an error? Or is that a sensible state to be in?

When the front-end is displaying tasks, it should be aware of which job those tasks belong to. If that is not the case, I feel that this is a bug. Such bugs should not be hidden, not be defensively worked around. They should be visible, so that they are easy to recognise as bugs. That way people report them, and we can get them fixed.

All that to say: it's probably better to write something like:

    canCancel() {
      const jobs = useJobs();
      const activeJob = jobs.activeJob;

      if (!activeJob) {
        console.warn('no active job, unable to determine whether the active task is cancellable');
        return false;
      }

      if (activeJob.status == 'pause-requested') {
        // Cancelling a task should not be possible while the job is being paused.
        // In the future this might be supported, see issue #104315.
        return false;
      }

      // Allow cancellation for specified task statuses.
      return this._anyTaskWithStatus(['queued', 'active', 'soft-failed']);
    },

Also the triple === is unnecessary. The job status is never going to be something other than a string, and if it's some other type, if it compares equal to "pause-requested", it's good enough a reason to disable cancellation of the task.

There's generally two reasons to test something with an `if` statement: - checking preconditions (for example to avoid errors), and - actual business logic. In this case the check for `activeJob` is there to prevent errors when accessing `activeJob.status` in case there is no active job. This mixes the two above reasons, though. Another way to look at this, is asking yourself: **does it make sense to even answer the query** ("can I cancel the active task?") if there is no active job? If too little information is given, in this particular case we know the task but not the job, is that an error? Or is that a sensible state to be in? When the front-end is displaying tasks, it should be aware of which job those tasks belong to. If that is not the case, I feel that this is a bug. Such bugs should not be hidden, not be defensively worked around. They should be visible, so that they are easy to recognise as bugs. That way people report them, and we can get them fixed. All that to say: it's probably better to write something like: ```js canCancel() { const jobs = useJobs(); const activeJob = jobs.activeJob; if (!activeJob) { console.warn('no active job, unable to determine whether the active task is cancellable'); return false; } if (activeJob.status == 'pause-requested') { // Cancelling a task should not be possible while the job is being paused. // In the future this might be supported, see issue #104315. return false; } // Allow cancellation for specified task statuses. return this._anyTaskWithStatus(['queued', 'active', 'soft-failed']); }, ``` Also the triple `===` is unnecessary. The job status is never going to be something other than a string, and if it's some other type, if it compares equal to `"pause-requested"`, it's good enough a reason to disable cancellation of the task.
return false; return false;
} }
David-Zhang-10 marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-06-17 15:28:20 +02:00
Outdated
Review
Copy Link

Make comments proper sentences, so end with a period.

Make comments proper sentences, so end with a period.
if (activeJob.status == 'pause-requested') {
// Cancelling a task should not be possible while the job is being paused.
// In the future this might be supported, see issue #104315.
return false;
}
// Allow cancellation for specified task statuses. // Allow cancellation for specified task statuses.
return this._anyTaskWithStatus(['queued', 'active', 'soft-failed']); return this._anyTaskWithStatus(['queued', 'active', 'soft-failed']);
}, },