You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by sohami <gi...@git.apache.org> on 2018/01/03 00:13:57 UTC
[GitHub] drill pull request #1079: DRILL-6063: Set correct ThreadContext ClassLoader ...
GitHub user sohami opened a pull request:
https://github.com/apache/drill/pull/1079
DRILL-6063: Set correct ThreadContext ClassLoader before using Hadoop…
… Configuration class in DrillClient
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sohami/drill DRILL-6063
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1079.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 #1079
----
commit e7a7273df06f0602c6ca154297b2db01a1181c33
Author: Sorabh Hamirwasia <sh...@...>
Date: 2018-01-02T18:35:19Z
DRILL-6063: Set correct ThreadContext ClassLoader before using Hadoop Configuration class in DrillClient
----
---
[GitHub] drill issue #1079: DRILL-6063: Set correct ThreadContext ClassLoader before ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/1079
Thanks for the review!
---
[GitHub] drill pull request #1079: DRILL-6063: Set correct ThreadContext ClassLoader ...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1079#discussion_r159487621
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java ---
@@ -344,7 +344,18 @@ private boolean clientNeedsAuthExceptPlain(DrillProperties props) {
mechanismName = factory.getSimpleName();
logger.trace("Will try to authenticate to server using {} mechanism with encryption context {}",
mechanismName, connection.getEncryptionCtxtString());
+
+ // Update the thread context class loader to current class loader
+ // See DRILL-6063 for detailed description
+ final ClassLoader oldThreadCtxtCL = Thread.currentThread().getContextClassLoader();
+ final ClassLoader newThreadCtxtCL = this.getClass().getClassLoader();
+ Thread.currentThread().setContextClassLoader(newThreadCtxtCL);
+
--- End diff --
Is this the only place we need to update? For eaxmple, with SASL the authentication mechanism plugin would have to be in the class path when the authentication method is dtermined. But you have already restored the thread context. (Or does the oldThreadContextCL have the SASL jars in the classpath?)
---
[GitHub] drill pull request #1079: DRILL-6063: Set correct ThreadContext ClassLoader ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/1079#discussion_r159500241
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java ---
@@ -344,7 +344,18 @@ private boolean clientNeedsAuthExceptPlain(DrillProperties props) {
mechanismName = factory.getSimpleName();
logger.trace("Will try to authenticate to server using {} mechanism with encryption context {}",
mechanismName, connection.getEncryptionCtxtString());
+
+ // Update the thread context class loader to current class loader
+ // See DRILL-6063 for detailed description
+ final ClassLoader oldThreadCtxtCL = Thread.currentThread().getContextClassLoader();
+ final ClassLoader newThreadCtxtCL = this.getClass().getClassLoader();
+ Thread.currentThread().setContextClassLoader(newThreadCtxtCL);
+
--- End diff --
This is the only place since this is when `Hadoop Configuration `object is created inside `createAndLoginUser`. SASL authentication mechanism factories are scanned before this code itself and moreover they are scanned in context of current class ClassLoader not in ThreadContext class loader.
---
[GitHub] drill pull request #1079: DRILL-6063: Set correct ThreadContext ClassLoader ...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on a diff in the pull request:
https://github.com/apache/drill/pull/1079#discussion_r159508398
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java ---
@@ -344,7 +344,18 @@ private boolean clientNeedsAuthExceptPlain(DrillProperties props) {
mechanismName = factory.getSimpleName();
logger.trace("Will try to authenticate to server using {} mechanism with encryption context {}",
mechanismName, connection.getEncryptionCtxtString());
+
+ // Update the thread context class loader to current class loader
+ // See DRILL-6063 for detailed description
+ final ClassLoader oldThreadCtxtCL = Thread.currentThread().getContextClassLoader();
+ final ClassLoader newThreadCtxtCL = this.getClass().getClassLoader();
+ Thread.currentThread().setContextClassLoader(newThreadCtxtCL);
+
--- End diff --
makes sense
---
[GitHub] drill issue #1079: DRILL-6063: Set correct ThreadContext ClassLoader before ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/1079
@parthchandra - Please help to review.
---
[GitHub] drill pull request #1079: DRILL-6063: Set correct ThreadContext ClassLoader ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1079
---