You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by manku-timma <gi...@git.apache.org> on 2014/04/04 04:36:24 UTC

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

GitHub user manku-timma opened a pull request:

    https://github.com/apache/spark/pull/322

    [SPARK-1403] Move the class loader creation back to where it was in 0.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 https://github.com/apache/spark/commit/7edbea41b43e0dc11a2de156be220db8b7952d01 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.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/manku-timma/spark-1 spark-1403

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/322.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #322
    
----
commit 89109d7bf2ce35e58a252bd7144438fc720ee362
Author: Bharath Bhushan <ma...@outlook.com>
Date:   2014-04-04T02:26:26Z

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

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39689262
  
    @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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39543185
  
    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`.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-40284773
  
     Merged build triggered. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39533342
  
    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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-40285724
  
    Merged build finished. All automated tests passed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39546729
  
    Put the following line at the beginning of `registered` method (at [line 53](https://github.com/manku-timma/spark-1/blob/89109d7bf2ce35e58a252bd7144438fc720ee362/core/src/main/scala/org/apache/spark/executor/MesosExecutorBackend.scala#L53))
    
    ```
    Thread.currentThread.setContextClassLoader(getClass.getClassLoader)
    ```



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39627690
  
    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
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39676303
  
    @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)
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39699481
  
    Okay I think the broader issue is that 
    
    @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.
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-40284781
  
    Merged build started. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39541428
  
    I can confirm that the third line is needed. Without that line I see the same failure as earlier.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-40285726
  
    All automated tests passed.
    Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/14078/


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39676625
  
    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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39533417
  
    @ueshin removed it in SPARK-1210:
    https://github.com/apache/spark/pull/15
    
    I proposed a more conservative change in this comment:
    https://github.com/apache/spark/pull/15#issuecomment-38758426
    
    But we ultimately went ahead and just removed it because we couldn't see a case where it was necessary...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-40284736
  
    Jenkins, retest this please.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-40298968
  
    Thanks for the fix - merged into master and 1.0


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39530349
  
    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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39551604
  
    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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39655348
  
    I see that PR 334 made the java 6 change. So I reverted mine. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/322


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39543590
  
    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"


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39543328
  
    @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...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-40125951
  
    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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39544602
  
    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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39641219
  
    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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39676705
  
    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?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39560562
  
    @manku-timma, I just wondered that we should restore the class loader in `registered` method like:
    
    ```scala
    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](https://github.com/manku-timma/spark-1/blob/89109d7bf2ce35e58a252bd7144438fc720ee362/core/src/main/scala/org/apache/spark/executor/Executor.scala#L180).
    
    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)



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39700076
  
    @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.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39699792
  
    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).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39643866
  
    LGTM about the class loader issue.
    But I'm not sure the last fix i.e. Java7 API may be used or not.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/322#discussion_r11318644
  
    --- Diff: core/src/main/scala/org/apache/spark/executor/MesosExecutorBackend.scala ---
    @@ -50,13 +50,18 @@ private[spark] class MesosExecutorBackend
           executorInfo: ExecutorInfo,
           frameworkInfo: FrameworkInfo,
           slaveInfo: SlaveInfo) {
    -    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)
    +    val cl = Thread.currentThread.getContextClassLoader
    +    try {
    --- End diff --
    
    You need one more line here
    
    ```scala
    Thread.currentThread.setContextClassLoader(getClass.getClassLoader)
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39674063
  
    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)
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39552826
  
    @ueshin, your one line change works for me.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by ueshin <gi...@git.apache.org>.
Github user ueshin commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39550942
  
    @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`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39720333
  
    So the current fix looks fine?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39976847
  
    @pwendell: Do you plan to pick this up for 1.0? Is there anything more I need to do?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39526912
  
    Can one of the admins verify this patch?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by manku-timma <gi...@git.apache.org>.
Github user manku-timma commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39535193
  
    I am testing out if removing the last line is ok.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] spark pull request: [SPARK-1403] Move the class loader creation ba...

Posted by pwendell <gi...@git.apache.org>.
Github user pwendell commented on the pull request:

    https://github.com/apache/spark/pull/322#issuecomment-39543041
  
    Which exact failure is it? Could you post the stack trace?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---