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

GH-3007: Ensure version specific Jackson classes are shaded #3008

Closed
wants to merge 1 commit into from

Conversation

ted-jenks
Copy link

@ted-jenks ted-jenks commented Sep 9, 2024

Rationale for this change

The current jar contains version specific classes under META-INF/versions/... with unshaded package names. This leads to clashes with jackson and thus classpath duplicates. This is a major problem in many big projects right now (e.g. Spark).

What changes are included in this PR?

Ensure these classes are shaded.

Are these changes tested?

Are there any user-facing changes?

Closes #3007

@ted-jenks
Copy link
Author

cc @wgtmac hey this is a pretty big issue for us. It is preventing us from taking other fixes. There might be a smarter way to do this but just wanted to kick this off - thanks 😄

@wgtmac
Copy link
Member

wgtmac commented Sep 10, 2024

@gszadovszky @Fokko Could you please help review this? Thanks!

@gszadovszky
Copy link
Contributor

@ted-jenks, have you tried this one?
Otherwise, I am fine with the current solution. My only fear is we'll forget to update with the new java versions. It would be nice if we could handle it with some automatism.

@ted-jenks
Copy link
Author

@gszadovszky I agree having the automation would be much better, but I cannot get that to work. When using <Multi-Release> the jar still have the unshaded classes. I think that the use of the relocations doesn't agree with it somehow? Not sure since it isn't really documented. The approach in this PR does work though.

@ted-jenks
Copy link
Author

Would it be possible to get a minor release of this? It is kinda dangerous to use the current release with anything that has a different jackson version.

@ted-jenks
Copy link
Author

@gszadovszky @Fokko @wgtmac any updates on this? sorry for pushing it is just blocking us taking some fixes right now.

@ted-jenks
Copy link
Author

@gszadovszky @Fokko @wgtmac 🙏🏻 would be super helpful to get this through

@gszadovszky
Copy link
Contributor

Sorry, @ted-jenks, I was on vacation. I've quickly checked the content of the jenkins jars on my local maven repo and found the following versions in META-INF/versions:
9, 11, 17, 19, and 21. Maybe, we should simply list every available java release here to be sure?
Unfortunately, I don't have time to manage a release currently. Maybe bring this up on the dev list after this PR went in.

@z-anderson
Copy link
Contributor

@gszadovszky Thank you, I addressed your comment. (I work with Ted). #3017 . This is blocking us

<shadedPattern>META-INF.versions.17.${shade.prefix}.${jackson.package}</shadedPattern>
</relocation>
<relocation>
<pattern>META-INF.versions.21.${jackson.package}</pattern>
Copy link
Member

Choose a reason for hiding this comment

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

Why do we pick up only 11/17/21 here?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the new PR #3017, we pick up 9/11/17/19/21

@wgtmac
Copy link
Member

wgtmac commented Sep 20, 2024

For the next release, I think we can discuss on the dev@parquet ML. It may be the time to directly release 1.15.0 instead of 1.14.x

@gszadovszky
Copy link
Contributor

@z-anderson, shall we close this PR in favor of #3017, then?

@z-anderson
Copy link
Contributor

@z-anderson, shall we close this PR in favor of #3017, then?
@gszadovszky I think yes

@gszadovszky
Copy link
Contributor

Closing this one in favor of #3017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version specific Jackson classes are left unshaded
4 participants