You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by squito <gi...@git.apache.org> on 2017/09/19 18:16:00 UTC

[GitHub] spark pull request #19280: [SPARK-21928][CORE] Set classloader on Serializer...

GitHub user squito opened a pull request:

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

    [SPARK-21928][CORE] Set classloader on SerializerManager private kryo

    ## What changes were proposed in this pull request?
    
    We have to make sure thatthat SerializerManager's private instance of
    kryo also uses the right classloader, regardless of the current thread
    classloader.  In particular, this fixes serde during remote cache
    fetches, as those occur in netty threads.
    
    ## How was this patch tested?
    
    Manual tests & existing suite via jenkins.  I haven't been able to reproduce this is in a unit test, because when a remote RDD partition can be fetched, there is a warning message and then the partition is just recomputed locally.  I manually verified the warning message is no longer present.

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

    $ git pull https://github.com/squito/spark SPARK-21928_ser_classloader

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

    https://github.com/apache/spark/pull/19280.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 #19280
    
----
commit acbaf8b65629344d760360b768e89f1712af8942
Author: Imran Rashid <ir...@cloudera.com>
Date:   2017-09-19T15:19:43Z

    [SPARK-21928][CORE] Set classloader on SerializerManager private kryo
    
    We have to make sure thatthat SerializerManager's private instance of
    kryo also uses the right classloader, regardless of the current thread
    classloader.  In particular, this fixes serde during remote cache
    fetches, as those occur in netty threads.

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by squito <gi...@git.apache.org>.

Github user squito commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    > Looks ok to me, assuming the "default serializer" in SerializerManager is configured correctly through other means.
    
    I think that part is fine.  The serializer is created here:
    https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/SparkEnv.scala#L279
    
    The same instance is assigned to `SparkEnv.serializer`: https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/SparkEnv.scala#L374
    
    Which has its default classloader set in Executor.scala, right by the part I'm changing: https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/executor/Executor.scala#L131


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    LGTM, merging to master / 2.2 / 2.1.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    Didn't merge to 2.1, please open a PR against that branch if you want it there.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    **[Test build #81949 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81949/testReport)** for PR 19280 at commit [`20e3585`](https://github.com/apache/spark/commit/20e3585eac16ef3bfe403ec23f57a2705ff47ecb).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by vanzin <gi...@git.apache.org>.

Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    Looks ok to me, assuming the "default serializer" in SerializerManager is configured correctly through other means.
    
    Title would sound better with a possessive: "SerializerManager's private kryo"


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81949/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by SparkQA <gi...@git.apache.org>.

Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    **[Test build #81944 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/81944/testReport)** for PR 19280 at commit [`acbaf8b`](https://github.com/apache/spark/commit/acbaf8b65629344d760360b768e89f1712af8942).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/81944/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org

[GitHub] spark issue #19280: [SPARK-21928][CORE] Set classloader on SerializerManager...

Posted by AmplabJenkins <gi...@git.apache.org>.

Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19280
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org