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

Improve maven setup for Maven 4.x compatibility #20806

Merged
merged 3 commits into from
Feb 27, 2024
Merged

Improve maven setup for Maven 4.x compatibility #20806

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f283227
Install instead of packaging while checking if commit compiles
wendigo Feb 22, 2024
4e698fd
Define confluent repository for server assembly
wendigo Feb 22, 2024
5b52992
Define maven wrapper plugin version
wendigo Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/actions/compile-commit/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ runs:

# For building with Maven we need MAVEN_OPTS to equal MAVEN_INSTALL_OPTS
export MAVEN_OPTS="${MAVEN_INSTALL_OPTS}"
$MAVEN package \
$MAVEN install \
${MAVEN_COMPILE_COMMITS} `# defaults, kept in sync with ci.yml` \
-Dair.check.skip-all=false -Dair.check.skip-basic=true -Dair.check.skip-extended=true -Dair.check.skip-checkstyle=false \
${MAVEN_GIB}
Expand Down
11 changes: 11 additions & 0 deletions core/trino-server/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

trino/plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/schema/confluent/ConfluentModule.java

Lines 189 to 190 in b90fe09

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

trino/plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/schema/confluent/ConfluentModule.java

Lines 256 to 257 in b90fe09

// 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.

<repositories>
<repository>
<snapshots>
<enabled>false</enabled>
</snapshots>
<id>confluent</id>
<url>https://packages.confluent.io/maven/</url>
wendigo marked this conversation as resolved.
Show resolved Hide resolved
</repository>
</repositories>

<build>
<plugins>
<plugin>
Expand Down