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

Run tests with more parallelism #22096

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Run tests with more parallelism #22096

wants to merge 3 commits into from

Conversation

findepi
Copy link
Member

@findepi findepi commented May 23, 2024

No description provided.

@findepi findepi added test no-release-notes This pull request does not require release notes entry labels May 23, 2024
@cla-bot cla-bot bot added the cla-signed label May 23, 2024
@github-actions github-actions bot added iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector labels May 23, 2024
hashhar
hashhar reviewed May 23, 2024
View reviewed changes
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java Outdated

@TestInstance(PER_CLASS)
@Execution(CONCURRENT)
Copy link
Member

Choose a reason for hiding this comment

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

IIRC this is inherited from base classes (but please confirm) and ATQF is marked CONCURRENT already.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point

@findepi
Copy link
Member Author

findepi commented May 23, 2024

green except checkstyle

@findepi findepi force-pushed the findepi/run-tests-with-more-parallelism-f1f758 branch from d563421 to e91a8db Compare May 23, 2024 19:58
@findepi findepi requested a review from hashhar May 23, 2024 19:59
hashhar
hashhar reviewed May 24, 2024
View reviewed changes
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Outdated

@Isolated // TODO remove
Copy link
Member

Choose a reason for hiding this comment

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

nit: does this need stress testing on CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't see anything special about this class.

Copy link
Member

Choose a reason for hiding this comment

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

the isolated was removed but I have no idea why this is safe.

Copy link
Member

Choose a reason for hiding this comment

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

also iceberg CI is red.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. but #21736 is also a known flake, so i don't feel guilty.

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 isolated was removed but I have no idea why this is safe.

i have no idea why it was added in the first place

hashhar
hashhar approved these changes May 24, 2024
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM % comments.

@findepi findepi force-pushed the findepi/run-tests-with-more-parallelism-f1f758 branch from e91a8db to 438a6c6 Compare May 27, 2024 13:40
@findepi
Copy link
Member Author

findepi commented May 27, 2024

green

@hashhar
Copy link
Member

hashhar commented May 28, 2024

this time both TestIcebergMinioOrcConnectorTest.testOptimizeTimePartitionedTable and TestIcebergMinioOrcConnectorTest.testIfRemoveOrphanFilesCleansUnnecessaryDataFilesInPartitionedTable failed.

@findepi
Copy link
Member Author

findepi commented May 28, 2024

both seem same cause.

@findepi findepi mentioned this pull request May 28, 2024
Open
@findepi findepi force-pushed the findepi/run-tests-with-more-parallelism-f1f758 branch from ba4d567 to 286c9ec Compare May 28, 2024 09:28
@findepi
Copy link
Member Author

findepi commented May 28, 2024

BaseIcebergConnectorTest.java changed moved out to #22172

@findepi
Run tests with more parallelism
c411a8c 
During migration from TestNG to JUnit `@TestInstance(PER_CLASS)`
annotation was added, but it implies single-threaded execution. Restore
previous parallelism: either add `@Execution(CONCURRENT)` or inherit it
from base class.
@findepi
Copy link
Member Author

findepi commented May 28, 2024

CI

@findepi findepi force-pushed the findepi/run-tests-with-more-parallelism-f1f758 branch from 35e75ed to 0b41912 Compare May 28, 2024 13:48
@findepi findepi force-pushed the findepi/run-tests-with-more-parallelism-f1f758 branch from 0b41912 to 3af6b9d Compare May 28, 2024 13:59
@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jun 18, 2024
@findepi findepi added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashhar hashhar hashhar approved these changes

@wendigo wendigo Awaiting requested review from wendigo

@losipiuk losipiuk Awaiting requested review from losipiuk

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector no-release-notes This pull request does not require release notes entry stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. test
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@findepi @hashhar