From e0d95381ffc70c22bc2c8a27f908d97f565b2566 Mon Sep 17 00:00:00 2001 From: Kohki Nishio Date: Sun, 26 Feb 2017 08:22:03 -0800 Subject: [PATCH 1/7] Set parent classloader as null for ExecutorClassLoader This change is to address a classloader problem under sbt environment more details can be found in Jira [https://issues.apache.org/jira/browse/SPARK-19675] --- .../main/scala/org/apache/spark/repl/ExecutorClassLoader.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala b/repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala index df13b32451af2..6b291e376e5e5 100644 --- a/repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala +++ b/repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala @@ -44,7 +44,7 @@ class ExecutorClassLoader( env: SparkEnv, classUri: String, parent: ClassLoader, - userClassPathFirst: Boolean) extends ClassLoader with Logging { + userClassPathFirst: Boolean) extends ClassLoader(null) with Logging { val uri = new URI(classUri) val directory = uri.getPath From 0a490bbb48803a72b8dd5b3e7ccd4336bf1e4c46 Mon Sep 17 00:00:00 2001 From: Kohki Nishio Date: Sun, 26 Feb 2017 10:05:51 -0800 Subject: [PATCH 2/7] Add a UT for ExecutorClassLoader --- .../spark/repl/ExecutorClassLoaderSuite.scala | 41 ++++++++++++++++++- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala index 6d274bddb7782..9110c925da50d 100644 --- a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala +++ b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala @@ -23,10 +23,11 @@ import java.nio.channels.{FileChannel, ReadableByteChannel} import java.nio.charset.StandardCharsets import java.nio.file.{Paths, StandardOpenOption} import java.util +import java.util.{Arrays, Collections} +import javax.tools.{JavaFileObject, SimpleJavaFileObject, ToolProvider} import scala.io.Source import scala.language.implicitConversions - import com.google.common.io.Files import org.mockito.Matchers.anyString import org.mockito.Mockito._ @@ -34,7 +35,6 @@ import org.mockito.invocation.InvocationOnMock import org.mockito.stubbing.Answer import org.scalatest.BeforeAndAfterAll import org.scalatest.mock.MockitoSugar - import org.apache.spark._ import org.apache.spark.internal.Logging import org.apache.spark.rpc.RpcEnv @@ -77,6 +77,43 @@ class ExecutorClassLoaderSuite } } + test("child over system classloader") { + // JavaFileObject for scala.Option class + val scalaOptionFile = new SimpleJavaFileObject( + URI.create(s"string:///scala/Option.java"), + JavaFileObject.Kind.SOURCE) { + + override def getCharContent(ignoreEncodingErrors: Boolean): CharSequence = { + "package scala; class Option {}" + } + } + // compile fake scala.Option class + ToolProvider + .getSystemJavaCompiler + .getTask(null, null, null, null, null, Collections.singletonList(scalaOptionFile)).call() + + // create 'scala' dir in tempDir1 + val scalaDir = new File(tempDir1, "scala") + assert(scalaDir.mkdir(), s"Failed to create 'scala' directory in $tempDir1") + + // move the generated class into scala dir + val filename = "Option.class" + val result = new File(filename) + assert(result.exists(), "Compiled file not found: " + result.getAbsolutePath) + + val out = new File(scalaDir, filename) + Files.move(result, out) + assert(out.exists(), "Destination file not moved: " + out.getAbsolutePath) + + // construct class loader tree + val parentLoader = new URLClassLoader(urls2, null) + val classLoader = new ExecutorClassLoader(new SparkConf(), null, url1, parentLoader, true) + + // load 'scala.Option' + val optionClass = Class.forName("scala.Option", false, classLoader) + assert(optionClass.getClassLoader == classLoader, "scala.Option didn't come from ExecutorClassLoader") + } + test("child first") { val parentLoader = new URLClassLoader(urls2, null) val classLoader = new ExecutorClassLoader(new SparkConf(), null, url1, parentLoader, true) From 13b016747c6f70584268a6a06c76c2211cf2c9bd Mon Sep 17 00:00:00 2001 From: Kohki Nishio Date: Sun, 26 Feb 2017 10:10:09 -0800 Subject: [PATCH 3/7] Fixed unwanted import change --- .../org/apache/spark/repl/ExecutorClassLoaderSuite.scala | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala index 9110c925da50d..728158035266a 100644 --- a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala +++ b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala @@ -23,11 +23,12 @@ import java.nio.channels.{FileChannel, ReadableByteChannel} import java.nio.charset.StandardCharsets import java.nio.file.{Paths, StandardOpenOption} import java.util -import java.util.{Arrays, Collections} +import java.util.Collections import javax.tools.{JavaFileObject, SimpleJavaFileObject, ToolProvider} import scala.io.Source import scala.language.implicitConversions + import com.google.common.io.Files import org.mockito.Matchers.anyString import org.mockito.Mockito._ @@ -35,6 +36,7 @@ import org.mockito.invocation.InvocationOnMock import org.mockito.stubbing.Answer import org.scalatest.BeforeAndAfterAll import org.scalatest.mock.MockitoSugar + import org.apache.spark._ import org.apache.spark.internal.Logging import org.apache.spark.rpc.RpcEnv From d66ad0cbd30ed4f834d62e559456dd4bb03b747e Mon Sep 17 00:00:00 2001 From: Kohki Nishio Date: Thu, 2 Mar 2017 18:48:50 -0800 Subject: [PATCH 4/7] Add comment for disabling scalastyle --- .../org/apache/spark/repl/ExecutorClassLoaderSuite.scala | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala index 728158035266a..7383d542b6f63 100644 --- a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala +++ b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala @@ -111,8 +111,12 @@ class ExecutorClassLoaderSuite val parentLoader = new URLClassLoader(urls2, null) val classLoader = new ExecutorClassLoader(new SparkConf(), null, url1, parentLoader, true) - // load 'scala.Option' + // load 'scala.Option', using Class.forName to do the exact same behavior as + // what JavaDeserializationStream does + + // scalastyle:off classforname val optionClass = Class.forName("scala.Option", false, classLoader) + // scalastyle:on classforname assert(optionClass.getClassLoader == classLoader, "scala.Option didn't come from ExecutorClassLoader") } From 92c810f67591f12a0ea71aae7d8cc19b54d180d4 Mon Sep 17 00:00:00 2001 From: Kohki Nishio Date: Thu, 2 Mar 2017 19:03:15 -0800 Subject: [PATCH 5/7] Another attempt to resolve scala style issue --- .../org/apache/spark/repl/ExecutorClassLoaderSuite.scala | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala index 7383d542b6f63..64f3bd0654e84 100644 --- a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala +++ b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala @@ -109,15 +109,19 @@ class ExecutorClassLoaderSuite // construct class loader tree val parentLoader = new URLClassLoader(urls2, null) - val classLoader = new ExecutorClassLoader(new SparkConf(), null, url1, parentLoader, true) + val classLoader = new ExecutorClassLoader( + new SparkConf(), null, url1, parentLoader, true) // load 'scala.Option', using Class.forName to do the exact same behavior as // what JavaDeserializationStream does + // scalastyle:off classforname val optionClass = Class.forName("scala.Option", false, classLoader) // scalastyle:on classforname - assert(optionClass.getClassLoader == classLoader, "scala.Option didn't come from ExecutorClassLoader") + + assert(optionClass.getClassLoader == classLoader, + "scala.Option didn't come from ExecutorClassLoader") } test("child first") { From f9204c2b4efd5e11429262772f379738f2ce2cbd Mon Sep 17 00:00:00 2001 From: Kohki Nishio Date: Thu, 2 Mar 2017 19:45:43 -0800 Subject: [PATCH 6/7] ScalaStyle complains about comment --- .../scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala index 64f3bd0654e84..092d3c272b8f6 100644 --- a/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala +++ b/repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala @@ -112,10 +112,9 @@ class ExecutorClassLoaderSuite val classLoader = new ExecutorClassLoader( new SparkConf(), null, url1, parentLoader, true) - // load 'scala.Option', using Class.forName to do the exact same behavior as + // load 'scala.Option', using ClassforName to do the exact same behavior as // what JavaDeserializationStream does - // scalastyle:off classforname val optionClass = Class.forName("scala.Option", false, classLoader) // scalastyle:on classforname From f9f97706f1508bfba8d00b8727af259868c88c2c Mon Sep 17 00:00:00 2001 From: Kohki Nishio Date: Tue, 20 Jun 2017 22:05:50 -0700 Subject: [PATCH 7/7] Add a comment to explain why this class loader does not set parent classloader --- .../main/scala/org/apache/spark/repl/ExecutorClassLoader.scala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala b/repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala index 6b291e376e5e5..8195305234c96 100644 --- a/repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala +++ b/repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala @@ -38,6 +38,8 @@ import org.apache.spark.util.{ParentClassLoader, Utils} * Allows the user to specify if user class path should be first. * This class loader delegates getting/finding resources to parent loader, * which makes sense until REPL never provide resource dynamically. + * This class does not set parent classloader since this class loader + * has higher precedence over its parent class loader. */ class ExecutorClassLoader( conf: SparkConf,