[SPARK-1403] Move the class loader creation back to where it was in 0.9.0 #322

ghost · 2014-04-04T02:36:22Z

[SPARK-1403] I investigated why spark 0.9.0 loads fine on mesos while spark 1.0.0 fails. What I found was that in SparkEnv.scala, while creating the SparkEnv object, the current thread's classloader is null. But in 0.9.0, at the same place, it is set to org.apache.spark.repl.ExecutorClassLoader . I saw that 7edbea4 moved it to it current place. I moved it back and saw that 1.0.0 started working fine on mesos.

I just created a minimal patch that allows me to run spark on mesos correctly. It seems like SecurityManager's creation needs to be taken into account for a correct fix. Also moving the creation of the serializer out of SparkEnv might be a part of the right solution. PTAL.

AmplabJenkins · 2014-04-04T02:37:23Z

Can one of the admins verify this patch?

ghost · 2014-04-04T04:13:26Z

After looking at the code a bit more, I see that the code to setContextClassLoader does not use SecurityManager AFAIS. createClassLoader is creating a File object. addReplClassLoaderIfNeeded is dynamically loading a class file. I dont see any use of SecurityManager in these two methods. I am not sure if it gets used implicitly.

pwendell · 2014-04-04T05:37:52Z

This patch also adds back a line that we earlier removed:

 Thread.currentThread.setContextClassLoader(replClassLoader)

Does your fix require that line to work? If so, what is the error if you remove it?

pwendell · 2014-04-04T05:40:16Z

@ueshin removed it in SPARK-1210:
#15

I proposed a more conservative change in this comment:
#15 (comment)

But we ultimately went ahead and just removed it because we couldn't see a case where it was necessary...

ghost · 2014-04-04T06:24:14Z

I am testing out if removing the last line is ok.

ghost · 2014-04-04T08:15:56Z

I can confirm that the third line is needed. Without that line I see the same failure as earlier.

pwendell · 2014-04-04T08:39:41Z

Which exact failure is it? Could you post the stack trace?

ueshin · 2014-04-04T08:41:42Z

Hi,
I think MesosExecutorBackend should have own ContextClassLoader and it should be set to the class loader of MesosExecutorBackend class before it creates Executor object.

I mistakenly thought that MesosExecutorBackend has own context class loader, but it doesn't. While creating SparkEnv in the Executor's constructor, Thread.currentThread.getContextClassLoader returns null because MesosExecutorBackend doesn't have context class loader. Class.forName() method with null for 3rd argument tries to load class from bootstrap class loader, which doesn't know the class org.apache.spark.serializer.JavaSerializer.

pwendell · 2014-04-04T08:43:40Z

@ueshin - ah cool. Do you mind giving a code snippet of what that would look like? Then @manku-timma can see if it fixes the problem. Probably is a better solution...

ghost · 2014-04-04T08:47:36Z

The actual error is below. Line numbers might be a bit off due to some of my code.

java.lang.ClassNotFoundException: org/apache/spark/serializer/JavaSerializer
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:249)
        at org.apache.spark.SparkEnv$.instantiateClass$1(SparkEnv.scala:173)
        at org.apache.spark.SparkEnv$.create(SparkEnv.scala:184)
        at org.apache.spark.executor.Executor.<init>(Executor.scala:115)
        at org.apache.spark.executor.MesosExecutorBackend.registered(MesosExecutorBackend.scala:56)
Exception in thread "Thread-0"

ghost · 2014-04-04T09:01:59Z

From my limited understanding, Executor is used by the mesos backend and the standalone backend. If mesos backend has its own class loader, will that be enough? Does the standalone backend already have its own class loader?

ueshin · 2014-04-04T09:28:40Z

Put the following line at the beginning of registered method (at line 53)

Thread.currentThread.setContextClassLoader(getClass.getClassLoader)

ueshin · 2014-04-04T10:24:32Z

@manku-timma Yes, the standalone backend org.apache.spark.executor.CoarseGrainedExecutorBackend and the local-mode backend org.apache.spark.scheduler.local.LocalActor have their own context class loader set by ActorSystem.

ueshin · 2014-04-04T10:33:10Z

BTW, we might have to restore the context class loader of MesosExecutorBackend to bootstrap class loader.
We should restore if there are 2 or more threads to handle MesosExecutorBackend, but I don't know how many threads there are.

ghost · 2014-04-04T10:51:29Z

@ueshin, your one line change works for me.

Let me know if you want me to test any of the restore code.

Just for my understanding, the flow after your one-line fix:

MesosExecutorBackend starts off
MesosExecutorBackend sets classloader
MesosExecutorBackend creates Executor
Executor creates SparkEnv
Exectutor sets classloader to the http based one (so that later classes can be downloaded from the master)

ueshin · 2014-04-04T12:43:33Z

@manku-timma, I just wondered that we should restore the class loader in registered method like:

val cl = Thread.currentThread.getContextClassLoader
try {
  Thread.currentThread.setContextClassLoader(getClass.getClassLoader)
  logInfo("Registered with Mesos as executor ID " + executorInfo.getExecutorId.getValue)
  this.driver = driver
  val properties = Utils.deserialize[Array[(String, String)]](executorInfo.getData.toByteArray)
  executor = new Executor(
    executorInfo.getExecutorId.getValue,
    slaveInfo.getHostname,
    properties)
} finally {
  Thread.currentThread.setContextClassLoader(cl)
}

I think this is better than before.

The http based class loader for TaskRunner is correctly set at this point.

The flow you wrote would be:

when registered method is called
- MesosExecutorBackend starts off
- MesosExecutorBackend sets classloader
- MesosExecutorBackend creates Executor
- Executor creates SparkEnv
- MesosExecutorBackend restore classlaoder
when launchTask method is called
- MesosExecutorBackend calls Executor.launchTask method
- Executor creates TaskRunner
- Executor submits the task runner
when the task runner is run
- TaskRunner sets classloader to the http based one (so that later classes can be downloaded from the master)

ueshin · 2014-04-05T00:16:01Z

core/src/main/scala/org/apache/spark/executor/MesosExecutorBackend.scala

You need one more line here

Thread.currentThread.setContextClassLoader(getClass.getClassLoader)

ghost · 2014-04-05T03:26:21Z

Oops. Added that line.

I am facing this error in the current git tree

[error] /home/vagrant/spark2/sql/core/src/main/scala/org/apache/spark/sql/parque
t/ParquetRelation.scala:106: value getGlobal is not a member of object java.util
.logging.Logger
[error]       logger.setParent(Logger.getGlobal)
[error]                               ^
[error] one error found

ghost · 2014-04-05T15:21:07Z

Let me know if there is any other change I need to make. I have tested after merging from master and things look fine. This is good to be merged from my end.

ueshin · 2014-04-05T16:52:38Z

LGTM about the class loader issue.
But I'm not sure the last fix i.e. Java7 API may be used or not.

ghost · 2014-04-06T00:40:38Z

I see that PR 334 made the java 6 change. So I reverted mine.

pwendell · 2014-04-06T17:23:21Z

Hey @ueshin @manku-timma - as a simpler fix, would it be possible to just change the way SparkEnv captures the class loader? I think it was probably just an oversight when that was added. If there is not a context class loader, then it should just load the bootstrap class loader:

val classLoader = Option(Thread.currentThread.getContextClassLoader).getOrElse(getClass.getClassLoader)

ghost · 2014-04-06T18:39:01Z

@pwendell: I tested your fix to SparkEnv.scala (after reverting my earlier change). It does not work. SparkEnv's loader turns out to be sun.misc.Launcher$AppClassLoader@12360be0 and it fails with the following error.

java.lang.ClassNotFoundException: org/apache/spark/io/LZFCompressionCodec
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:249)
        at org.apache.spark.io.CompressionCodec$.createCodec(CompressionCodec.scala:45)
        at org.apache.spark.io.CompressionCodec$.createCodec(CompressionCodec.scala:41)
        at org.apache.spark.broadcast.HttpBroadcast$.initialize(HttpBroadcast.scala:110)
        at org.apache.spark.broadcast.HttpBroadcastFactory.initialize(HttpBroadcast.scala:71)
        at org.apache.spark.broadcast.BroadcastManager.initialize(Broadcast.scala:82)
        at org.apache.spark.broadcast.BroadcastManager.<init>(Broadcast.scala:69)
        at org.apache.spark.SparkEnv$.create(SparkEnv.scala:195)
        at org.apache.spark.executor.Executor.<init>(Executor.scala:105)
        at org.apache.spark.executor.MesosExecutorBackend.registered(MesosExecutorBackend.scala:56)

My patch to SparkEnv.scala:

@@ -142,7 +142,10 @@ object SparkEnv extends Logging {
       conf.set("spark.driver.port",  boundPort.toString)
     }

-    val classLoader = Thread.currentThread.getContextClassLoader
+    val classLoader = Option(Thread.currentThread.getContextClassLoader)
+      .getOrElse(getClass.getClassLoader)

pwendell · 2014-04-06T18:50:12Z

I guess what I don't understand about the current fix is, where is the context class loader getting changed from the boostrap class loader in the first place? The current approach is to capture the class loader of MesosExecutorDriver and set it to the context class loader and then set it back. Isn't this also just the sun.misc.Launcher class loader?

pwendell · 2014-04-06T18:52:51Z

Another way of putting my question. Currently there is a line:

Thread.currentThread.setContextClassLoader(getClass.getClassLoader)

What is the actual classloader being set here... isn't it just the sun.misc.Launcher one?

ghost · 2014-04-07T01:08:48Z

@pwendell: You are right. Actually sun.misc.Launcher$AppClassLoader@12360be0 is the classloader even in the earlier code.

Looks like classes directly loaded by SparkEnv get to use the right classloader since it is passed as an argument. But classes loaded at the second level (like in BroadcastManager above) still use the null classloader and fail.

pwendell · 2014-04-07T06:26:54Z

@ueshin you said before that "Class.forName() method with null for 3rd argument tries to load class from bootstrap class loader, which doesn't know the class org.apache.spark.serializer.JavaSerializer."

But I think in this case we'd expect the bootstrap classloader to know about JavaSerializer (this should be on the classpath when the executor starts), right? I'm still not sure why it would fail in this case. I don't see why MesosExecutorDriver could be on the java classpath but JavaSerializer isn't.

@manku-timma I looked more and the reason this doesn't work is that it looks like other parts of the code don't directly use the classLoader form the executor. I can look more tomorrow and see how we can best clean this up. The current approach works but it's a bit of a hack. There might be a nicer way to clean this up.

pwendell · 2014-04-07T06:34:30Z

Ah I see, @ueshin you are right. It's the bootstrap class loader and it won't have any spark definitions. I was mixing this up with the system class loader.

./bin/spark-shell
scala> Class.forName("org.apache.spark.serializer.JavaSerializer")
res7: Class[_] = class org.apache.spark.serializer.JavaSerializer

scala> Class.forName("org.apache.spark.serializer.JavaSerializer", true, null)
java.lang.ClassNotFoundException: org/apache/spark/serializer/JavaSerializer
    at java.lang.Class.forName0(Native Method)
    at java.lang.Class.forName(Class.java:270)
    at $iwC$$iwC$$iwC$$iwC.<init>(<console>:11)
    at $iwC$$iwC$$iwC.<init>(<console>:16)
    at $iwC$$iwC.<init>(<console>:18)
    at $iwC.<init>(<console>:20)

We should definitely clean this up. The behavior we want in every case is to either use the context class loader (if present) and if not use the classloader that loads spark classes (e.g. the system classloader).

ueshin · 2014-04-07T06:40:27Z

@pwendell Yes, the bootstrap class loader knows only core Java APIs and the Spark classes (specified by -cp java command argument) are loaded by the system class loader (which would be sun.misc.Launcher$AppClassLoader with Oracle JDK).

ghost · 2014-04-07T11:42:35Z

So the current fix looks fine?

SPARK-1009 Updated MLlib docs to show how to use it in Python In addition added detailed examples for regression, clustering and recommendation algorithms in a separate Scala section. Fixed a few minor issues with existing documentation.

ghost · 2014-04-09T15:21:50Z

@pwendell: Do you plan to pick this up for 1.0? Is there anything more I need to do?

pwendell · 2014-04-10T19:11:46Z

Jenkins, retest this please.

I'm fine to have this is a workaround for 1.0. I'll make a JIRA describing the broader issue and we can fix it for 1.1.

pwendell · 2014-04-12T16:21:40Z

Jenkins, retest this please.

AmplabJenkins · 2014-04-12T16:23:12Z

Merged build triggered.

AmplabJenkins · 2014-04-12T16:23:22Z

Merged build started.

AmplabJenkins · 2014-04-12T17:02:07Z

Merged build finished. All automated tests passed.

AmplabJenkins · 2014-04-12T17:02:07Z

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14078/

pwendell · 2014-04-13T03:54:37Z

Thanks for the fix - merged into master and 1.0

@ueshin

….9.0 [SPARK-1403] I investigated why spark 0.9.0 loads fine on mesos while spark 1.0.0 fails. What I found was that in SparkEnv.scala, while creating the SparkEnv object, the current thread's classloader is null. But in 0.9.0, at the same place, it is set to org.apache.spark.repl.ExecutorClassLoader . I saw that 7edbea4 moved it to it current place. I moved it back and saw that 1.0.0 started working fine on mesos. I just created a minimal patch that allows me to run spark on mesos correctly. It seems like SecurityManager's creation needs to be taken into account for a correct fix. Also moving the creation of the serializer out of SparkEnv might be a part of the right solution. PTAL. Author: Bharath Bhushan <[email protected]> Closes #322 from manku-timma/spark-1403 and squashes the following commits: 606c2b9 [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 ec8f870 [Bharath Bhushan] revert the logger change for java 6 compatibility as PR 334 is doing it 728beca [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 044027d [Bharath Bhushan] fix compile error 6f260a4 [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 b3a053f [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 04b9662 [Bharath Bhushan] add missing line 4803c19 [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 f3c9a14 [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 42d3d6a [Bharath Bhushan] used code fragment from @ueshin to fix the problem in a better way 89109d7 [Bharath Bhushan] move the class loader creation back to where it was in 0.9.0 (cherry picked from commit ca11919) Signed-off-by: Patrick Wendell <[email protected]>

@ueshin

….9.0 [SPARK-1403] I investigated why spark 0.9.0 loads fine on mesos while spark 1.0.0 fails. What I found was that in SparkEnv.scala, while creating the SparkEnv object, the current thread's classloader is null. But in 0.9.0, at the same place, it is set to org.apache.spark.repl.ExecutorClassLoader . I saw that apache@7edbea4 moved it to it current place. I moved it back and saw that 1.0.0 started working fine on mesos. I just created a minimal patch that allows me to run spark on mesos correctly. It seems like SecurityManager's creation needs to be taken into account for a correct fix. Also moving the creation of the serializer out of SparkEnv might be a part of the right solution. PTAL. Author: Bharath Bhushan <[email protected]> Closes apache#322 from manku-timma/spark-1403 and squashes the following commits: 606c2b9 [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 ec8f870 [Bharath Bhushan] revert the logger change for java 6 compatibility as PR 334 is doing it 728beca [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 044027d [Bharath Bhushan] fix compile error 6f260a4 [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 b3a053f [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 04b9662 [Bharath Bhushan] add missing line 4803c19 [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 f3c9a14 [Bharath Bhushan] Merge remote-tracking branch 'upstream/master' into spark-1403 42d3d6a [Bharath Bhushan] used code fragment from @ueshin to fix the problem in a better way 89109d7 [Bharath Bhushan] move the class loader creation back to where it was in 0.9.0

move the class loader creation back to where it was in 0.9.0

89109d7

Bharath Bhushan added 3 commits April 4, 2014 13:56

used code fragment from @ueshin to fix the problem in a better way

42d3d6a

Merge remote-tracking branch 'upstream/master' into spark-1403

f3c9a14

Merge remote-tracking branch 'upstream/master' into spark-1403

4803c19

ueshin reviewed Apr 5, 2014
View reviewed changes

Bharath Bhushan added 2 commits April 5, 2014 01:49

add missing line

04b9662

Merge remote-tracking branch 'upstream/master' into spark-1403

b3a053f

Bharath Bhushan added 3 commits April 5, 2014 03:27

Merge remote-tracking branch 'upstream/master' into spark-1403

6f260a4

fix compile error

044027d

Merge remote-tracking branch 'upstream/master' into spark-1403

728beca

revert the logger change for java 6 compatibility as PR 334 is doing it

ec8f870

Merge remote-tracking branch 'upstream/master' into spark-1403

606c2b9

iven mentioned this pull request Apr 12, 2014

Add role and checkpoint support for Mesos backend #60

Closed

asfgit closed this in ca11919 Apr 13, 2014

[SPARK-1403] Move the class loader creation back to where it was in 0.9.0 #322

[SPARK-1403] Move the class loader creation back to where it was in 0.9.0 #322

Uh oh!

Conversation

ghost commented Apr 4, 2014

Uh oh!

AmplabJenkins commented Apr 4, 2014

Uh oh!

ghost commented Apr 4, 2014

Uh oh!

pwendell commented Apr 4, 2014

Uh oh!

pwendell commented Apr 4, 2014

Uh oh!

ghost commented Apr 4, 2014

Uh oh!

ghost commented Apr 4, 2014

Uh oh!

pwendell commented Apr 4, 2014

Uh oh!

ueshin commented Apr 4, 2014

Uh oh!

pwendell commented Apr 4, 2014

Uh oh!

ghost commented Apr 4, 2014

Uh oh!

ghost commented Apr 4, 2014

Uh oh!

ueshin commented Apr 4, 2014

Uh oh!

ueshin commented Apr 4, 2014

Uh oh!

ueshin commented Apr 4, 2014

Uh oh!

ghost commented Apr 4, 2014

Uh oh!

ueshin commented Apr 4, 2014

Uh oh!

ueshin Apr 5, 2014

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 5, 2014

Uh oh!

ghost commented Apr 5, 2014

Uh oh!

ueshin commented Apr 5, 2014

Uh oh!

ghost commented Apr 6, 2014

Uh oh!

pwendell commented Apr 6, 2014

Uh oh!

ghost commented Apr 6, 2014

Uh oh!

pwendell commented Apr 6, 2014

Uh oh!

pwendell commented Apr 6, 2014

Uh oh!

ghost commented Apr 7, 2014

Uh oh!

pwendell commented Apr 7, 2014

Uh oh!

pwendell commented Apr 7, 2014

Uh oh!

ueshin commented Apr 7, 2014

Uh oh!

ghost commented Apr 7, 2014

Uh oh!

ghost commented Apr 9, 2014

Uh oh!

pwendell commented Apr 10, 2014

Uh oh!

pwendell commented Apr 12, 2014

Uh oh!

AmplabJenkins commented Apr 12, 2014

Uh oh!

AmplabJenkins commented Apr 12, 2014

Uh oh!

AmplabJenkins commented Apr 12, 2014

Uh oh!

AmplabJenkins commented Apr 12, 2014