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.

Sign up for GitHub

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

Jump to bottom

airbyte-ci: concurrent java tests #31426

Merged
merged 4 commits into from
Oct 16, 2023
Merged

airbyte-ci: concurrent java tests #31426

merged 4 commits into from
Oct 16, 2023

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Oct 15, 2023 •
edited
Loading

What

Closes #31444

Make java connector tests run concurrently:
Unit, integration and CAT can run in parallel.

How

Run the test steps concurrently in a task group

Performance gain

master this branch speed gain
total duration 29mn38s 20mn34s -9mn4s
unit tests 15mn57s 17mn9s +1mn12s
integration tests 7mn22s 8mn50s +1mn28s
CAT 02mn12s 3mn59s +1mn47s

Conclusion: Concurrent execution helps drastically in speeding up the total duration. We observe that the single test steps duration is increased. It probably mean that we have CPU contention. We'll find out with #31443

🚨 User Impact 🚨

When running tests for source-postgres locally the test crashes: I believe this is a problem with the local resource: concurrent test run puts more pressure than my laptop can bear. It's not failing on the CI with 16 CPUs.

A fail fast logic would be harder to implement:
A fail fast logic would be harder to implement while using the task group: a task synchronization efforts should be done with anyio if we want to support a fail fast mode.
I don't think it's an effort we should make in the scope of the test performance improvement.

@vercel
Copy link

vercel bot commented Oct 15, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Oct 16, 2023 8:03pm

@alafanechere Graphite App
Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@alafanechere alafanechere force-pushed the augustin/10-15-airbyte-ci_concurrent_java_tests branch from a97dc31 to b6b49f4 Compare October 15, 2023 13:00
@alafanechere alafanechere marked this pull request as ready for review October 16, 2023 11:24
@alafanechere alafanechere requested a review from a team October 16, 2023 11:24
@alafanechere alafanechere force-pushed the augustin/10-15-airbyte-ci_concurrent_java_tests branch from 2300346 to 25c2a1e Compare October 16, 2023 16:55
evantahler
evantahler reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

Nice!

what does this do to the logs for someone debugging a test run?

postamar
postamar reviewed Oct 16, 2023
View reviewed changes
airbyte-ci/connectors/pipelines/pipelines/tests/java_connectors.py Outdated
integration_tests_results = await IntegrationTests(context).run(connector_image_tar_file, normalization_tar_file)
step_results.append(integration_tests_results)
async with asyncer.create_task_group() as test_task_group:
soon_unit_tests_results = test_task_group.soonify(UnitTests(context).run)()
Copy link
Contributor

@postamar postamar Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

UnitTests doesn't depend on building and loading the connector image. You might get even better performance (due to gradle caching) if you kicked off this task as soon as the distTar is built. Possibly sooner.

Perhaps this is out of scope for this work but it would be nice to take a more concurrent approach here: kick off each of these steps ASAP and have each of them wait on their dependencies being fulfilled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this suggestion, trying this out right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It speeds things up by 1mn!

@alafanechere
Copy link
Contributor Author

Nice!

what does this do to the logs for someone debugging a test run?

The high level logs are a bit more intertwined:

Testing connector source-postgres - Java Connector Integration Tests: Invalidating the persisted dagger cache for source-postgres. Only used in the context of the CI performance improvements project for source-postgres.
Testing connector source-postgres - Java Connector Integration Tests: Getting a fresh gradle cache volume name for source-postgres to not use remote caching. Only used in the context of the CI performance improvements project for source-postgres.
Testing connector source-postgres - Java Connector Unit Tests: ⏳ Still running... (duration: 02mn00s)
Testing connector source-postgres - Java Connector Integration Tests: ⏳ Still running... (duration: 01mn30s)
Testing connector source-postgres - Acceptance tests: ⏳ Still running... (duration: 01mn30s)

@postamar
Copy link
Contributor

The high level logs are a bit more intertwined

FWIW this honestly doesn't bother me, quite the opposite in fact.

postamar
postamar approved these changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

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

LGTM assuming this change actually works!

@alafanechere alafanechere merged commit 4b33225 into master Oct 16, 2023
24 checks passed
@alafanechere alafanechere deleted the augustin/10-15-airbyte-ci_concurrent_java_tests branch October 16, 2023 20:34
@sentry-io
Copy link

sentry-io bot commented Oct 17, 2023 •
edited
Loading

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ ExceptionGroup: 2 exceptions were raised in the task group: pipelines.tests.java_connectors in run_all_tests View Issue
  • ‼️ ExceptionGroup: 2 exceptions were raised in the task group: pipelines.airbyte_ci.connectors.test.steps.java... View Issue
  • ‼️ ExceptionGroup: 2 exceptions were raised in the task group: pipelines.airbyte_ci.connectors.test.steps.java... View Issue

Did you find this useful? React with a 👍 or 👎

ariesgun pushed a commit to ariesgun/airbyte that referenced this pull request Oct 23, 2023
@alafanechere @ariesgun
airbyte-ci: concurrent java tests (airbytehq#31426)
9af2c48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evantahler evantahler evantahler approved these changes

@postamar postamar postamar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

airbyte-ci: concurrent run of java connector tests
3 participants
@alafanechere @postamar @evantahler