DRILL-5876: Remove netty-tcnative dependency from java-exec/pom.xml #991
Conversation
| <version>1.5.0.Final</version> | ||
| </extension> | ||
| </extensions> | ||
| <!-- Uncomment the following to get a debug build that allows openssl support --> |
There was a problem hiding this comment.
Can it be in a separate profile (openssl) that is disabled by default?
There was a problem hiding this comment.
How would that work in conjunction with the main pom's profiles? I would want to have both the (apache | mapr | cdh | hdp) profile and the openssl profile enabled.
There was a problem hiding this comment.
For example `-P apache,openssl'. Another option will be to enable openssl profile based on a property.
There was a problem hiding this comment.
How about we forget the whole os dependent stuff and put in all the variants (there are only four) and make the scope for all of them provided?
Since netty is internally loading the correct jar based on the os, all we need to do is make sure that the jar files are in the classpath.
There was a problem hiding this comment.
Looking at netty code it seems to use Class.forName("io.netty.internal.tcnative.SSL", false, OpenSsl.class.getClassLoader()); (see OpenSsl), so if all netty-tcnative OS dependent jars are on the classpath, it should load the first in the classpath (if delegated to the system classloader) or one that OpenSsl classloader finds. My understanding is that OpenSsl class is loaded by the system classloader (I may be wrong), but in this case, having all variants of netty-tcnative on the classpath, will not resolve the issue as netty will try to load "io.netty.internal.tcnative.SSL" class only from one of the jars and if it happens to be a wrong jar, will disable OpenSsl functionality.
There was a problem hiding this comment.
OK. So I tried to put the commented out code in a 'openssl' profile and discovered that maven does not allow the tag inside a profile. Since the problem is essentially caused by this extension being included, not being able to put it in the 'openssl' profile makes this useless.
There is a sort of a convoluted hack around this described here: https://stackoverflow.com/questions/17639778/maven-3-profile-with-extensions.
IMO this introduces a bit of unreadable, magical stuff in the pom which I would rather avoid.
What do you think?
There was a problem hiding this comment.
Sounds like a hack. I'd recommend to keep it commented out or try .mvn/extensions.xml solution.
There was a problem hiding this comment.
Updated the PR with the latest recommendations. Using a different profile seems to work well.
detection plugin. Include netty-tcnative only if the openssl profile is enabled. netty-tcnative is no longer provided scope see it is included specifically in a profile. DRILL-5876: Update comment
| <dependency> | ||
| <groupId>io.netty</groupId> | ||
| <artifactId>netty-tcnative</artifactId> | ||
| <version>2.0.1.Final</version> |
| @@ -22,7 +22,10 @@ | |||
| <libpam4j.version>1.8-rev1</libpam4j.version> | |||
| <!-- Configure the os-maven-plugin extension to expand the classifier on --> | |||
| <!-- Fedora-"like" systems. Used for netty-tcnative inclusion --> | |||
| <!-- Uncomment the following to get a debug build that allows openssl support --> | |||
There was a problem hiding this comment.
Please uncomment (should be harmful) or move to openssl profile.
|
@vrozov per your other tests, this is still broken for eclipse. So it seems that the best bet is to comment out the dependency and the os extension. Developers needing to debug, will need to uncomment the dependency. |
|
@vrozov your proposed solution is better and works for intellij and eclipse. Closing this PR and will wait for you to submit a new PR. |
|
@parthchandra open PR #1004 |
Commented out the netty-tcnative dependency. Tested build on command line, intellij. Ran unit tests.
@paul-rogers, @vrozov Can you guys please review?