Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow number of simultaneous workers to be configured #4257

Merged
merged 5 commits into from
Sep 16, 2024

Conversation

theunrepentantgeek
Copy link
Member

What this PR does / why we need it:

Does what it says on the box: makes the number of concurrent workers configurable.

More workers can make the import process complete more quickly - but not in all cases.

The default of 4 seems to be a good compromise, ensuring a single slow resource doesn't stall the entire process too much. Making this configurable gives consumers more control when required.

Closes #4251

How does this PR make you feel

gif

@theunrepentantgeek theunrepentantgeek added this to the v2.10.0 milestone Sep 12, 2024
@theunrepentantgeek theunrepentantgeek self-assigned this Sep 12, 2024
@theunrepentantgeek theunrepentantgeek changed the title Feature/configurable workers Allow number of simultaneous workers to be configured Sep 16, 2024
Copy link
Member

@matthchr matthchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, but have reservations about ignoring invalid user input for --workers flag. I think rejecting would be better.

v2/cmd/asoctl/cmd/import_azure_resource.go Outdated Show resolved Hide resolved
@@ -282,3 +291,12 @@ func (ri *ResourceImporter) importResource(

return result
}

// desiredWorkers returns the number of workers to use for importing resources.
func (ri *ResourceImporter) desiredWorkers() int {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: This method isn't hurting but my understanding is that it's not actually possible to take this path unless the user explicitly passes a negative or nonzero value for the cmdline option, because if the user doesn't specify anything the default of 4 is used at the cmdline flag level (thus not hitting the default of 4 here).

I'm wondering if it's better to add a validate helper to importAzureResourceOptions, and then call that at the start of importAzureResource and fail the cmd if the user passes a negative value? Could also be extended to check the provided labels for validity (length, allowed number of characters?).

Oh wait I see you already parse the labels it's just down below after the resources are imported. Maybe move the parsing of the labels up front, and validate input of workers too to fail fast on bad user input, then run import (and use the actual parsed labels if configured?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default will always be 4 if used from the command-line, but that's not the only way to get here.
With the relocation under pkg we've allowed for reuse. The value here is an option provided in the ResourceImporterOptions struct, which might not be set by a caller.

@theunrepentantgeek theunrepentantgeek added this pull request to the merge queue Sep 16, 2024
Merged via the queue into main with commit d2ec2b0 Sep 16, 2024
8 checks passed
@theunrepentantgeek theunrepentantgeek deleted the feature/configurable-workers branch September 16, 2024 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Make the number of workers used by asoctl configurable
2 participants