[SPARK-18646][REPL] Set parent classloader as null for ExecutorClassLoader #17074

taroplus · 2017-02-26T16:41:38Z

What changes were proposed in this pull request?

By default, a class loader will have java's system class loader as its parent, however this ExecutorClassLoader should always resolve classes from given 'paren't class loader.

Problem happens when an application has a class loader structure like below

System-Classloader [ClassA, Runtime]
|
Application-Classloader [ClassA, Spark, Jackson etc etc] : parent
|
Executor Class Loader

The application serializes ClassA using Application class loader however Spark deserializes the class using System Class loader, this leads to various problems.

The change here to pass null to the parent class so that a link between ExecutorClassLoader and SystemClassLoader no longer exists.

For most of situations, the parent class loader is the system class loader so this wouldn't have any impact.

How was this patch tested?

UTs

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]

taroplus · 2017-02-26T16:48:24Z

@holdenk @zsxwing

holdenk · 2017-02-26T17:09:42Z

Thanks for working on this. It's been awhile since I looked at the classloader bits so I'd need to refresh my memory a bit, but regardless this PR probably needs better tests - specifically including something which wasn't working before and does work with the change.

taroplus · 2017-02-26T18:19:13Z

@holdenk
I believe this was introduced in this change
fa0524f#diff-bb538fda94224dd0af01d0fd7e1b4ea0R39

the change is to stop passing 'parent' to the parent class(ClassLoader), however this introduced a link between ExecutorClassLoader to SystemClassLoader.

Most of the time, SystemClassLoader is parent loader itself. Then it causes issues with userClassPathFirst (SPARK-18646) and if it is not parent loader, it causes issues like InvalidClassException(SPARK-19675)

I added a UT for this

vanzin · 2017-03-03T00:44:57Z

ok to test

SparkQA · 2017-03-03T00:49:21Z

Test build #73796 has finished for PR 17074 at commit 13b0167.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-03T02:54:35Z

Test build #73799 has finished for PR 17074 at commit d66ad0c.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-03T03:09:30Z

Test build #73801 has finished for PR 17074 at commit 92c810f.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-03-03T03:47:30Z

Test build #73805 has started for PR 17074 at commit f9204c2.

taroplus · 2017-03-04T01:35:09Z

@sknapp
Why my build was canceled ?

jiangxb1987 · 2017-06-19T15:05:05Z

retest this please

SparkQA · 2017-06-19T15:20:02Z

Test build #78258 has finished for PR 17074 at commit f9204c2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987

This PR looks good overall, only have some minor comments.

jiangxb1987 · 2017-06-20T14:02:52Z

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 {


Maybe add comment over this to explain why we are setting a null value to parent?

added to the description of this class

jiangxb1987 · 2017-06-20T14:09:51Z

repl/src/test/scala/org/apache/spark/repl/ExecutorClassLoaderSuite.scala

+    assert(scalaDir.mkdir(), s"Failed to create 'scala' directory in $tempDir1")
+
+    // move the generated class into scala dir
+    val filename = "Option.class"


Why not use createCompiledClass() here?

it's because createCompiledClass doesn't handle package name, it needs to put the generated class under 'scala' package and also it needs to move the generated class to 'scala' folder.

Since createCompiledClass is widely used, I didn't want to touch it just for this very unique situation.

jiangxb1987 · 2017-06-20T14:10:39Z

Also cc @cloud-fan

jiangxb1987 · 2017-06-21T03:58:44Z

In the jira, you mentioned ExecutorClassLoader.getParent() will return the unexpected class loader, could you explain where we call this? At least I didn't see them in Spark codebase(neither current master branch nor branch 1.6)

…assloader

SparkQA · 2017-06-21T05:20:38Z

Test build #78368 has finished for PR 17074 at commit f9f9770.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

taroplus · 2017-06-21T05:24:50Z

SPARK-18646 is not the one I created, SPARK-19675 is the one I filed. But it kept getting closed again and again, so I linked my PR to SPARK-18646 which is still open.

But I think I know what it means. The author of that jira suggested that the fix would be to pass parent class loader to the constructor. If we call ExecutorClassLoader.getParent via debugger or something, it returns SystemClassLoader which is wrong and the person says that it should be the 'parent' class loader.

Original change (fa0524f), which introduced this issue, was not to pass parent to the constructor so that this class loader takes precedence over parent class loader, however it looks to me that the change forgot to set parent as null then if system class loader has something different class from the current or parent class loader, problems like this can happen.

I'm not sure if I explained well, .. let me know if you need more info

jiangxb1987

This approach looks good to merge, and it would be even better if a concise example of how this is causing a failure in production environment could be provided.

jiangxb1987 · 2017-06-21T13:54:34Z

repl/src/main/scala/org/apache/spark/repl/ExecutorClassLoader.scala

@@ -38,13 +38,15 @@ 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


How about rephrase to:

[[ClassLoader]] will preferentially load class from parent. Only when parent is null or the load failed, that it will call the overridden `findClass` function. To avoid the potential issue caused by loading class using inappropriate class loader, we should set the parent of ClassLoader to null, so that we can fully control which class loader is used.

Also add a empty line before this statement.

jiangxb1987 · 2017-06-26T15:45:11Z

ping @taroplus

…oader ## What changes were proposed in this pull request? `ClassLoader` will preferentially load class from `parent`. Only when `parent` is null or the load failed, that it will call the overridden `findClass` function. To avoid the potential issue caused by loading class using inappropriate class loader, we should set the `parent` of `ClassLoader` to null, so that we can fully control which class loader is used. This is take over of #17074, the primary author of this PR is taroplus . Should close #17074 after this PR get merged. ## How was this patch tested? Add test case in `ExecutorClassLoaderSuite`. Author: Kohki Nishio <taroplus@me.com> Author: Xingbo Jiang <xingbo.jiang@databricks.com> Closes #18614 from jiangxb1987/executor_classloader. (cherry picked from commit e08d06b) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…oader ## What changes were proposed in this pull request? `ClassLoader` will preferentially load class from `parent`. Only when `parent` is null or the load failed, that it will call the overridden `findClass` function. To avoid the potential issue caused by loading class using inappropriate class loader, we should set the `parent` of `ClassLoader` to null, so that we can fully control which class loader is used. This is take over of apache#17074, the primary author of this PR is taroplus . Should close apache#17074 after this PR get merged. ## How was this patch tested? Add test case in `ExecutorClassLoaderSuite`. Author: Kohki Nishio <taroplus@me.com> Author: Xingbo Jiang <xingbo.jiang@databricks.com> Closes apache#18614 from jiangxb1987/executor_classloader. (cherry picked from commit e08d06b) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…oader ## What changes were proposed in this pull request? `ClassLoader` will preferentially load class from `parent`. Only when `parent` is null or the load failed, that it will call the overridden `findClass` function. To avoid the potential issue caused by loading class using inappropriate class loader, we should set the `parent` of `ClassLoader` to null, so that we can fully control which class loader is used. This is take over of apache#17074, the primary author of this PR is taroplus . Should close apache#17074 after this PR get merged. ## How was this patch tested? Add test case in `ExecutorClassLoaderSuite`. Author: Kohki Nishio <taroplus@me.com> Author: Xingbo Jiang <xingbo.jiang@databricks.com> Closes apache#18614 from jiangxb1987/executor_classloader.

Set parent classloader as null for ExecutorClassLoader

e0d9538

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]

taroplus added 2 commits February 26, 2017 10:05

Add a UT for ExecutorClassLoader

0a490bb

Fixed unwanted import change

13b0167

Add comment for disabling scalastyle

d66ad0c

Another attempt to resolve scala style issue

92c810f

ScalaStyle complains about comment

f9204c2

jiangxb1987 reviewed Jun 20, 2017

View reviewed changes

Add a comment to explain why this class loader does not set parent cl…

f9f9770

…assloader

jiangxb1987 approved these changes Jun 21, 2017

View reviewed changes

jiangxb1987 mentioned this pull request Jul 12, 2017

[SPARK-18646][REPL] Set parent classloader as null for ExecutorClassLoader #18614

Closed

asfgit closed this in e08d06b Jul 13, 2017

taroplus deleted the executor_classloader branch September 23, 2021 04:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-18646][REPL] Set parent classloader as null for ExecutorClassLoader #17074

[SPARK-18646][REPL] Set parent classloader as null for ExecutorClassLoader #17074

taroplus commented Feb 26, 2017 •

edited

Loading

taroplus commented Feb 26, 2017

holdenk commented Feb 26, 2017

taroplus commented Feb 26, 2017

vanzin commented Mar 3, 2017

SparkQA commented Mar 3, 2017

SparkQA commented Mar 3, 2017

SparkQA commented Mar 3, 2017

SparkQA commented Mar 3, 2017

taroplus commented Mar 4, 2017

jiangxb1987 commented Jun 19, 2017

SparkQA commented Jun 19, 2017

jiangxb1987 left a comment

jiangxb1987 Jun 20, 2017

taroplus Jun 21, 2017

jiangxb1987 Jun 20, 2017

taroplus Jun 21, 2017

jiangxb1987 commented Jun 20, 2017

jiangxb1987 commented Jun 21, 2017 •

edited

Loading

SparkQA commented Jun 21, 2017

taroplus commented Jun 21, 2017 •

edited

Loading

jiangxb1987 left a comment

jiangxb1987 Jun 21, 2017

jiangxb1987 Jun 21, 2017

jiangxb1987 commented Jun 26, 2017

[SPARK-18646][REPL] Set parent classloader as null for ExecutorClassLoader #17074

[SPARK-18646][REPL] Set parent classloader as null for ExecutorClassLoader #17074

Conversation

taroplus commented Feb 26, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

taroplus commented Feb 26, 2017

holdenk commented Feb 26, 2017

taroplus commented Feb 26, 2017

vanzin commented Mar 3, 2017

SparkQA commented Mar 3, 2017

SparkQA commented Mar 3, 2017

SparkQA commented Mar 3, 2017

SparkQA commented Mar 3, 2017

taroplus commented Mar 4, 2017

jiangxb1987 commented Jun 19, 2017

SparkQA commented Jun 19, 2017

jiangxb1987 left a comment

Choose a reason for hiding this comment

jiangxb1987 Jun 20, 2017

Choose a reason for hiding this comment

taroplus Jun 21, 2017

Choose a reason for hiding this comment

jiangxb1987 Jun 20, 2017

Choose a reason for hiding this comment

taroplus Jun 21, 2017

Choose a reason for hiding this comment

jiangxb1987 commented Jun 20, 2017

jiangxb1987 commented Jun 21, 2017 • edited Loading

SparkQA commented Jun 21, 2017

taroplus commented Jun 21, 2017 • edited Loading

jiangxb1987 left a comment

Choose a reason for hiding this comment

jiangxb1987 Jun 21, 2017

Choose a reason for hiding this comment

jiangxb1987 Jun 21, 2017

Choose a reason for hiding this comment

jiangxb1987 commented Jun 26, 2017

taroplus commented Feb 26, 2017 •

edited

Loading

jiangxb1987 commented Jun 21, 2017 •

edited

Loading

taroplus commented Jun 21, 2017 •

edited

Loading