[SPARK-26839][SQL] Work around classloader changes in Java 9 for Hive isolation #24057
Conversation
…larify fact that boot classloader is just 'null'
| val baseClassLoader: ClassLoader = Thread.currentThread().getContextClassLoader, | ||
| val sharedPrefixes: Seq[String] = Seq.empty, | ||
| val barrierPrefixes: Seq[String] = Seq.empty) | ||
| extends Logging { | ||
|
|
||
| // Check to make sure that the root classloader does not know about Hive. | ||
| assert(Try(rootClassLoader.loadClass("org.apache.hadoop.hive.conf.HiveConf")).isFailure) |
There was a problem hiding this comment.
This always failed before with an NPE. The root classloader is represented by null!
| name.startsWith("java.lang.") || | ||
| name.startsWith("java.net") || | ||
| name.startsWith("java.") || | ||
| name.startsWith("javax.") || |
There was a problem hiding this comment.
javax.sql et al were not found without this change. I think they should always have been considered non-shared classes? they're in the JDK from Java 8 at least.
There was a problem hiding this comment.
yes I found the same, this seems like a mistake. I also noticed that I needed to add at least
// needed for at least com.sun.security.auth.module.UnixLoginModule
name.startsWith("com.sun") ||
When I ran some tests w/ security. I wondered whether I should make that rule even tighter, but I figured everything under com.sun is probably considered shared.
There was a problem hiding this comment.
Would it make sense to just do javax.sql in the starts with here given other things like javax.servlet or are those non-shareable as well?
| case _: ClassNotFoundException => | ||
| super.loadClass(name, resolve) | ||
| if (allJars.isEmpty) { | ||
| rootClassLoader |
There was a problem hiding this comment.
This was the tail end of my attempt to get this working; this part doesn't seem quite right.
In Java 9+, we won't have the JARs from the system classloader to construct a new ClassLoader here. (There are risky hacks to get it, but not going there yet.) But this case only comes up in "builtin" mode, where we want to use the JARs already on the classpath. I figured, why build a new ClassLoader? just use the current one.
This gets close but now can't find Spark-Hive integration classes like HiveClientImpl.
I think I need to try baseClassLoader here, and will do so.
Just raising my current state FYI to @squito , maybe @vanzin or @cloud-fan . I know it's been years since you've touched it, but maybe @andrewor14 or @marmbrus wants to see what I'm trying to do here.
| name.startsWith("java.lang.") || | ||
| name.startsWith("java.net") || | ||
| name.startsWith("java.") || | ||
| name.startsWith("javax.") || |
There was a problem hiding this comment.
yes I found the same, this seems like a mistake. I also noticed that I needed to add at least
// needed for at least com.sun.security.auth.module.UnixLoginModule
name.startsWith("com.sun") ||
When I ran some tests w/ security. I wondered whether I should make that rule even tighter, but I figured everything under com.sun is probably considered shared.
| if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) { | ||
| // Do nothing. The system classloader is no longer a URLClassLoader in Java 9, | ||
| // so it won't match the case in allJars above. It no longer exposes URLs of | ||
| // the system classpath |
There was a problem hiding this comment.
I have something slightly different here -- admittedly I wasn't really sure if this was the right thing to do or not, it was just a hack to let me get a little further, not principled at all.
// For java 11, allJars() does *not* get jars on the classpath added, so we add
// them here
val classPathJars = sys.props("java.class.path").split(":").map(new File(_).toURI().toURL())
logWarning(s"Classpath jars = ${classPathJars.mkString(",")}")
val jars = allJars(classLoader) ++ classPathJars
I think this will solve your HiveClientImpl issue ... but that doesn't mean its the right thing to do :P
There was a problem hiding this comment.
Ah nice call there. I'll try just avoiding a whole new classloader here too just to see if it works; for 'builtin' that seems simpler. But I am wary of changes. But I am wary of depending on something else in the env too, even though that's pretty safe.
| name.startsWith("java.lang.") || | ||
| name.startsWith("java.net") || | ||
| name.startsWith("java.") || | ||
| name.startsWith("javax.") || |
There was a problem hiding this comment.
Would it make sense to just do javax.sql in the starts with here given other things like javax.servlet or are those non-shareable as well?
|
|
|
For those keeping score, the https://issues.apache.org/jira/browse/HIVE-17632 |
|
Test build #103340 has finished for PR 24057 at commit
|
|
Test build #103332 has finished for PR 24057 at commit
|
|
Farther, after updating datanucleus, but I think we might be hitting an incompatibility with Hive, which uses a much older version. The I'm concerned that we're going to hit a wall with Java 9 compatibility with big dependencies like Hive and Scala. A Hive update may help here, not sure; the JIRA for support of all this in Hive is still open. |
|
Test build #103342 has finished for PR 24057 at commit
|
|
Yeah, these changes to datanucleus aren't going to work for the Java 8 build, even. I don't think they solve the problem for Java 11 anyway, unfortunately; it may really only work with new Hive versions. I'm going to revert the datanucleus part as I think the rest is, at least, progress and correct for Java 11, and doesn't affect Java 8. |
|
Test build #103397 has finished for PR 24057 at commit
|
|
@squito what do you think of this much? |
srowen
changed the title
| "Unable to locate hive jars to connect to metastore. " + | ||
| s"Please set ${HIVE_METASTORE_JARS.key}.") | ||
| if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) { | ||
| // Do nothing. The system classloader is no longer a URLClassLoader in Java 9, |
There was a problem hiding this comment.
Do you mean we are unable to detect the missing Hive jar with Java 9+?
There was a problem hiding this comment.
The code above tries to get the URLs of all the JARs in the classpath from the system classloader. This isn't available in Java 9. However these only seem to be needed in 'builtin' mode, in which case (I believe) we don't need to do this at all -- don't make a new ClassLoader, just use the current ClassLoader.
@squito has an alternative, to read the classpath off of java.class.path. I think that works too. It's less change, but, I was also wondering whether the whole idea of making a new ClassLoader in this mode isn't necessary anyway.
There was a problem hiding this comment.
I think your approach makes more sense. I was a little worried that someone would add a jar directly to classloader, from java code, which wouldn't show up on java.class.path. probably an unsupported gray area anyway, but this might make it work in a few more cases
|
looks reasonable -- just want to mention again about the extra rule I needed in the list of shared prefixes for security: I don't have any insight yet into the datanucleus issues -- my fork has a different version of hive so I'm not encountering that issue (or at least, not yet). the security thing may also be specific to my fork, though I doubt it. |
|
What version of Hive are you looking at? it would be good to know if version X of Hive is even sorta compatible with Java 9+. It seems like Datanucleus only officially supports it all in 5.2+, but older versions might happen to work. |
|
oh the hive version I'm looking at is heavily forked from any upstream version, so I dunno if it would really be that helpful. Nominally its based on hive 2.1.1. Looks like its using datanucleus 4.1.6, so maybe I'm going to hit the same problems you are. Can you give me an exact test case where you hit the issues w/ datanucleus? Probably I would hit the datanucleus errors, but its covered up by some other error which I hadn't dug into yet. (Sorry this work has been preempted a bit on my end) |
|
@squito OK good to know. If you were to revert the commit 3ad22b6 here and run on Java 11, you'd see the error at #24057 (comment) Of course, this may mean that we just can't support Hive older than X on Java 11. This particular failure, as I understand, is from testing against 'builtin' Hive, at 1.2.1. It feels like we'd have to update the built-in Hive version (which really ought to happen for 3.0 anyway), and if we need to test against older ones, just disable them for Java 11. I don't know which Hive version works, so any confirmation you have that later versions work would be a great data point. (Aside: we still have REPL-related problems in Java 11 which may be hard to resolve as it depends on Scala compile-time support for Java 9+. This may not be the last kind of blocker. Right now seeing if I can get this one in and then move ahead to fix the YARN failure I saw.) |
|
@squito what do you think of this much of the change? Don't know if it just works on Hive 2 but I think this much is necessary in any event |
|
It looks reasonable to me, but I'm a hesitant to say the classloader changes to the builtin mode are the right ones till we get everything else working. But if you feel pretty good about it, I won't object to you committing -- we can always make further adjustments. |
|
Yeah that's my take. It may not be the last change here to be sure. It would probably facilitate further testing |
|
Merged to master. I didn't resolve the JIRA though. |
…uiltin' Hive version for metadata client ### 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. It also seems that this separate classloader was unnecessary from the start, since the intent of 'builtin' mode is to use the JARs already existing on the regular classloader (as alluded to [here](#24057 (comment))). The isolated clientloader was originally added in #5876 in 2015. This bit in the PR description is the only mention of the behavior for "builtin": > attempt to discover the jars that were used to load Spark SQL and use those. This option is only valid when using the execution version of Hive. I can't follow the logic here; the user classloader clearly has all of the necessary Hive JARs, since that's where we're getting the JAR URLs from, so we could just use that directly instead of grabbing the URLs. When this was initially added, it only used the JARs from the user classloader, not any of its parents, which I suspect was the motivating factor (to try to avoid more Spark classes being duplicated inside of the isolated classloader, I guess). But that was changed a month later anyway in #6435 / #6459, so I think this may have basically been deadcode from the start. It has also caused at least one issue over the years, e.g. SPARK-21428, which disables the new-classloader behavior in the case of running inside of a CLI session. ### 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. Closes #40144 from xkrogen/xkrogen/SPARK-42539/hive-isolatedclientloader-builtin-user-jar-conflict-fix/java9strategy. Authored-by: Erik Krogen <xkrogen@apache.org> Signed-off-by: Chao Sun <sunchao@apple.com>
…uiltin' Hive version for metadata client ### 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. It also seems that this separate classloader was unnecessary from the start, since the intent of 'builtin' mode is to use the JARs already existing on the regular classloader (as alluded to [here](#24057 (comment))). The isolated clientloader was originally added in #5876 in 2015. This bit in the PR description is the only mention of the behavior for "builtin": > attempt to discover the jars that were used to load Spark SQL and use those. This option is only valid when using the execution version of Hive. I can't follow the logic here; the user classloader clearly has all of the necessary Hive JARs, since that's where we're getting the JAR URLs from, so we could just use that directly instead of grabbing the URLs. When this was initially added, it only used the JARs from the user classloader, not any of its parents, which I suspect was the motivating factor (to try to avoid more Spark classes being duplicated inside of the isolated classloader, I guess). But that was changed a month later anyway in #6435 / #6459, so I think this may have basically been deadcode from the start. It has also caused at least one issue over the years, e.g. SPARK-21428, which disables the new-classloader behavior in the case of running inside of a CLI session. ### 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. Closes #40144 from xkrogen/xkrogen/SPARK-42539/hive-isolatedclientloader-builtin-user-jar-conflict-fix/java9strategy. Authored-by: Erik Krogen <xkrogen@apache.org> Signed-off-by: Chao Sun <sunchao@apple.com>
…uiltin' Hive version for metadata client 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. 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. It also seems that this separate classloader was unnecessary from the start, since the intent of 'builtin' mode is to use the JARs already existing on the regular classloader (as alluded to [here](apache#24057 (comment))). The isolated clientloader was originally added in apache#5876 in 2015. This bit in the PR description is the only mention of the behavior for "builtin": > attempt to discover the jars that were used to load Spark SQL and use those. This option is only valid when using the execution version of Hive. I can't follow the logic here; the user classloader clearly has all of the necessary Hive JARs, since that's where we're getting the JAR URLs from, so we could just use that directly instead of grabbing the URLs. When this was initially added, it only used the JARs from the user classloader, not any of its parents, which I suspect was the motivating factor (to try to avoid more Spark classes being duplicated inside of the isolated classloader, I guess). But that was changed a month later anyway in apache#6435 / apache#6459, so I think this may have basically been deadcode from the start. It has also caused at least one issue over the years, e.g. SPARK-21428, which disables the new-classloader behavior in the case of running inside of a CLI session. No, except to protect Spark itself from potentially being broken by bad user JARs. 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.
…uiltin' Hive version for metadata client ### 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. Please note that this is a re-submit of #40144. That one introduced test failures, and potentially a real issue, because the PR works by setting `isolationOn = false` for `builtin` mode. In addition to adjusting the classloader, `HiveClientImpl` relies on `isolationOn` to determine if it should use an isolated copy of `SessionState`, so the PR inadvertently switched to using a shared `SessionState` object. I think we do want to continue to have the isolated session state even in `builtin` mode, so this adds a new flag `sessionStateIsolationOn` which controls whether the session state should be isolated, _separately_ from the `isolationOn` flag which controls whether the classloader should be isolated. Default behavior is for `sessionStateIsolationOn` to be set equal to `isolationOn`, but for `builtin` mode, we override it to enable session state isolated even though classloader isolation is turned off. ### 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. It also seems that this separate classloader was unnecessary from the start, since the intent of 'builtin' mode is to use the JARs already existing on the regular classloader (as alluded to [here](#24057 (comment))). The isolated clientloader was originally added in #5876 in 2015. This bit in the PR description is the only mention of the behavior for "builtin": > attempt to discover the jars that were used to load Spark SQL and use those. This option is only valid when using the execution version of Hive. I can't follow the logic here; the user classloader clearly has all of the necessary Hive JARs, since that's where we're getting the JAR URLs from, so we could just use that directly instead of grabbing the URLs. When this was initially added, it only used the JARs from the user classloader, not any of its parents, which I suspect was the motivating factor (to try to avoid more Spark classes being duplicated inside of the isolated classloader, I guess). But that was changed a month later anyway in #6435 / #6459, so I think this may have basically been deadcode from the start. It has also caused at least one issue over the years, e.g. SPARK-21428, which disables the new-classloader behavior in the case of running inside of a CLI session. ### 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. Unit tests failing in #40144 have been locally tested (`HiveUtilsSuite`, `HiveSharedStateSuite`, `HiveCliSessionStateSuite`, `JsonHadoopFsRelationSuite`). Closes #40224 from xkrogen/xkrogen/SPARK-42539/hive-isolatedclientloader-builtin-user-jar-conflict-fix/take2. Authored-by: Erik Krogen <xkrogen@apache.org> Signed-off-by: Chao Sun <sunchao@apple.com>
Note, this doesn't really resolve the JIRA, but makes the changes we can make so far that would be required to solve it.
What changes were proposed in this pull request?
Java 9+ changed how ClassLoaders work. The two most salient points:
How was this patch tested?
We'll see whether Java 8 tests still pass here. Java 11 tests do not fully pass at this point; more notes below. This does make progress on the failures though.
(NB: to test with Java 11, you need to build with Java 8 first, setting JAVA_HOME and java's executable correctly, then switch both to Java 11 for testing.)