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

Resolved Task Limit error in Flamenco Manager #104201 #104205

Merged
Sybren A. Stüvel merged 3 commits from Anish-Bharadwaj/flamenco:104201-Resolving-Task-limit-issue-in-Flamenco-manager into main 2023-04-24 15:11:01 +02:00
Conversation 3 Commits 3 Files Changed 3 +69 -5
Anish-Bharadwaj commented 2023-04-06 15:15:23 +02:00
Contributor
Copy Link

Resolving Task Limit In Falmenco Manager

In my recent commit, I had to address an issue with SQL Server not liking all dependencies being inserted at once. To resolve this problem, I uploaded the dependencies of a task in batches of 1000 allowing the server to handle larger task loads.

(Accidentally Closed Previous Pull Request #104203)

Resolving Task Limit In Falmenco Manager In my recent commit, I had to address an issue with SQL Server not liking all dependencies being inserted at once. To resolve this problem, I uploaded the dependencies of a task in batches of 1000 allowing the server to handle larger task loads. (Accidentally Closed Previous Pull Request #104203)
Sybren A. Stüvel requested review from Sybren A. Stüvel 2023-04-07 12:32:02 +02:00
Sybren A. Stüvel added this to the v3.3 milestone 2023-04-07 12:32:15 +02:00
Sybren A. Stüvel commented 2023-04-07 12:33:34 +02:00
Owner
Copy Link

Could you write a unit test as well, that tests the creation of >1000 tasks? You can just add a new test to internal/manager/persistence/jobs_test.go

Could you write a unit test as well, that tests the creation of >1000 tasks? You can just add a new test to `internal/manager/persistence/jobs_test.go`
Anish-Bharadwaj commented 2023-04-07 12:43:59 +02:00
Author
Contributor
Copy Link

Yeah, definitely! I'll write the unit test for this functionality and add it to this PR

Yeah, definitely! I'll write the unit test for this functionality and add it to this PR
Anish-Bharadwaj changed title from Resolved Task Limit error in Flamenco Manager #104201 to WIP: Resolved Task Limit error in Flamenco Manager #104201 2023-04-07 12:44:24 +02:00
Anish-Bharadwaj changed title from WIP: Resolved Task Limit error in Flamenco Manager #104201 to Resolved Task Limit error in Flamenco Manager #104201 2023-04-12 12:02:07 +02:00
Anish-Bharadwaj force-pushed 104201-Resolving-Task-limit-issue-in-Flamenco-manager from 8a2ee95fd3 to b235fe5e01 2023-04-12 13:11:52 +02:00 Compare
Anish-Bharadwaj force-pushed 104201-Resolving-Task-limit-issue-in-Flamenco-manager from b235fe5e01 to 6941e9f931 2023-04-12 13:28:18 +02:00 Compare
Anish-Bharadwaj commented 2023-04-12 13:36:53 +02:00
Author
Contributor
Copy Link

I was ran into some odd merge conflicts so I reverted a previous push in the:
"force-pushed 104201-Resolving-Task-limit-issue-in-Flamenco-manager from 8a2ee95fd3 to b235fe5e01 "

And then pulled codebase changes again to resolve the issue.

I was ran into some odd merge conflicts so I reverted a previous push in the: "force-pushed 104201-Resolving-Task-limit-issue-in-Flamenco-manager from 8a2ee95fd3 to b235fe5e01 " And then pulled codebase changes again to resolve the issue.
Sybren A. Stüvel requested changes 2023-04-17 16:42:35 +02:00
Sybren A. Stüvel left a comment
Owner
Copy Link

Thanks for the test, I'm glad to have it in there :)

Just a few tiny remarks, almost done!

Thanks for the test, I'm glad to have it in there :) Just a few tiny remarks, almost done!
internal/manager/persistence/jobs_test.go Outdated
@ -594,6 +606,37 @@ func createTestAuthoredJobWithTasks() job_compilers.AuthoredJob {
return createTestAuthoredJob("263fd47e-b9f8-4637-b726-fd7e47ecfdae", task1, task2, task3)
}
//Create a Job with a Specified number of tasks
Sybren A. Stüvel commented 2023-04-17 16:35:40 +02:00
Owner
Copy Link

Function documentation in Go should start with the function name, so in this case:

// createTestAuthoredJobWithNumTasks creates a Job with a specified number of tasks
func createTestAuthoredJobWithNumTasks(numTasks int) job_compilers.AuthoredJob {

In this particular case, though, I think the comment can be removed as it just repeats the same info that's already in the function name.

Function documentation in Go should start with the function name, so in this case: ```go // createTestAuthoredJobWithNumTasks creates a Job with a specified number of tasks func createTestAuthoredJobWithNumTasks(numTasks int) job_compilers.AuthoredJob { ``` In this particular case, though, I think the comment can be removed as it just repeats the same info that's already in the function name.
internal/manager/persistence/jobs_test.go Outdated
@ -596,1 +608,4 @@
//Create a Job with a Specified number of tasks
func createTestAuthoredJobWithNumTasks(numTasks int) job_compilers.AuthoredJob {
//Generates all of the render jobs
Sybren A. Stüvel commented 2023-04-17 16:37:16 +02:00
Owner
Copy Link

Please configure your IDE to use auto-formatting, or run go fmt ./... before committing.

Please configure your IDE to use auto-formatting, or run `go fmt ./...` before committing.
internal/manager/persistence/jobs_test.go Outdated
@ -597,0 +613,4 @@
for i := 0; i < numTasks-1; i++ {
currtask := job_compilers.AuthoredTask{
Name: "render-" + fmt.Sprintf("%d", i),
Type: "ffmpeg",
Sybren A. Stüvel commented 2023-04-17 16:37:43 +02:00
Owner
Copy Link

Just for consistency with the name, this should be Type: "blender-render".

Just for consistency with the name, this should be `Type: "blender-render"`.
internal/manager/persistence/task_scheduler_test.go Outdated
@ -16,6 +16,7 @@ import (
)
const schedulerTestTimeout = 100 * time.Millisecond
const schedulerTestTimeoutlong = 5000 * time.Millisecond
Sybren A. Stüvel commented 2023-04-17 16:41:06 +02:00
Owner
Copy Link

This is a good idea, to have another timeout for this longer-running test.

To take it one step further, in TestCheckIfJobsHoldLargeNumOfTasks() you could add this code at the start of the function:

if testing.Short() {
    t.Skip("skipping test in short mode")
}

That way running go test -short will actually skip the test. It's not a necessity, but just a cherry on top. Up to you if you want to spend time on this, I'll accept the patch with or without this change.

This is a good idea, to have another timeout for this longer-running test. To take it one step further, in `TestCheckIfJobsHoldLargeNumOfTasks()` you could add this code at the start of the function: ```go if testing.Short() { t.Skip("skipping test in short mode") } ``` That way running `go test -short` will actually skip the test. It's not a necessity, but just a cherry on top. Up to you if you want to spend time on this, I'll accept the patch with or without this change.
Anish-Bharadwaj force-pushed 104201-Resolving-Task-limit-issue-in-Flamenco-manager from 6941e9f931 to 140115f820 2023-04-18 00:35:53 +02:00 Compare
Sybren A. Stüvel approved these changes 2023-04-24 15:09:25 +02:00
Sybren A. Stüvel left a comment
Owner
Copy Link

Thanks!

Thanks!
Sybren A. Stüvel merged commit 0502498dfa into main 2023-04-24 15:11:01 +02:00
Sybren A. Stüvel deleted branch 104201-Resolving-Task-limit-issue-in-Flamenco-manager 2023-04-24 15:11:01 +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
v3.3
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#104205
No description provided.
Delete Branch "Anish-Bharadwaj/flamenco:104201-Resolving-Task-limit-issue-in-Flamenco-manager"

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?