feat(gcp)!: Initialization concurrency#10700
Conversation
| ProjectFilter string `json:"project_filter"` | ||
| BackoffDelay int `json:"backoff_delay"` | ||
| BackoffRetries int `json:"backoff_retries"` | ||
| DiscoveryConcurrency *int `json:"discovery_concurrency"` |
There was a problem hiding this comment.
I'd suggest to keep it a plain value and check for 0 instead of nil
There was a problem hiding this comment.
Since 0 would be meaningless (not a good idea to have infinite concurrency?) keeping it plain is an option, OTOH having this as a pointer is a better way of signalling to the user (or code reader/maintainer) that this is an optional value. Can go either way.
There was a problem hiding this comment.
I was trying to keep the same style as FolderRecursionDepth on line 8
|
|
||
| var defaultDiscoveryConcurrency = 100 | ||
| if spec.DiscoveryConcurrency == nil { | ||
| spec.DiscoveryConcurrency = &defaultDiscoveryConcurrency |
There was a problem hiding this comment.
You could define defaultDiscoveryConcurrency inside the block.
There was a problem hiding this comment.
I was trying to match the style in lines 19-21...
|
Why is this marked as a breaking change? Can it not be done in a backwards-compatible way? |
It's not really a 'certain breaking change' if we're lowering the default discovery from thousands to 100, but it might still break some orgs that need the higher discovery concurrency (if it works, at all). Making the default value same as the concurrency val would make it non-breaking but I'm guessing we don't want that as the default because it doesn't really work most of the time? |
|
I marked it as breaking because if a user had adjusted concurrency to a value other than default this would now change their configuration. |
🤖 I have created a release *beep* *boop* --- ## [9.0.0](plugins-source-gcp-v8.5.1...plugins-source-gcp-v9.0.0) (2023-05-25) ### ⚠ BREAKING CHANGES * This release introduces an internal change to our type system to use [Apache Arrow](https://arrow.apache.org/). This should not have any visible breaking changes, however due to the size of the change we are introducing it under a major version bump to communicate that it might have some bugs that we weren't able to catch during our internal tests. If you encounter an issue during the upgrade, please submit a [bug report](https://github.com/cloudquery/cloudquery/issues/new/choose). * **gcp:** Initialization concurrency ([#10700](#10700)) * **gcp:** Make Listing Enabled Services failures not block the sync ([#10699](#10699)) ### Features * **deps:** Upgrade to Apache Arrow v13 (latest `cqmain`) ([#10605](#10605)) ([a55da3d](a55da3d)) * **gcp:** Enable Better Logging for when `enabled_services_only` fails ([#10695](#10695)) ([a849aea](a849aea)) * **gcp:** Initialization concurrency ([#10700](#10700)) ([0c9f510](0c9f510)) * **gcp:** Make Listing Enabled Services failures not block the sync ([#10699](#10699)) ([5b49481](5b49481)) * Update to use [Apache Arrow](https://arrow.apache.org/) type system ([612d3ea](612d3ea)) ### Bug Fixes * **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.0.8 ([#10798](#10798)) ([27ff430](27ff430)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
|
I am not able to find the documentation for this change, is there any? |
|
@BenitezDev Good point, I think we missed it on this one! Opened an issue so we don't forget: #11170 In short you should be able to set |
Summary
We currently try and use the same concurrency value that we use for resolving all tables for parallelizing
ListServiceswhen the default is used then the concurrency can overload the credential provider as seen by user on discord.This PR creates a separate concurrency option that enables users to have a large sync concurrency but a much more limited initialization concurrency