-
Notifications
You must be signed in to change notification settings - Fork 40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, had a few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless this is a blocker for a client I would hold off any non critical speed and new features for aws provider until we are able to stabilize it and get around bugs/sentry.
This should ideally be refactored in the SDK maybe? not sure |
I agree that we should not be adding more, but this (IMO) is a stabilizing change... We currently have at least 3 different implementations of this pattern that we are trying to standardize on a single implementation. |
I think you are correct, but I am not trying to get ahead of ourselves. I am trying to be slow/deliberate going from resolver --> client --> sdk. I think I would only put this in the SDK if we implemented the same pattern in multiple providers |
@bbernays can you please point me to the 3 places in the code we use this pattern? |
cq-provider-aws/resources/services/athena/data_catalogs.go Lines 221 to 249 in 53d391b
cq-provider-aws/resources/services/athena/work_groups.go Lines 416 to 442 in 53d391b
cq-provider-aws/resources/services/sagemaker/training_jobs.go Lines 590 to 617 in 53d391b
cq-provider-aws/resources/services/ecs/task_definitions.go Lines 647 to 678 in 53d391b
|
Co-authored-by: Kemal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with couple nits
Old review, issues/questions have been addressed
🎉 Thank you for making CloudQuery awesome by submitting a PR 🎉
Summary
This pr Attempts to centralize the parallelization logic for resources in order to provide a singular location for tuning to occur (if necessary). While this initial implementation still has a hardcoded number of go routines, in the future we could swap out that hard coded number for a dynamic value based on resource utilization.
Use the following steps to ensure your PR is ready to be reviewed
go fmt
to format your code 🖊golangci-lint run
🚨 (install golangci-lint here)go run ./docs/docs.go
and committing the changes 📃