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

PHOENIX-7482 Replace uses of org.iq80.snappy:snappy with org.xerial.snappy:snappy-java #2039

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

apurtell
Copy link
Contributor

@apurtell apurtell commented Dec 9, 2024

No description provided.

@apurtell apurtell requested review from stoty and virajjasani December 9, 2024 21:04
Copy link
Contributor

@stoty stoty left a comment

Choose a reason for hiding this comment

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

+1 LGTM

Copy link
Contributor

@virajjasani virajjasani left a comment

Choose a reason for hiding this comment

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

+1

@apurtell
Copy link
Contributor Author

apurtell commented Dec 19, 2024

Pushed an update, see 24963c3
Xerial snappy cannot be shaded because there is a JNI component. Shading can't/won't rewrite the native symbols. The result is runtime link failures. We ran a local precommit at $dayjob and it passed, this issue was found later, so there may not be test coverage for this case.

@stoty
Copy link
Contributor

stoty commented Jan 9, 2025

It seems that Hadoop also depends on this library.

I wonder if that could cause JNI version conflict problems when running with external Hadoop ?
I haven't dug into this, just a thought.
(Actually, I'm not sure how we handle native dependencies when using external Hadoop at all)

@apurtell
Copy link
Contributor Author

The precommit test links are stale. Let me rebase this on the latest master and push the update to trigger another round. I did a local RAT check and it passed.

@apurtell
Copy link
Contributor Author

apurtell commented Jan 13, 2025

@stoty

I wonder if that could cause JNI version conflict problems when running with external Hadoop ?

The native code is shipped as a resource inside the jar and is dynamically loaded from the jar by the Java code also shipped in that jar. It is triggered by the Snappy class static initializer and calls SnappyLoader which will load the native libraries with class level synchronization. So this will be done only once per JVM, in a way that prevents version mismatch, no matter if even multiple versions of the xerial snappy jar are on the classpath.

@apurtell
Copy link
Contributor Author

Ok, of the reported issues, the ASF license problems are not related to this patch:

phoenix-client-parent/target/javadoc-bundle-options/javadoc-options-javadoc-resources.xml:1:Missing Apache License
phoenix-server/target/javadoc-bundle-options/javadoc-options-javadoc-resources.xml:1:Missing Apache License
phoenix-mapreduce-byo-shaded-hbase/target/javadoc-bundle-options/javadoc-options-javadoc-resources.xml:1:Missing Apache License

I've pushed a fix for the checkstyle findings. Line length issues.

WALRecoveryRegionPostOpenIT seems flaky, in that it usually passes for me locally both on current master and master plus this change, and also sometimes it doesn't pass, and that does not seem tied to the change either.

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.

3 participants