studio/flamenco
studio/flamenco
12
30
Fork 38
Code Issues 62 Pull Requests 8 Releases Wiki Activity

Allow jobs to be submitted in paused status #104322

Merged
David Zhang merged 3 commits from David-Zhang-10/flamenco:submit-as-paused into main 2024-07-11 17:08:27 +02:00
Conversation 1 Commits 3 Files Changed 11 +166 -124
David Zhang commented 2024-07-02 21:13:09 +02:00
Collaborator
Copy Link

Overview
This pull request implements Issue #99439 Allow jobs to be submitted in paused status and builds upon PR #104313 Support pausing jobs.

Deliverables

  • Updates in relevant API implementation.
  • If the user does wish to submit a job in paused status, the job shows up correctly in the web interface.
**Overview** This pull request implements [Issue #99439 Allow jobs to be submitted in paused status](https://projects.blender.org/studio/flamenco/issues/99439) and builds upon [PR #104313 Support pausing jobs](https://projects.blender.org/studio/flamenco/pulls/104313). **Deliverables** - Updates in relevant API implementation. - If the user does wish to submit a job in `paused` status, the job shows up correctly in the web interface.
David Zhang requested review from Sybren A. Stüvel 2024-07-02 22:34:16 +02:00
Sybren A. Stüvel requested changes 2024-07-08 15:27:38 +02:00
Dismissed
internal/manager/api_impl/jobs.go Outdated
@ -94,2 +94,2 @@
// TODO: check whether this job should be queued immediately or start paused.
authoredJob.Status = api.JobStatusQueued
submittedJob := api.SubmittedJob(job)
if *submittedJob.InitialStatus == api.JobStatusPaused {
Sybren A. Stüvel commented 2024-07-08 15:20:06 +02:00
Owner
Copy Link

If submittedJob.InitialStatus is nil, dereferencing the pointer will cause a panic.

If `submittedJob.InitialStatus` is `nil`, dereferencing the pointer will cause a panic.
David-Zhang-10 marked this conversation as resolved
internal/manager/api_impl/jobs.go Outdated
@ -96,0 +95,4 @@
if *submittedJob.InitialStatus == api.JobStatusPaused {
authoredJob.Status = api.JobStatusPaused
} else {
authoredJob.Status = api.JobStatusQueued
Sybren A. Stüvel commented 2024-07-08 15:26:48 +02:00
Owner
Copy Link

This will cause a job with initialStatus: "failed" to be accepted, and silently its initial status will be overwritten. I don't this is a good approach. I think this is sufficient:

initialStatus := api.JobStatusQueued
if (submittedJob.InitialStatus != nil) {
    initialStatus = *submittedJob.InitialStatus
}
authoredJob.Status = initialStatus

If you want to do more validation you could, but I don't think it's necessary. In that case, a global variable declared at the top could help:

var validInitialJobStatuses = map[api.JobStatus]bool{
	api.JobStatusQueued: true,
	api.JobStatusPaused: true,
}

// ...

if !validInitialJobStatuses[initialStatus] {
  // .. reject submitted job with appropriate HTTP error status code
}
This will cause a job with `initialStatus: "failed"` to be accepted, and silently its initial status will be overwritten. I don't this is a good approach. I think this is sufficient: ```go initialStatus := api.JobStatusQueued if (submittedJob.InitialStatus != nil) { initialStatus = *submittedJob.InitialStatus } authoredJob.Status = initialStatus ``` If you want to do more validation you could, but I don't think it's necessary. In that case, a global variable declared at the top could help: ```go var validInitialJobStatuses = map[api.JobStatus]bool{ api.JobStatusQueued: true, api.JobStatusPaused: true, } // ... if !validInitialJobStatuses[initialStatus] { // .. reject submitted job with appropriate HTTP error status code } ```
David-Zhang-10 marked this conversation as resolved
David Zhang changed title from WIP: Allow jobs to be submitted in `paused` status to Allow jobs to be submitted in `paused` status 2024-07-08 16:02:45 +02:00
David Zhang requested review from Sybren A. Stüvel 2024-07-09 03:18:36 +02:00
Sybren A. Stüvel approved these changes 2024-07-09 14:26:01 +02:00
Sybren A. Stüvel left a comment
Owner
Copy Link

Looking good!

Looking good!
David Zhang force-pushed submit-as-paused from 01d5b42e9b to 0ab9f47aad 2024-07-10 04:54:58 +02:00 Compare
David Zhang force-pushed submit-as-paused from 0ab9f47aad to 00848a3755 2024-07-10 04:57:12 +02:00 Compare
Sybren A. Stüvel commented 2024-07-11 09:56:09 +02:00
Owner
Copy Link

The split-up into three commits looks good! And for me it's also confirming that the choice to do these splits is a good one. Now there is a clear overview of what changed in the API definition, and what was necessary to implement that change. Without the noise of all the generated files, this gets much clearer.

Please press the 'Rebase then fast-forward' button :)

The split-up into three commits looks good! And for me it's also confirming that the choice to do these splits is a good one. Now there is a clear overview of what changed in the API definition, and what was necessary to implement that change. Without the noise of all the generated files, this gets much clearer. Please press the 'Rebase then fast-forward' button :)
🎉 1 🚀 1
David Zhang merged commit 00848a3755 into main 2024-07-11 17:08:27 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Sybren A. Stüvel
Labels
Clear labels   
Good First Issue
  
Priority
High

  
Priority
Low

  
Priority
Normal

  
Status
Archived

  
Status
Confirmed

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Job Type
Custom Flamenco job type

  
Type
Known Issue

  
Type
Patch

  
Type
Report

  
Type
To Do

No Label
Good First Issue
Priority
High
Priority
Low
Priority
Normal
Status
Archived
Status
Confirmed
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Job Type
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: studio/flamenco#104322
No description provided.
Delete Branch "David-Zhang-10/flamenco:submit-as-paused"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?