[SPARK-16808][Core] History Server main page does not honor APPLICATION_WEB_PROXY_BASE #15742

vijoshi · 2016-11-02T23:44:55Z

What changes were proposed in this pull request?

Application links generated on the history server UI no longer (regression from 1.6) contain the configured spark.ui.proxyBase in the links. To address this, made the uiRoot available globally to all javascripts for Web UI. Updated the mustache template (historypage-template.html) to include the uiroot for rendering links to the applications.

The existing test was not sufficient to verify the scenario where ajax call is used to populate the application listing template, so added a new selenium test case to cover this scenario.

How was this patch tested?

Existing tests and a new unit test.
No visual changes to the UI.

SparkQA · 2016-11-02T23:50:16Z

Test build #68035 has finished for PR 15742 at commit d00a7ff.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-03T02:05:57Z

Test build #68036 has finished for PR 15742 at commit 1b153bf.

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

SparkQA · 2016-11-03T09:46:50Z

Test build #68052 has finished for PR 15742 at commit 0584339.

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

vijoshi · 2016-11-03T09:57:17Z

@ajbozarth @srowen - for your review if possible.

ajbozarth · 2016-11-03T20:03:37Z

Nice catch, from a quick look this seems good. My issue is one of consistency, in previous changes I've made I've used a setter rather that declaring a var on the scala side. I think using a default val and a setter is safer than checking for undefined. An example can be seen at the top of historyserver.js

vijoshi · 2016-11-04T17:40:50Z

@ajbozarth updated the code. Guess, there is agreement to keep uiRoot in webui.js to make it available to any more templates / javascript that need it. I removed the undefined check for uiRoot in historypage.js as that in a way makes the expectation that uiRoot is available explicit. In case it isn't, the testcases would fail which would be better than defaulting to a value that may not be correct.

ajbozarth · 2016-11-04T18:04:27Z

LGTM, putting the var and setter in webui.js is a good idea

SparkQA · 2016-11-04T19:51:08Z

Test build #68146 has finished for PR 15742 at commit 6071d5b.

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

vanzin

Couple of comments on the test, otherwise LGTM.

vanzin · 2016-11-04T22:56:20Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala

+
+    val proxyBaseBeforeTest = System.getProperty("spark.ui.proxyBase")
+    val uiRoot = "/testwebproxybase"
+    System.setProperty("spark.ui.proxyBase", uiRoot)


Do you need to use system properties for this? Isn't it enough to set it in SparkConf?

If you need to use system properties, you should use ResetSystemProperties since if your test fails, it will leave the modified system properties behind.

I do need to use system properties as that's how this particular property seems to work. The test suite HistoryServerSuite already uses ResetSystemProperties, so I have fixed the tests to not try to store/reset system properties on their own.

vanzin · 2016-11-04T23:00:35Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala

+    implicitlyWait(org.scalatest.time.Span(5, org.scalatest.time.Seconds))
+
+    // once this findAll call returns, we know the ajax load of the table completed
+    findAll (ClassNameQuery("odd"))


nit: no space before ( in method calls , also happens below.

vijoshi · 2016-11-05T11:13:13Z

@vanzin @ajbozarth besides your review comments, I noticed that the test case was getting blocked for 30 secs in the after { } where the history server is stopped. Tracked it to Jetty waiting for 30 secs for graceful killing of open connections. Not needed during testing, so added a change for that.

SparkQA · 2016-11-05T13:05:45Z

Test build #68204 has finished for PR 15742 at commit e314450.

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

vijoshi · 2016-11-05T15:38:17Z

retest this please

SparkQA · 2016-11-05T17:19:57Z

Test build #68211 has finished for PR 15742 at commit e314450.

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

SparkQA · 2016-11-05T17:40:26Z

Test build #68212 has finished for PR 15742 at commit 4a443c5.

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

ajbozarth · 2016-11-05T20:35:47Z

core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala

@@ -143,6 +143,12 @@ class HistoryServer(
    appCache.stop()
  }

+  // For testing - override stop timeout used by jetty
+  private[history] def setStopTimeout(timeout: Long): Unit = {


I think that adding a function just to speed up tests by 30sec is unnecessary additional code in this case

this was 30 secs additional for just the one test i added. if more such tests are added the time would add up - so i felt the option to bypass the wait was useful to have. assuming more of the UI could transition to ajax etc I could move this utility function up into the base WebUI class so it's available more generally for future UI tests - not just history server?

I'd say if we're going to add a test util function like this that we might as well make it more available to tests, I still don't like it much, but we seem to moving in the direction where it may be useful. @vanzin what do you think of this addition?

I think this is a little fishy, because other tests run Jetty and don't seem to suffer from this problem (or at least no one has complained).

Maybe, if you explicitly close the selenium driver in the test, this will go away without the need for this method?

This looks to be due to the ProxyServlet handler, the way it handles requests. I am able to get around the problem by explicitly stopping the servlet handler before the test ends. No need to alter Jetty stop timeout with this change. Updated the pull request.

vijoshi · 2016-11-06T10:47:18Z

retest this please

SparkQA · 2016-11-06T13:10:44Z

Test build #68238 has finished for PR 15742 at commit 4a443c5.

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

SparkQA · 2016-11-08T07:26:47Z

Test build #68318 has finished for PR 15742 at commit a7e380d.

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

vanzin · 2016-11-08T18:02:18Z

core/src/test/scala/org/apache/spark/deploy/history/HistoryServerSuite.scala

+      .map(_.get)
+      .filter(_.startsWith(url)).toList
+
+    contextHandler.stop()


Do you need to do this in a finally block? And do you also need to call server.stop()?

EDIT: nevermind the second part, the after block already does that.

Okay, moved it to a finally block. This should make both good and bad testcase runs consistent in their cleanup.

ajbozarth · 2016-11-08T18:24:20Z

LGTM

vanzin · 2016-11-08T18:45:59Z

LGTM pending tests.

SparkQA · 2016-11-08T22:52:41Z

Test build #68354 has finished for PR 15742 at commit fcc779d.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2016-11-08T22:53:31Z

retest this please

SparkQA · 2016-11-09T02:09:10Z

Test build #68366 has finished for PR 15742 at commit fcc779d.

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

vanzin · 2016-11-09T18:39:53Z

Merging to master / 2.1.

…ON_WEB_PROXY_BASE ## What changes were proposed in this pull request? Application links generated on the history server UI no longer (regression from 1.6) contain the configured spark.ui.proxyBase in the links. To address this, made the uiRoot available globally to all javascripts for Web UI. Updated the mustache template (historypage-template.html) to include the uiroot for rendering links to the applications. The existing test was not sufficient to verify the scenario where ajax call is used to populate the application listing template, so added a new selenium test case to cover this scenario. ## How was this patch tested? Existing tests and a new unit test. No visual changes to the UI. Author: Vinayak <vijoshi5@in.ibm.com> Closes #15742 from vijoshi/SPARK-16808_master. (cherry picked from commit 06a13ec) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

mariobriggs · 2016-11-11T01:43:57Z

@vanzin since this a regression bux in 2.0, any particular reason it is merged only to 2.1 . I believe this should be in 2.0.2/3 as well

vanzin · 2016-11-11T01:46:37Z

It doesn't merge cleanly into branch-2.0. You can file a separate PR if you want it fixed there.

mariobriggs · 2016-11-11T03:07:18Z

ok. will look into that

vijoshi · 2016-11-11T22:36:27Z

@vanzin @mariobriggs Opened #15855 for merging to branch-2.0

…ON_WEB_PROXY_BASE ## What changes were proposed in this pull request? Backport SPARK-16808 (#15742) to branch-2.0. Application links generated on the history server UI no longer (regression from 1.6) contain the configured spark.ui.proxyBase in the links. To address this, made the uiRoot available globally to all javascripts for Web UI. Updated the mustache template (historypage-template.html) to include the uiroot for rendering links to the applications. The existing test was not sufficient to verify the scenario where ajax call is used to populate the application listing template, so added a new selenium test case to cover this scenario. ## How was this patch tested? Existing tests and a new unit test. No visual changes to the UI. Author: Vinayak <vijoshi5@in.ibm.com> Closes #15855 from vijoshi/SPARK-16808_branch-2.0.

…ON_WEB_PROXY_BASE ## What changes were proposed in this pull request? Application links generated on the history server UI no longer (regression from 1.6) contain the configured spark.ui.proxyBase in the links. To address this, made the uiRoot available globally to all javascripts for Web UI. Updated the mustache template (historypage-template.html) to include the uiroot for rendering links to the applications. The existing test was not sufficient to verify the scenario where ajax call is used to populate the application listing template, so added a new selenium test case to cover this scenario. ## How was this patch tested? Existing tests and a new unit test. No visual changes to the UI. Author: Vinayak <vijoshi5@in.ibm.com> Closes apache#15742 from vijoshi/SPARK-16808_master.

vijoshi changed the title ~~initial commit~~ [SPARK-16808][Core] History Server main page does not honor APPLICATION_WEB_PROXY_BASE Nov 2, 2016

vijoshi force-pushed the SPARK-16808_master branch from 1b153bf to 0584339 Compare November 3, 2016 06:49

andrewor14 mentioned this pull request Nov 3, 2016

[SPARK-16808][Core] prepend base URI for links on main history server #15739

Closed

vanzin reviewed Nov 4, 2016

View reviewed changes

vijoshi added 4 commits November 5, 2016 21:14

initial commit

2f1a27a

fix scalastyle

b4328e9

update review comments-1

9c1d199

review comments and server stop timeout

4a443c5

vijoshi force-pushed the SPARK-16808_master branch from e314450 to 4a443c5 Compare November 5, 2016 15:46

ajbozarth reviewed Nov 5, 2016

View reviewed changes

review comments 3

a7e380d

vanzin reviewed Nov 8, 2016

View reviewed changes

cleanup in finally in testcase

fcc779d

asfgit closed this in 06a13ec Nov 9, 2016

vijoshi mentioned this pull request Nov 11, 2016

[SPARK-16808][Core] History Server main page does not honor APPLICATION_WEB_PROXY_BASE #15855

Closed

vijoshi deleted the SPARK-16808_master branch January 24, 2017 10:31

[SPARK-16808][Core] History Server main page does not honor APPLICATION_WEB_PROXY_BASE #15742

[SPARK-16808][Core] History Server main page does not honor APPLICATION_WEB_PROXY_BASE #15742

Conversation

vijoshi commented Nov 2, 2016

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Nov 2, 2016

SparkQA commented Nov 3, 2016

SparkQA commented Nov 3, 2016

vijoshi commented Nov 3, 2016

ajbozarth commented Nov 3, 2016

vijoshi commented Nov 4, 2016 • edited Loading

ajbozarth commented Nov 4, 2016

SparkQA commented Nov 4, 2016

vanzin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijoshi commented Nov 5, 2016 • edited Loading

SparkQA commented Nov 5, 2016

vijoshi commented Nov 5, 2016

SparkQA commented Nov 5, 2016

SparkQA commented Nov 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vijoshi commented Nov 6, 2016

SparkQA commented Nov 6, 2016

SparkQA commented Nov 8, 2016

vanzin Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajbozarth commented Nov 8, 2016

vanzin commented Nov 8, 2016

SparkQA commented Nov 8, 2016

vanzin commented Nov 8, 2016

SparkQA commented Nov 9, 2016

vanzin commented Nov 9, 2016

mariobriggs commented Nov 11, 2016

vanzin commented Nov 11, 2016

mariobriggs commented Nov 11, 2016

vijoshi commented Nov 11, 2016

vijoshi commented Nov 4, 2016 •

edited

Loading

vijoshi commented Nov 5, 2016 •

edited

Loading

vanzin Nov 8, 2016 •

edited

Loading