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

Properly produce JDBC non-shaded jar #23446

Closed
wants to merge 1 commit into from
Closed

Properly produce JDBC non-shaded jar #23446

wants to merge 1 commit into from

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Sep 16, 2024 •
edited
Loading

This introduces a new module that publishes non-shaded JAR along with a POM containing transitive dependencies and the shaded JAR along with a POM that doesn't contain transitive dependencies. For backward compatibility, the shaded JAR is named trino-jdbc while non-shaded is trino-jdbc-non-shaded.

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# JDBC
* The JDBC driver is now published as both a shaded `trino-jdbc` jar (including transitive dependencies) and a non-shaded `trino-jdbc-core` jar (without transitive dependencies). Previously added `non-shaded` classifier was removed from `trino-jdbc`.

@cla-bot cla-bot bot added the cla-signed label Sep 16, 2024
@wendigo wendigo changed the title Properly produce JDBC non-shaded jar Properly produce JDBC thin (non-shaded) jar Sep 16, 2024
@github-actions github-actions bot added docs jdbc Relates to Trino JDBC driver labels Sep 16, 2024
@wendigo wendigo force-pushed the serafin/non-shaded-jar branch 3 times, most recently from d035f3d to fcfcaa5 Compare September 16, 2024 14:41
martint
martint reviewed Sep 16, 2024
View reviewed changes
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@wendigo
Properly produce JDBC non-shaded jar
3bdf2cd 
This introduces a new module that publishes unshaded JDBC JAR along with a POM
containing transitive dependencies and the fat JAR along with a POM
that doesn't contain transitive dependencies. For backward compatibility,
the fat JAR is named `trino-jdbc` while unshaded is `trino-jdbc-core`.
@wendigo
Copy link
Contributor Author

wendigo commented Sep 16, 2024

I'd like to get this into next release @martint

mosabua
mosabua approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment •
edited
Loading

Choose a reason for hiding this comment

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

After a change to use "core" and suggestion of a release note entry I'm good with this

@wendigo
Copy link
Contributor Author

wendigo commented Sep 16, 2024

@mosabua renamed, RN added

@wendigo wendigo changed the title Properly produce JDBC thin (non-shaded) jar Properly produce JDBC core (non-shaded) jar Sep 16, 2024
@wendigo wendigo changed the title Properly produce JDBC core (non-shaded) jar Properly produce JDBC non-shaded jar Sep 16, 2024
@electrum
Copy link
Member

What is the purpose of doing this? JDBC drivers should not have dependencies.

@wendigo
Copy link
Contributor Author

wendigo commented Sep 16, 2024

@electrum using JDBC driver in an application where you want to control dependencies and their versions (i.e. you could exclude aircompressor and use newer one that is targeting JDK 22 instead of 8)

mosabua
mosabua reviewed Sep 16, 2024
View reviewed changes
pom.xml Show resolved Hide resolved
mosabua
mosabua approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Ship it

@electrum
Copy link
Member

In that case, then we should add another shaded JDBC driver artifact for modern JDKs. We don't produce libraries with the expectation that users will swap out dependencies and mess around with the internals.

We can chat offline about this if that's helpful.

@losipiuk
Copy link
Member

Release notes does not seem to much PR desc

@wendigo wendigo closed this Sep 16, 2024
@mosabua
Copy link
Member

mosabua commented Sep 16, 2024

Why is this closed? The currrent impl in master is busted and this is a good improvement and fix

@wendigo
Copy link
Contributor Author

wendigo commented Sep 16, 2024

@mosabua @electrum would like to revert non-shaded jdbc altogether

@mosabua mosabua mentioned this pull request Sep 17, 2024
Merged
nineinchnick
nineinchnick reviewed Sep 17, 2024
View reviewed changes
client/trino-jdbc/pom.xml
<relocation>
<pattern>io.opentelemetry.api.incubator</pattern>
<shadedPattern>${shadeBase}.opentelemetry.api.incubator</shadedPattern>
<pattern>io.opentelemetry</pattern>
Copy link
Member

Choose a reason for hiding this comment

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

Does this fix #21689 or any other similar issue (there's like 3)?

@wendigo wendigo deleted the serafin/non-shaded-jar branch October 2, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nineinchnick nineinchnick nineinchnick left review comments

@mosabua mosabua mosabua approved these changes

@electrum electrum Awaiting requested review from electrum

@losipiuk losipiuk Awaiting requested review from losipiuk

@ebyhr ebyhr Awaiting requested review from ebyhr

@hashhar hashhar Awaiting requested review from hashhar

@martint martint Awaiting requested review from martint

Assignees
No one assigned
Labels
cla-signed docs jdbc Relates to Trino JDBC driver
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wendigo @electrum @losipiuk @mosabua @martint @nineinchnick