-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
enable parallelism on bigquery tests #32580
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
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.
Its confusing so
systemProperty 'junit.jupiter.execution.parallel.enabled', 'true'
is already set but this systemProperty 'junit.jupiter.execution.parallel.config.fixed.parallelism', concurrency
can take precedence and limit it ?
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.
I'm happy if the CI is happy! Presumably, this fix can be applied to other destinations whose tests are thread-safe.
@gisripa there are two layers of concurrency: gradle will happily spawn an arbitrary number of workers and partition the test set into each of their JVMs, however JUnit offers in-JVM concurrency. The former is safer because you don't have to worry about thread-safety, and it remains the default. The junit properties are a bit confusing but they enable developers to opt out of the gradle concurrency and into the junit concurrency by setting just one property.
/approve-and-merge reason="irrelevant CI failures #32580 (comment)" |
Make destination-bigquery tests fast again by reenabling parallelism.
I'm still not sure why destination-snowflake tests are also slower. It already has a gradle.properties file with parallelism set to 4. Metabase says destination-snowflake tests got slower around the same time, so presumably https://github.com/airbytehq/airbyte/pull/32108/files#diff-a3dd6be9c3819c1fcfd593b0f11c29663912e686e0dd74cae14610ac9c282480 did something? But I'm not sure what exactly.
Would it break things to set testExecutionConcurrency=-1 by default? My understanding is that tests need to opt into concurrent execution (with
@Execution(ExecutionMode.CONCURRENT)
).