You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "xkrogen (via GitHub)" <gi...@apache.org> on 2023/02/23 18:03:16 UTC
[GitHub] [spark] xkrogen opened a new pull request, #40144: [SPARK-42539][SQL][HIVE] Elminiate separate classloader when using 'builtin' Hive version for metadata client
xkrogen opened a new pull request, #40144:
URL: https://github.com/apache/spark/pull/40144
### What changes were proposed in this pull request?
When using the 'builtin' Hive version for the Hive metadata client, do not create a separate classloader, and rather continue to use the overall user/application classloader (regardless of Java version). This standardizes the behavior for all Java versions with that of Java 9+. See SPARK-42539 for more details on why this approach was chosen.
### Why are the changes needed?
Please see a much more detailed description in SPARK-42539. The tl;dr is that user-provided JARs (such as `hive-exec-2.3.8.jar`) take precedence over Spark/system JARs when constructing the classloader used by `IsolatedClientLoader` on Java 8 in 'builtin' mode, which can cause unexpected behavior and/or breakages. This violates the expectation that, unless user-first classloader mode is used, Spark JARs should be prioritized over user JARs.
### Does this PR introduce _any_ user-facing change?
No, except to protect Spark itself from potentially being broken by bad user JARs.
### How was this patch tested?
This includes a new unit test in `HiveUtilsSuite` which demonstrates the issue and shows that this approach resolves it. It has also been tested on a live cluster running Java 8 and Hive communication functionality continues to work as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1445933911
lgtm if all tests pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1447543451
Seems like the tests didn't pass .. I am reverting this as it causes a lot of test failures. e.g.)
- https://github.com/apache/spark/runs/11644563592
- https://github.com/apache/spark/actions/runs/4288729207/jobs/7471072831
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1447241820
Merged to master/branch-3.4. Thanks @xkrogen !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao closed pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao closed pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
URL: https://github.com/apache/spark/pull/40144
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1447565379
Thank you for recovering `master` branch by reverting, @HyukjinKwon ! The reverting unblocks other PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xkrogen commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1447269849
Thanks @sunchao and @cloud-fan !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xkrogen commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1448966321
I agree as well. Posted a new PR at #40224.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1447566249
Shall we revert it from 3.4 as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1448614830
If you don't mind, shall we consider this for Apache Spark 3.5 only? `ClassLoader` issue has been tricky always in the community. We need enough time to stabilize in `master` branch to give a chance to be verified in several cases by different organizations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao commented on a diff in pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #40144:
URL: https://github.com/apache/spark/pull/40144#discussion_r1119075418
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveUtilsSuite.scala:
##########
@@ -77,6 +80,32 @@ class HiveUtilsSuite extends QueryTest with SQLTestUtils with TestHiveSingleton
}
}
+ test("SPARK-42539 User-provided JARs should not take precedence over builtin Hive JARs") {
Review Comment:
nit: add a colon after `SPARK-42539`
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveUtilsSuite.scala:
##########
@@ -19,13 +19,16 @@ package org.apache.spark.sql.hive
import org.apache.hadoop.conf.Configuration
import org.apache.hadoop.hive.conf.HiveConf.ConfVars
-
-import org.apache.spark.SparkConf
+import org.apache.spark.{SparkConf, TestUtils}
import org.apache.spark.deploy.SparkHadoopUtil
import org.apache.spark.sql.QueryTest
+import org.apache.spark.sql.catalyst.catalog.CatalogDatabase
import org.apache.spark.sql.hive.test.TestHiveSingleton
import org.apache.spark.sql.test.SQLTestUtils
-import org.apache.spark.util.ChildFirstURLClassLoader
+import org.apache.spark.util.{ChildFirstURLClassLoader, MutableURLClassLoader}
+
+import java.io.File
Review Comment:
nit: I think we usually put `java.` imports at the beginning
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xkrogen commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1448420938
I will take a look at the test failures, thanks @HyukjinKwon for addressing the revert!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1447576391
Hmm interesting. Somehow the tests were shown all passing for me when I merged this. Sorry for the trouble.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40144: [SPARK-42539][SQL][HIVE] Elminiate separate classloader when using 'builtin' Hive version for metadata client
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1442251188
Also, cc @mridulm , @cloud-fan , @rednaxelafx, @zsxwing, @kiszk , @maropu .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #40144: [SPARK-42539][SQL][HIVE] Elminiate separate classloader when using 'builtin' Hive version for metadata client
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1442714986
It makes sense to use the builtin classloader when using builtin Hive. To clarification: we still have the class loading issue if people specifies a certain hive version (not builtin), right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1447568429
Yes, it's reverted here, 26009d47c1.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sunchao commented on pull request #40144: [SPARK-42539][SQL][HIVE] Eliminate separate classloader when using 'builtin' Hive version for metadata client
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1448644334
+1. I agree with @dongjoon-hyun .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xkrogen commented on pull request #40144: [SPARK-42539][SQL][HIVE] Elminiate separate classloader when using 'builtin' Hive version for metadata client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1442227955
cc @sunchao @AngersZhuuuu since you've worked on somewhat-related changes in #34690, #32887, etc.
cc @srowen @squito since you were involved in #24057 for the Java 9+ changes
cc @HyukjinKwon @dongjoon-hyun for any general interest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xkrogen commented on pull request #40144: [SPARK-42539][SQL][HIVE] Elminiate separate classloader when using 'builtin' Hive version for metadata client
Posted by "xkrogen (via GitHub)" <gi...@apache.org>.
xkrogen commented on PR #40144:
URL: https://github.com/apache/spark/pull/40144#issuecomment-1444004552
Great question @cloud-fan , and actually no, we don't. For all of the other values of `spark.sql.hive.metastore.jars` besides 'builtin', the user JARs are not included at all ([refer to this section of HiveUtils](https://github.com/apache/spark/blob/87e3d5625e76bb734b8dd753bfb25002822c8585/sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveUtils.scala#L451-L513)). In all of those cases, the JAR list is constructed purely from the dependencies for the specified Hive version. Whether that behavior is correct is another question -- @shardulm94 informed me that user JARs are required to support custom serdes inside of the Hive client -- but in any case, 'builtin' is the only mode that is susceptible to this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org