[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[parthchandra]

Github user parthchandra closed the pull request at:

https://github.com/apache/drill/pull/991


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r145291048
  
--- Diff: exec/java-exec/pom.xml ---
@@ -22,7 +22,10 @@
 1.8-rev1
 
 
+
--- End diff --

Please uncomment (should be harmful) or move to openssl profile.


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r145289493
  
--- Diff: exec/java-exec/pom.xml ---
@@ -693,6 +699,19 @@
 
   
 
+
+  openssl
+  
+
+  io.netty
+  netty-tcnative
+  2.0.1.Final
--- End diff --

Please add provided scope.


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r145288182
  
--- Diff: exec/java-exec/pom.xml ---
@@ -701,18 +707,21 @@
   
-  
-kr.motd.maven
-os-maven-plugin
-1.5.0.Final
-  
-
+  
--- End diff --

Updated the PR with the latest recommendations. Using a different profile 
seems to work well.


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r145008784
  
--- Diff: exec/java-exec/pom.xml ---
@@ -701,18 +707,21 @@
   
-  
-kr.motd.maven
-os-maven-plugin
-1.5.0.Final
-  
-
+  
--- End diff --

Sounds like a hack. I'd recommend to keep it commented out or try 
`.mvn/extensions.xml` solution.


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r144960078
  
--- Diff: exec/java-exec/pom.xml ---
@@ -701,18 +707,21 @@
   
-  
-kr.motd.maven
-os-maven-plugin
-1.5.0.Final
-  
-
+  
--- End diff --

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?



---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r144697258
  
--- Diff: exec/java-exec/pom.xml ---
@@ -701,18 +707,21 @@
   
-  
-kr.motd.maven
-os-maven-plugin
-1.5.0.Final
-  
-
+  
--- End diff --

Looking at netty code it seems to use 
`Class.forName("io.netty.internal.tcnative.SSL", false, 
OpenSsl.class.getClassLoader());` ([see 
OpenSsl](https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/OpenSsl.java)),
 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.


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r144666062
  
--- Diff: exec/java-exec/pom.xml ---
@@ -701,18 +707,21 @@
   
-  
-kr.motd.maven
-os-maven-plugin
-1.5.0.Final
-  
-
+  
--- End diff --

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.


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r144659314
  
--- Diff: exec/java-exec/pom.xml ---
@@ -701,18 +707,21 @@
   
-  
-kr.motd.maven
-os-maven-plugin
-1.5.0.Final
-  
-
+  
--- End diff --

For example `-P apache,openssl'. Another option will be to enable openssl 
profile based on a property.


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[parthchandra]

Github user parthchandra commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r144651132
  
--- Diff: exec/java-exec/pom.xml ---
@@ -701,18 +707,21 @@
   
-  
-kr.motd.maven
-os-maven-plugin
-1.5.0.Final
-  
-
+  
--- End diff --

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.


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[vrozov]

Github user vrozov commented on a diff in the pull request:

https://github.com/apache/drill/pull/991#discussion_r144624216
  
--- Diff: exec/java-exec/pom.xml ---
@@ -701,18 +707,21 @@
   
-  
-kr.motd.maven
-os-maven-plugin
-1.5.0.Final
-  
-
+  
--- End diff --

Can it be in a separate profile (openssl) that is disabled by default?


---

[GitHub] drill pull request #991: DRILL-5876: Remove netty-tcnative dependency from j...

[parthchandra]

GitHub user parthchandra opened a pull request:

https://github.com/apache/drill/pull/991

DRILL-5876: Remove netty-tcnative dependency from java-exec/pom.xml

Commented out the netty-tcnative dependency. Tested build on command line, 
intellij. Ran unit tests.
@paul-rogers, @vrozov  Can you guys please review?


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/parthchandra/drill DRILL-5876

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/drill/pull/991.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #991


commit 9bf86429958f2e04843dc91322f3f75d9e34fde3
Author: Parth Chandra 
Date:   2017-10-13T17:29:31Z

DRILL-5876: Remove netty-tcnative dependency from java-exec/pom.xml




---