[SPARK-27122][core] Jetty classes must not be return via getters in org.apache.spark.ui.WebUI by ajithme · Pull Request #24088 · apache/spark

ajithme · 2019-03-14T08:34:25Z

What changes were proposed in this pull request?

When we run YarnSchedulerBackendSuite, the class path seems to be made from the classes folder(resource-managers/yarn/target/scala-2.12/classes) instead of jar (resource-managers/yarn/target/spark-yarn_2.12-3.0.0-SNAPSHOT.jar) . ui.getHandlers is in spark-core and its loaded from spark-core.jar which is shaded and hence refers to org.spark_project.jetty.servlet.ServletContextHandler

Here in org.apache.spark.scheduler.cluster.YarnSchedulerBackend, as its not shaded, it expects org.eclipse.jetty.servlet.ServletContextHandler
Refer discussion @ https://issues.apache.org/jira/browse/SPARK-27122?focusedCommentId=16792318&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16792318

Hence as a fix, org.apache.spark.ui.WebUI must only return a wrapper class instance or references so that Jetty classes can be avoided in getters which are accessed outside spark-core

How was this patch tested?

Existing UT can pass

ajithme · 2019-03-14T08:35:19Z

ping @srowen @dongjoon-hyun @gengliangwang @cloud-fan

cloud-fan · 2019-03-14T11:48:04Z

ok to test

cloud-fan · 2019-03-14T11:49:09Z

The title makes me think that this is a patch for build files(sbt or maven), but it is not. Can you explain how do you achieve it?

ajithme · 2019-03-14T13:15:34Z

@cloud-fan i have updated the PR description and PR title. Does it seem right now.?

SparkQA · 2019-03-14T18:46:44Z

Test build #103495 has finished for PR 24088 at commit 49921ee.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

ajithme · 2019-03-15T01:11:10Z

Updated as per review comments. Please check

vanzin · 2019-03-15T02:34:20Z

This is not the right style. See the class's constructor for an example.

@vanzin I have run scalafmt on modified class and updated the PR. Is it ok now.?

SparkQA · 2019-03-15T06:14:17Z

Test build #103525 has finished for PR 24088 at commit d7e8404.

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

srowen

I'm OK with this approach, as opposed to adjusting the classpath. @ajithme although I only encountered this when running vs Java 11, it doesn't seem specific to Java 11. Maybe it was really just triggered by running the test in isolation. Anyway, seems OK to fix.

vanzin · 2019-03-15T15:40:03Z

Looks ok pending tests which don't seem to have re-triggered...

vanzin · 2019-03-15T15:40:09Z

retest this please

SparkQA · 2019-03-15T22:00:33Z

Test build #103535 has finished for PR 24088 at commit 3f4b831.

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

SparkQA · 2019-03-16T19:34:14Z

Test build #4629 has started for PR 24088 at commit 3f4b831.

SparkQA · 2019-03-17T05:51:14Z

Test build #4633 has finished for PR 24088 at commit 3f4b831.

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

srowen · 2019-03-17T11:44:27Z

Merged to master

dongjoon-hyun · 2019-09-12T19:13:25Z

Hi, All.
Can we have this in branch-2.4 because it's LTS branch, too?
Originally, this is reported on JDK9+, but this is reproduced in JDK8 (as reported on JIRA), too.

dongjoon-hyun · 2019-09-12T19:15:38Z

cc @dbtsai

srowen · 2019-09-12T19:30:47Z

This is a pretty internal change right? I think it could be fine for 2.4.

dongjoon-hyun · 2019-09-12T19:38:47Z

Thank you, @srowen .
Could you make a backport PR please, @ajithme?

dongjoon-hyun · 2019-09-15T05:16:10Z

I made a backport PR to branch-2.4.

… in org.apache.spark.ui.WebUI ### What changes were proposed in this pull request? This is a backport of #24088 to avoid CCE. ### Why are the changes needed? When we run YarnSchedulerBackendSuite, the class path seems to be made from the classes folder(`resource-managers/yarn/target/scala-2.12/classes`) instead of jar (`resource-managers/yarn/target/spark-yarn_2.12-*-SNAPSHOT.jar`) . `ui.getHandlers` is in spark-core and its loaded from spark-core.jar which is shaded and hence refers to `org.spark_project.jetty.servlet.ServletContextHandler` Here in org.apache.spark.scheduler.cluster.YarnSchedulerBackend, as its not shaded, it expects org.eclipse.jetty.servlet.ServletContextHandler Refer discussion https://issues.apache.org/jira/browse/SPARK-27122?focusedCommentId=16792318&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16792318 Hence as a fix, org.apache.spark.ui.WebUI must only return a wrapper class instance or references so that Jetty classes can be avoided in getters which are accessed outside spark-core ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Pass the Jenkins with the existing tests. Closes#25793 from dongjoon-hyun/SPARK-27122. Authored-by: Ajith <ajith2489@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

… avoid CCE ### What changes were proposed in this pull request? [SPARK-27122](#24088) fixes `ClassCastException` at `yarn` module by introducing `DelegatingServletContextHandler`. Initially, this was discovered with JDK9+, but the class path issues affected JDK8 environment, too. After [SPARK-28709](#25439), I also hit the similar issue at `streaming` module. This PR aims to fix `streaming` module by adding `getContextPath` to `DelegatingServletContextHandler` and using it. ### Why are the changes needed? Currently, when we test `streaming` module independently, it fails like the following. ``` $ build/mvn test -pl streaming ... UISeleniumSuite: - attaching and detaching a Streaming tab *** FAILED *** java.lang.ClassCastException: org.sparkproject.jetty.servlet.ServletContextHandler cannot be cast to org.eclipse.jetty.servlet.ServletContextHandler ... Tests: succeeded 337, failed 1, canceled 0, ignored 1, pending 0 *** 1 TEST FAILED *** [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ ``` ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Pass the Jenkins with the modified tests. And do the following manually. Since you can observe this when you run `streaming` module test only (instead of running all), you need to install the changed `core` module and use it. ``` $ java -version openjdk version "1.8.0_222" OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_222-b10) OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.222-b10, mixed mode) $ build/mvn install -DskipTests $ build/mvn test -pl streaming ``` Closes#25791 from dongjoon-hyun/SPARK-29087. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

… avoid CCE ### What changes were proposed in this pull request? [SPARK-27122](#24088) fixes `ClassCastException` at `yarn` module by introducing `DelegatingServletContextHandler`. Initially, this was discovered with JDK9+, but the class path issues affected JDK8 environment, too. After [SPARK-28709](#25439), I also hit the similar issue at `streaming` module. This PR aims to fix `streaming` module by adding `getContextPath` to `DelegatingServletContextHandler` and using it. ### Why are the changes needed? Currently, when we test `streaming` module independently, it fails like the following. ``` $ build/mvn test -pl streaming ... UISeleniumSuite: - attaching and detaching a Streaming tab *** FAILED *** java.lang.ClassCastException: org.sparkproject.jetty.servlet.ServletContextHandler cannot be cast to org.eclipse.jetty.servlet.ServletContextHandler ... Tests: succeeded 337, failed 1, canceled 0, ignored 1, pending 0 *** 1 TEST FAILED *** [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ ``` ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Pass the Jenkins with the modified tests. And do the following manually. Since you can observe this when you run `streaming` module test only (instead of running all), you need to install the changed `core` module and use it. ``` $ java -version openjdk version "1.8.0_222" OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_222-b10) OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.222-b10, mixed mode) $ build/mvn install -DskipTests $ build/mvn test -pl streaming ``` Closes#25791 from dongjoon-hyun/SPARK-29087. Authored-by: Dongjoon Hyun <dhyun@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 729b318) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

ajithme changed the title ~~[SPARK-27122] Avoid exposing shaded jetty classes outside spark-core~~[SPARK-27122] Jetty classes must not be return via getters in org.apache.spark.ui.WebUIMar 14, 2019

vanzin changed the title ~~[SPARK-27122] Jetty classes must not be return via getters in org.apache.spark.ui.WebUI~~[SPARK-27122][core] Jetty classes must not be return via getters in org.apache.spark.ui.WebUIMar 14, 2019

vanzin reviewed Mar 14, 2019
View reviewed changes

vanzin reviewed Mar 15, 2019
View reviewed changes

ajithme added 3 commits March 15, 2019 14:26

avoid exposi shaded classes outside spark-core
631e991

review comments
20e17b2

update as per style
aab4a9a

ajithme force-pushed the shadebug branch from d7e8404 to aab4a9aCompare March 15, 2019 09:22

srowen reviewed Mar 15, 2019
View reviewed changes

Comment threadcore/src/main/scala/org/apache/spark/ui/WebUI.scala Outdated
Comment threadcore/src/main/scala/org/apache/spark/ui/WebUI.scala Outdated

update review comments
3f4b831

srowen reviewed Mar 15, 2019
View reviewed changes

srowen closed this in c324e1dMar 17, 2019

dongjoon-hyun mentioned this pull request Sep 15, 2019
[SPARK-29087][CORE][STREAMING] Use DelegatingServletContextHandler to avoid CCE #25791
Closed

dongjoon-hyun mentioned this pull request Sep 15, 2019
[SPARK-27122][CORE][2.4] Jetty classes must not be return via getters in org.apache.spark.ui.WebUI #25793
Closed

Uh oh!

Conversation

ajithme commented Mar 14, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

ajithme commented Mar 14, 2019

Uh oh!

cloud-fan commented Mar 14, 2019

Uh oh!

cloud-fan commented Mar 14, 2019

Uh oh!

ajithme commented Mar 14, 2019

Uh oh!

SparkQA commented Mar 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajithme commented Mar 15, 2019

Uh oh!

vanzinMar 15, 2019

Choose a reason for hiding this comment

Uh oh!

ajithmeMar 15, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 15, 2019

Uh oh!

Uh oh!

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

vanzin commented Mar 15, 2019

Uh oh!

vanzin commented Mar 15, 2019

Uh oh!

SparkQA commented Mar 15, 2019

Uh oh!

SparkQA commented Mar 16, 2019

Uh oh!

SparkQA commented Mar 17, 2019

Uh oh!

srowen commented Mar 17, 2019

Uh oh!

dongjoon-hyun commented Sep 12, 2019

Uh oh!

dongjoon-hyun commented Sep 12, 2019

Uh oh!

srowen commented Sep 12, 2019

Uh oh!

dongjoon-hyun commented Sep 12, 2019

Uh oh!

dongjoon-hyun commented Sep 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ajithme commented Mar 14, 2019•
edited
Loading