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

No-SNOW Update fasterxml #740

Merged
merged 4 commits into from
May 6, 2024
Merged

No-SNOW Update fasterxml #740

merged 4 commits into from
May 6, 2024

Conversation

sfc-gh-xhuang
Copy link
Contributor

Update to a newer version of fasterxml

Copy link
Contributor

@sfc-gh-asen sfc-gh-asen left a comment

Choose a reason for hiding this comment

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

Thank you

Copy link
Contributor

@sfc-gh-tzhang sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

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

LGTM, please fix the merge gate before merging

@@ -723,6 +723,7 @@
<banDuplicateClasses>
<ignoreClasses>
<ignoreClass>META-INF/versions/*/org/bouncycastle/*</ignoreClass>
<ignoreClass>META-INF/versions/*/com/fasterxml/jackson/core/io/doubleparser/*</ignoreClass>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we want to keep adding to this list to resolve duplicate class issue, I know that we have a long history of issue for boucycastle. I believe what we did before is to exclude the class from one of the package? cc: @sfc-gh-lsembera

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sfc-gh-lsembera any concerns here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bouncy Castle and now also Jackson use multi-release JARs, which means there are different versions of classes targeting different JVM versions. Our shading process loses these classes targeting specific Java versions, and only keeps the default version. It is probably ok because the default versions should work fine on newer Java runtimes, but we may be losing some performance improvements, so ideally we change our shading process to correctly relocate these classes, as well. I will try to find some time to look at it.

We also need better test coverage with newer Java versions, I opened #756. It is failing, so I need some more time to get it working. Please wait with merging this PR until #756 is merged.

@sfc-gh-xhuang
Copy link
Contributor Author

I "fixed" the merge gates by adding an ignoreclass. Is that the right way to do this?

cc: @sfc-gh-tzhang @sfc-gh-japatel

@sfc-gh-xhuang
Copy link
Contributor Author

Also @sfc-gh-lsembera @sfc-gh-tzhang Do you know where snyk is complaining about commons-configuration2?
I can't find where we define usage of that

@sfc-gh-tzhang
Copy link
Contributor

Also @sfc-gh-lsembera @sfc-gh-tzhang Do you know where snyk is complaining about commons-configuration2? I can't find where we define usage of that

Please check with @sfc-gh-lsembera , @sfc-gh-alhuang might also know more on this

@sfc-gh-lsembera
Copy link
Contributor

@sfc-gh-xhuang commons-configuration 2.8.0 is being pulled in transitively via hadoop-common. You can run mvn dependency:tree and then check target/dependency-list.txt.

@sfc-gh-lsembera sfc-gh-lsembera merged commit 83f4287 into master May 6, 2024
15 checks passed
@sfc-gh-lsembera sfc-gh-lsembera deleted the sfc-gh-xhuang-patch-1 branch May 6, 2024 20:53
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.

None yet

4 participants