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.

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

Improve maven setup for Maven 4.x compatibility #20806

Merged
merged 3 commits into from
Feb 27, 2024
Merged

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Feb 22, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Feb 22, 2024
@wendigo wendigo requested review from losipiuk and findepi February 22, 2024 12:45
@wendigo wendigo changed the title Improve maven setup for Maven 4.x compatiiblity Improve maven setup for Maven 4.x compatibility Feb 22, 2024
@wendigo wendigo requested a review from electrum February 22, 2024 12:52
pom.xml Show resolved Hide resolved
@wendigo wendigo force-pushed the serafin/improve-maven branch 3 times, most recently from 8882801 to 92871e0 Compare February 22, 2024 15:14
Since we don't have a cache for .m2, packaging on it's own won't install
assemblies which is required for provisio to build a server assembly.
@wendigo wendigo force-pushed the serafin/improve-maven branch from 92871e0 to 5b52992 Compare February 22, 2024 16:12
@wendigo
Copy link
Contributor Author

wendigo commented Feb 26, 2024

@findepi PTAL

@@ -38,6 +38,17 @@
</dependency>
</dependencies>

<!-- Needed for provisio to resolve transitive dependencies for server assembly -->
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually ship artifacts from this repository? I thought it was only for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. We shouldn't

Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't ship them, since those classes get loaded conditionally and need to be provided by the users.

See

// Make JVM to load lazily ProtobufSchemaProvider, so Kafka connector can be used
// without protobuf dependency for non protobuf based topics
and
// Make JVM to load lazily ProtobufSchemaParser, so Kafka connector can be used
// without protobuf dependency for non protobuf based topics

There's tests for that - https://github.com/trinodb/trino/blob/master/testing/trino-product-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/environment/EnvMultinodeConfluentKafka.java has the confluent jars on classpath while other env does not and we verify that nothing fails without those classes on classpath as long as you don't use specific functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. it seems that maven 4.x is just more strict. So it doesn't import repository definition from a dependency, which is a good thing I believe. It's just needed to resolve dependencies (provided one as well) but the jar is not part of the final build anyway. All good, just more strict!

Copy link
Member

Choose a reason for hiding this comment

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

We could probably avoid this by moving the Confluent code into a new module with <optional>true</optional>, so that the Kafka plugin won't have a direct dependency on it, but this is fine for now.

@wendigo wendigo merged commit ded0219 into master Feb 27, 2024
101 checks passed
@wendigo wendigo deleted the serafin/improve-maven branch February 27, 2024 23:43
@github-actions github-actions bot added this to the 440 milestone Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants