Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Move QuarkusSparkIT to a separate module #1109

Merged
merged 3 commits into from
Mar 6, 2025

Conversation

adutra
Copy link
Contributor

@adutra adutra commented Mar 4, 2025

The Spark integration tests require specific versions of some dependencies that conflicting more and more with the versions declared in the Quarkus BOM.

This PR simply isolates QuarkusSparkIT in its own module in order to alleviate the amount of tweaks we need to do in polaris-quarkus-service to get it to build correctly.

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

This change LGTM. The suggested change in the base tests module can be a follow-up PR (if we want to do that)

testFixturesApi(project(":polaris-tests"))
testFixturesApi(project(":polaris-tests")) {
// exclude all spark dependencies
exclude(group = "org.apache.iceberg", module = "iceberg-spark-3.5_2.12")
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to split polaris-tests into a general part (that only uses Iceberg Core) and a Spark-specific part (instead of excluding spark jars)?

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 4, 2025
@flyrain
Copy link
Contributor

flyrain commented Mar 4, 2025

Can we add QuarkusSparkIT to the module integration-tests instead of creating a new module? integration-tests holds the class PolarisSparkIntegrationTest which is the class QuarkusSparkIT inherits from. It makes more sense to put them together, as their functionalities and dependencies are similar.

@dimas-b
Copy link
Contributor

dimas-b commented Mar 4, 2025

Can we add QuarkusSparkIT to the module integration-tests instead of creating a new module?

The idea behind the integration-tests module is to allow reuse in different environments (including downstream projects).

QuarkusSparkIT executes the sharable test code in the specific env. of the Polaris Quarkus Server Integration Tests.

TBH, I do not see how it fits inside the integration-tests module.

@flyrain
Copy link
Contributor

flyrain commented Mar 4, 2025

The idea behind the integration-tests module is to allow reuse in different environments (including downstream projects).

QuarkusSparkIT executes the sharable test code in the specific env. of the Polaris Quarkus Server Integration Tests.

TBH, I do not see how it fits inside the integration-tests module.

Are we trying to support a Polaris server not powered by Quarkus? I'd assume not. In that case, the chances of reuse integration-tests module is rare. WDYT?

@dimas-b
Copy link
Contributor

dimas-b commented Mar 4, 2025

I mean any downstream build of Polaris. The integration-tests module allows running the sever in a variety of ways, not assuming any particular framework (like Quarkus Integration Tests).

@flyrain
Copy link
Contributor

flyrain commented Mar 4, 2025

I mean any downstream build of Polaris. The integration-tests module allows running the sever in a variety of ways, not assuming any particular framework (like Quarkus Integration Tests).

That would still be rare, as people need to rewrite a lot of code to bring it up with a non-Quarkus server. I am wondering how it could happen.

@dimas-b
Copy link
Contributor

dimas-b commented Mar 5, 2025

I mean running a custom server outside the Quarkus Integration Test framework. The server itself may very well be based on Quarkus, but running in docker, for example.

@adutra adutra force-pushed the isolated-spark-tests branch from e0a36fb to 4488eef Compare March 5, 2025 08:59
@adutra
Copy link
Contributor Author

adutra commented Mar 6, 2025

@flyrain are you OK merging this?

@flyrain
Copy link
Contributor

flyrain commented Mar 6, 2025

yeah, feel free to do so.

@adutra adutra merged commit 6df5117 into apache:main Mar 6, 2025
5 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Mar 6, 2025
@adutra adutra deleted the isolated-spark-tests branch March 6, 2025 18:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants