Resolved Task Limit error in Flamenco Manager #104201 #104205

Contributor

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)
Anish-Bharadwaj added 1 commit 2023-04-06 15:15:25 +02:00
4093f6de16 Update jobs.go
Setup Dependency update to run sql command in batches, allowing for larger task sizes in Flamenco Renders
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

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`
Author
Contributor

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 added 1 commit 2023-04-12 02:21:33 +02:00
Anish-Bharadwaj added 1 commit 2023-04-12 10:40:01 +02:00
8a2ee95fd3 Merge branch 'main' into 104201-Resolving-Task-limit-issue-in-Flamenco-manager
# Conflicts:
#	internal/manager/persistence/jobs_test.go
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
Author
Contributor

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

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!
@ -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

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.
@ -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

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.
@ -597,0 +613,4 @@
for i := 0; i < numTasks-1; i++ {
currtask := job_compilers.AuthoredTask{
Name: "render-" + fmt.Sprintf("%d", i),
Type: "ffmpeg",

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

Just for consistency with the name, this should be `Type: "blender-render"`.
@ -16,6 +16,7 @@ import (
)
const schedulerTestTimeout = 100 * time.Millisecond
const schedulerTestTimeoutlong = 5000 * time.Millisecond

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

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.
No description provided.