-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-29015][SQL][test-hadoop3.2]Reset class loader after initializing SessionState for built-in Hive 2.3 #25775
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
Conversation
|
ok to test. |
|
Test build #110523 has finished for PR 25775 at commit
|
This reverts commit edaf402.
|
Retest this please. |
ThriftServer MetaData Operation and CLI have some rely on SessionState. Some code rely on hive code too much. |
|
Test build #110529 has finished for PR 25775 at commit
|
|
Test build #110532 has finished for PR 25775 at commit
|
|
Test build #110548 has finished for PR 25775 at commit
|
|
Test build #110551 has finished for PR 25775 at commit
|
|
Test build #110555 has finished for PR 25775 at commit
|
|
@dongjoon-hyun |
|
Retest this please. |
|
Test build #110561 has finished for PR 25775 at commit
|
|
Test build #110587 has finished for PR 25775 at commit
|
|
Test build #110588 has finished for PR 25775 at commit
|
sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/HiveTableScanExec.scala
Outdated
Show resolved
Hide resolved
|
Test build #110640 has finished for PR 25775 at commit
|
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala
Outdated
Show resolved
Hide resolved
|
This PR tested by HiveThriftBinaryServerSuite#test add jar? |
|
Test build #110771 has finished for PR 25775 at commit
|
|
Test build #110805 has finished for PR 25775 at commit
|
|
@AngersZhuuuu Could you add these to How was this patch tested?: export JAVA_HOME=/usr/lib/jdk-11.0.3
export PATH=$JAVA_HOME/bin:$PATH
build/sbt -Phive-thriftserver -Phadoop-3.2
hive/test-only *HiveSparkSubmitSuite -- -z "SPARK-8368: includes jars passed in through --jars"
hive-thriftserver/test-only *HiveThriftBinaryServerSuite -- -z "test add jar" |
|
I see. Thank you @srowen |
|
Test build #111360 has finished for PR 25775 at commit
|
|
Could we revise the PR title more specifically, @AngersZhuuuu and @wangyum ? |
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala
Show resolved
Hide resolved
| // got changed. We reset it to clientLoader.ClassLoader here. | ||
| if (HiveUtils.isHive23) { | ||
| 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.
@gatorsmile @srowen @HyukjinKwon @dongjoon-hyun @juliuszsompolski Do you have more comments?
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.
No opinion from me, I know too little context of how these classloaders should be used to review.
|
Merged to master |
| // since HIVE-11878, and ADDJarCommand will add jars to clientLoader.classLoader. | ||
| // For this reason we cannot load the jars added by ADDJarCommand because of class loader | ||
| // got changed. We reset it to clientLoader.ClassLoader here. | ||
| if (HiveUtils.isHive23) { |
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.
Why not set the classLoader blindly to clientLoader.classLoader? HIVE-11878 got merged into hive 1.3.0 and 2.0.0? This issue can still exist in the production environment when we try to use non-builtin hive libraries.
What changes were proposed in this pull request?
Hive 2.3 will set a new UDFClassLoader to hiveConf.classLoader when initializing SessionState since HIVE-11878, and
hive.aux.jarsSPARK-28954 SPARK-28840 will be added to clientLoader.classLoader tooFor these reason we cannot load the jars added by ADDJarCommand because of class loader got changed. We reset it to clientLoader.ClassLoader here.
Why are the changes needed?
support for jdk11
Does this PR introduce any user-facing change?
NO
How was this patch tested?
UT