[SPARK-17714][Core][test-maven][test-hadoop2.6]Avoid using ExecutorClassLoader to load Netty generated classes #16859

zsxwing · 2017-02-08T20:12:20Z

What changes were proposed in this pull request?

Netty's MessageToMessageEncoder uses Javassist to generate a matcher class and the implementation calls Class.forName to check if this class is already generated. If MessageEncoder or MessageDecoder is created in ExecutorClassLoader.findClass, it will cause ClassCircularityError. This is because loading this Netty generated class will call ExecutorClassLoader.findClass to search this class, and ExecutorClassLoader will try to use RPC to load it and cause to load the non-exist matcher class again. JVM will report ClassCircularityError to prevent such infinite recursion.

Why it only happens in Maven builds

It's because Maven and SBT have different class loader tree. The Maven build will set a URLClassLoader as the current context class loader to run the tests and expose this issue. The class loader tree is as following:

bootstrap class loader ------ ... ----- REPL class loader ---- ExecutorClassLoader
|
|
URLClasssLoader

The SBT build uses the bootstrap class loader directly and ReplSuite.test("propagation of local properties") is the first test in ReplSuite, which happens to load io/netty/util/internal/__matchers__/org/apache/spark/network/protocol/MessageMatcher into the bootstrap class loader (Note: in maven build, it's loaded into URLClasssLoader so it cannot be found in ExecutorClassLoader). This issue can be reproduced in SBT as well. Here are the produce steps:

Enable hadoop.caller.context.enabled.
Replace Class.forName with Utils.classForName in object CallerContext.
Ignore ReplSuite.test("propagation of local properties").
Run ReplSuite using SBT.

This PR just creates a singleton MessageEncoder and MessageDecoder and makes sure they are created before switching to ExecutorClassLoader. TransportContext will be created when creating RpcEnv and that happens before creating ExecutorClassLoader.

How was this patch tested?

Jenkins

zsxwing · 2017-02-08T20:13:18Z

core/src/main/scala/org/apache/spark/util/Utils.scala

@@ -2599,14 +2599,10 @@ private[spark] object Utils extends Logging {

 private[util] object CallerContext extends Logging {
  val callerContextSupported: Boolean = {
-    SparkHadoopUtil.get.conf.getBoolean("hadoop.caller.context.enabled", false) && {
+    SparkHadoopUtil.get.conf.getBoolean("hadoop.caller.context.enabled", true) && {


I will change default value to false later. Just to test if this PR fixes the issue on Jenkins.

zsxwing · 2017-02-08T20:14:26Z

...on/network-common/src/main/java/org/apache/spark/network/server/TransportChannelHandler.java

@@ -48,7 +47,7 @@
 * on the channel for at least `requestTimeoutMs`. Note that this is duplex traffic; we will not
 * timeout if the client is continuously sending but getting no responses, for simplicity.
 */
-public class TransportChannelHandler extends SimpleChannelInboundHandler<Message> {
+public class TransportChannelHandler extends ChannelInboundHandlerAdapter {


SimpleChannelInboundHandler also uses Javassist to generate a matcher class. Since SimpleChannelInboundHandler provides little value for us, I just changed to extend ChannelInboundHandlerAdapter directly.

SparkQA · 2017-02-08T20:21:10Z

Test build #72598 has finished for PR 16859 at commit 1c88474.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class TransportChannelHandler extends ChannelInboundHandlerAdapter

dongjoon-hyun · 2017-02-08T20:27:10Z

Hi, @zsxwing .
Currently, SparkPullRequestBuilder is broken and HOTFIX is #16858 .

zsxwing · 2017-02-08T20:45:28Z

retest this please

SparkQA · 2017-02-08T23:31:10Z

Test build #72599 has finished for PR 16859 at commit 1c88474.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class TransportChannelHandler extends ChannelInboundHandlerAdapter

zsxwing · 2017-02-08T23:34:03Z

cc @vanzin

vanzin

Looks ok; you have a typo in the description (calls -> class).

vanzin · 2017-02-09T02:14:49Z

common/network-common/src/main/java/org/apache/spark/network/protocol/MessageDecoder.java

+  public static final MessageDecoder INSTANCE = new MessageDecoder();
+
+  private MessageDecoder() {
+    super();


nit: not really necessary (also in the other class)

vanzin · 2017-02-09T02:29:25Z

btw I think you want [test-maven] to run the builder on maven. (last one ran on sbt.)

SparkQA · 2017-02-09T07:45:22Z

Test build #3570 has finished for PR 16859 at commit 1c88474.

This patch fails from timeout after a configured wait of `250m`.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class TransportChannelHandler extends ChannelInboundHandlerAdapter

dongjoon-hyun · 2017-02-09T18:14:20Z

Retest this please

SparkQA · 2017-02-09T20:31:28Z

Test build #72655 has finished for PR 16859 at commit 1c88474.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class TransportChannelHandler extends ChannelInboundHandlerAdapter

zsxwing · 2017-02-09T21:05:22Z

Test build #3570 has finished for PR 16859 at commit 1c88474.

This is the known OOM issue.

Test build #72655 has finished for PR 16859 at commit 1c88474.

The only failure test is the kafka flaky test.

zsxwing · 2017-02-09T21:06:43Z

I'm going to change the default value back since ReplSuite passed in Test build #72655 has finished for PR 16859 at commit 1c88474..

SparkQA · 2017-02-09T23:57:53Z

Test build #72660 has finished for PR 16859 at commit 20b6fd4.

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

SparkQA · 2017-02-10T03:56:48Z

Test build #72665 has finished for PR 16859 at commit 7fcb317.

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

zsxwing · 2017-02-13T20:02:51Z

Thanks! Merging to master and 2.1.

…lassLoader to load Netty generated classes ## What changes were proposed in this pull request? Netty's `MessageToMessageEncoder` uses [Javassist](https://github.com/netty/netty/blob/91a0bdc17a8298437d6de08a8958d753799bd4a6/common/src/main/java/io/netty/util/internal/JavassistTypeParameterMatcherGenerator.java#L62) to generate a matcher class and the implementation calls `Class.forName` to check if this class is already generated. If `MessageEncoder` or `MessageDecoder` is created in `ExecutorClassLoader.findClass`, it will cause `ClassCircularityError`. This is because loading this Netty generated class will call `ExecutorClassLoader.findClass` to search this class, and `ExecutorClassLoader` will try to use RPC to load it and cause to load the non-exist matcher class again. JVM will report `ClassCircularityError` to prevent such infinite recursion. ##### Why it only happens in Maven builds It's because Maven and SBT have different class loader tree. The Maven build will set a URLClassLoader as the current context class loader to run the tests and expose this issue. The class loader tree is as following: ``` bootstrap class loader ------ ... ----- REPL class loader ---- ExecutorClassLoader | | URLClasssLoader ``` The SBT build uses the bootstrap class loader directly and `ReplSuite.test("propagation of local properties")` is the first test in ReplSuite, which happens to load `io/netty/util/internal/__matchers__/org/apache/spark/network/protocol/MessageMatcher` into the bootstrap class loader (Note: in maven build, it's loaded into URLClasssLoader so it cannot be found in ExecutorClassLoader). This issue can be reproduced in SBT as well. Here are the produce steps: - Enable `hadoop.caller.context.enabled`. - Replace `Class.forName` with `Utils.classForName` in `object CallerContext`. - Ignore `ReplSuite.test("propagation of local properties")`. - Run `ReplSuite` using SBT. This PR just creates a singleton MessageEncoder and MessageDecoder and makes sure they are created before switching to ExecutorClassLoader. TransportContext will be created when creating RpcEnv and that happens before creating ExecutorClassLoader. ## How was this patch tested? Jenkins Author: Shixiong Zhu <shixiong@databricks.com> Closes #16859 from zsxwing/SPARK-17714. (cherry picked from commit 905fdf0) Signed-off-by: Shixiong Zhu <shixiong@databricks.com>

zsxwing · 2017-02-13T20:06:11Z

@vanzin I just merged this one. If you have other comments, I will address them in a separate PR. Thanks!

vanzin · 2017-02-13T20:07:19Z

Nope, don't have anything else.

…lassLoader to load Netty generated classes ## What changes were proposed in this pull request? Netty's `MessageToMessageEncoder` uses [Javassist](https://github.com/netty/netty/blob/91a0bdc17a8298437d6de08a8958d753799bd4a6/common/src/main/java/io/netty/util/internal/JavassistTypeParameterMatcherGenerator.java#L62) to generate a matcher class and the implementation calls `Class.forName` to check if this class is already generated. If `MessageEncoder` or `MessageDecoder` is created in `ExecutorClassLoader.findClass`, it will cause `ClassCircularityError`. This is because loading this Netty generated class will call `ExecutorClassLoader.findClass` to search this class, and `ExecutorClassLoader` will try to use RPC to load it and cause to load the non-exist matcher class again. JVM will report `ClassCircularityError` to prevent such infinite recursion. ##### Why it only happens in Maven builds It's because Maven and SBT have different class loader tree. The Maven build will set a URLClassLoader as the current context class loader to run the tests and expose this issue. The class loader tree is as following: ``` bootstrap class loader ------ ... ----- REPL class loader ---- ExecutorClassLoader | | URLClasssLoader ``` The SBT build uses the bootstrap class loader directly and `ReplSuite.test("propagation of local properties")` is the first test in ReplSuite, which happens to load `io/netty/util/internal/__matchers__/org/apache/spark/network/protocol/MessageMatcher` into the bootstrap class loader (Note: in maven build, it's loaded into URLClasssLoader so it cannot be found in ExecutorClassLoader). This issue can be reproduced in SBT as well. Here are the produce steps: - Enable `hadoop.caller.context.enabled`. - Replace `Class.forName` with `Utils.classForName` in `object CallerContext`. - Ignore `ReplSuite.test("propagation of local properties")`. - Run `ReplSuite` using SBT. This PR just creates a singleton MessageEncoder and MessageDecoder and makes sure they are created before switching to ExecutorClassLoader. TransportContext will be created when creating RpcEnv and that happens before creating ExecutorClassLoader. ## How was this patch tested? Jenkins Author: Shixiong Zhu <shixiong@databricks.com> Closes apache#16859 from zsxwing/SPARK-17714.

Avoid using ExecutorClassLoader to load Netty generated classes

1c88474

zsxwing commented Feb 8, 2017

View reviewed changes

dongjoon-hyun mentioned this pull request Feb 8, 2017

[SPARK-19409][BUILD][test-maven] Fix ParquetAvroCompatibilitySuite failure due to test dependency on avro #16795

Closed

zsxwing changed the title ~~[SPARK-17714][Core][maven]Avoid using ExecutorClassLoader to load Netty generated classes~~ [SPARK-17714][Core][maven][test-hadoop2.6]Avoid using ExecutorClassLoader to load Netty generated classes Feb 8, 2017

vanzin approved these changes Feb 9, 2017

View reviewed changes

zsxwing changed the title ~~[SPARK-17714][Core][maven][test-hadoop2.6]Avoid using ExecutorClassLoader to load Netty generated classes~~ [SPARK-17714][Core][test-maven][test-hadoop2.6]Avoid using ExecutorClassLoader to load Netty generated classes Feb 9, 2017

Restore the default value

7fcb317

asfgit closed this in 905fdf0 Feb 13, 2017

zsxwing deleted the SPARK-17714 branch February 13, 2017 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-17714][Core][test-maven][test-hadoop2.6]Avoid using ExecutorClassLoader to load Netty generated classes #16859

[SPARK-17714][Core][test-maven][test-hadoop2.6]Avoid using ExecutorClassLoader to load Netty generated classes #16859

zsxwing commented Feb 8, 2017 •

edited

Loading

zsxwing Feb 8, 2017

zsxwing Feb 8, 2017

SparkQA commented Feb 8, 2017

dongjoon-hyun commented Feb 8, 2017

zsxwing commented Feb 8, 2017

SparkQA commented Feb 8, 2017

zsxwing commented Feb 8, 2017

vanzin left a comment

vanzin Feb 9, 2017

zsxwing Feb 9, 2017

vanzin commented Feb 9, 2017

SparkQA commented Feb 9, 2017

dongjoon-hyun commented Feb 9, 2017

SparkQA commented Feb 9, 2017

zsxwing commented Feb 9, 2017

zsxwing commented Feb 9, 2017

SparkQA commented Feb 9, 2017

SparkQA commented Feb 10, 2017

zsxwing commented Feb 13, 2017

zsxwing commented Feb 13, 2017

vanzin commented Feb 13, 2017

[SPARK-17714][Core][test-maven][test-hadoop2.6]Avoid using ExecutorClassLoader to load Netty generated classes #16859

[SPARK-17714][Core][test-maven][test-hadoop2.6]Avoid using ExecutorClassLoader to load Netty generated classes #16859

Conversation

zsxwing commented Feb 8, 2017 • edited Loading

What changes were proposed in this pull request?

Why it only happens in Maven builds

How was this patch tested?

zsxwing Feb 8, 2017

Choose a reason for hiding this comment

zsxwing Feb 8, 2017

Choose a reason for hiding this comment

SparkQA commented Feb 8, 2017

dongjoon-hyun commented Feb 8, 2017

zsxwing commented Feb 8, 2017

SparkQA commented Feb 8, 2017

zsxwing commented Feb 8, 2017

vanzin left a comment

Choose a reason for hiding this comment

vanzin Feb 9, 2017

Choose a reason for hiding this comment

zsxwing Feb 9, 2017

Choose a reason for hiding this comment

vanzin commented Feb 9, 2017

SparkQA commented Feb 9, 2017

dongjoon-hyun commented Feb 9, 2017

SparkQA commented Feb 9, 2017

zsxwing commented Feb 9, 2017

zsxwing commented Feb 9, 2017

SparkQA commented Feb 9, 2017

SparkQA commented Feb 10, 2017

zsxwing commented Feb 13, 2017

zsxwing commented Feb 13, 2017

vanzin commented Feb 13, 2017

zsxwing commented Feb 8, 2017 •

edited

Loading