[SPARK-27121][REPL] Resolve Scala compiler failure for Java 9+ in REPL #24239
Conversation
| val classpath = paths.map(new File(_).getAbsolutePath).mkString(File.pathSeparator) | ||
| System.setProperty(CONF_EXECUTOR_CLASSPATH, classpath) | ||
| classpath | ||
| case _ => System.getProperty("java.class.path") |
There was a problem hiding this comment.
@squito this is your suggestion. I wonder why we don't just use this in all cases, Java 11 or no?
There was a problem hiding this comment.
the only reason I can think of is the same reason I thought your change was better -- if somebody directly modifies the class path from within the JVM, then it won't show up in this system property.
But OTOH, this is our test code, so we control what's happening to the classpath, and it might as well be the same in java 11 and java 8 -- so I think this is fine.
|
Test build #104074 has finished for PR 24239 at commit
|
| val classpath = paths.map(new File(_).getAbsolutePath).mkString(File.pathSeparator) | ||
| System.setProperty(CONF_EXECUTOR_CLASSPATH, classpath) | ||
| classpath | ||
| case _ => System.getProperty("java.class.path") |
There was a problem hiding this comment.
the only reason I can think of is the same reason I thought your change was better -- if somebody directly modifies the class path from within the JVM, then it won't show up in this system property.
But OTOH, this is our test code, so we control what's happening to the classpath, and it might as well be the same in java 11 and java 8 -- so I think this is fine.
| System.clearProperty(CONF_EXECUTOR_CLASSPATH) | ||
| } else { | ||
| System.setProperty(CONF_EXECUTOR_CLASSPATH, oldExecutorClasspath) | ||
| } |
There was a problem hiding this comment.
I don't understand your changes to the handling of CONF_EXECUTOR_CLASSPATH. The old code saved the previous value, modified it, ran the tests, then restored the original value (which probably should have been in a finally). But it seems you're just resetting it to the same value it already has?
There was a problem hiding this comment.
This is my error, let me fix that. That behavior should be retained.
|
btw, how are you running tests? My approach has been to build everything with jdk8, switch to jdk11 and then run just the scalatest plugin ( |
|
I have been using Java 8 to compile, and Java 11 to test, yes. I set Yeah I'd want to be pretty sure that's valid before declaring everything working for Java 11, but it is letting me turn up Java 11-specific issues. That is, I don't expect that there's an issue that's hiding because somehow a class was compiled by Java 11, but it's possible. I expect the problems to be with the JDK classes and dependencies. I think we're going to call this support 'experimental' for 3.0 in any event as we may not ultimately resolve the Hive problems. |
|
At some point, I discovered that just setting JAVA_HOME wasn't enough -- I also had to make sure the right version of java was first on the path. I forget whether this was with sbt or mvn ... and maybe it was actually something else I was doing wrong and I misattributed the problem. But I wanted to mention it at least to you if things ever don't seem to be behaving as you expect |
|
Test build #104084 has finished for PR 24239 at commit
|
|
Yeah, I have to use |
|
(FWIW this updated change also still passes on Java 11) |
|
Merged to master. Let's see if the PR builder for Java 11 likes it now (because not sure it tests Hive) |
What changes were proposed in this pull request?
Avoid trying to extract the classpath of the environment from a URLClassLoader in Java 11, as the default classloader isn't one. Use
java.class.pathinstead.How was this patch tested?
Existing tests, manually tested under Java 11.