-
Notifications
You must be signed in to change notification settings - Fork 28k
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
[SPARK-19084][sql] Ensure context class loader is set when initializing Hive. #17154
Conversation
…ng Hive. A change in Hive 2.2 (most probably HIVE-13149) causes this code path to fail, since the call to "state.getConf.setClassLoader" does not actually change the context's class loader. Spark doesn't yet officially support Hive 2.2, but some distribution-specific metastore client libraries may have that change (as certain versions of CDH already do), and this also makes it easier to support 2.2 when it comes out. Tested with existing unit tests; we've also used this patch extensively with Hive metastore client jars containing the offending patch.
Test build #73865 has finished for PR 17154 at commit
|
// The classloader in clientLoader could be changed after addJar, always use the latest | ||
// classloader | ||
Thread.currentThread().setContextClassLoader(clientLoader.classLoader) | ||
state.getConf.setClassLoader(clientLoader.classLoader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like we fixed 2 problems here: 1. set thread context class loader. 2. set class loader back for state.getConf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; I'm not sure the second one is strictly necessary, but from a cleanliness p.o.v. it looks like the right thing to do.
// The classloader in clientLoader could be changed after addJar, always use the latest | ||
// classloader | ||
Thread.currentThread().setContextClassLoader(clientLoader.classLoader) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we add some comments to say why we need to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
LGTM |
retest this please |
FYI, this retest is to ensure the testing pick up the changes we just merged to support Hive metastore 2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending test
Test build #73878 has finished for PR 17154 at commit
|
Test build #73880 has finished for PR 17154 at commit
|
Thanks! Merging to master. |
@gatorsmile I want to understand the scope of this fix. |
A change in Hive 2.2 (most probably HIVE-13149) causes this code path to fail,
since the call to "state.getConf.setClassLoader" does not actually change the
context's class loader. Spark doesn't yet officially support Hive 2.2, but some
distribution-specific metastore client libraries may have that change (as certain
versions of CDH already do), and this also makes it easier to support 2.2 when it
comes out.
Tested with existing unit tests; we've also used this patch extensively with Hive
metastore client jars containing the offending patch.