Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-17724][Streaming][WebUI] Unevaluated new lines in tooltip in DAG Visualization of a job #16643

Closed
wants to merge 13 commits into from

Conversation

keypointt
Copy link
Contributor

@keypointt keypointt commented Jan 19, 2017

https://issues.apache.org/jira/browse/SPARK-17724

What changes were proposed in this pull request?

For unevaluated \n, evaluate it and enable line break, for Streaming WebUI stages page and job page.
(I didn't change Scala source file, since Jetty server has to somehow indicate line break and js to code display it.)
(This PR is a continue from previous PR #15353 for the same issue, sorry being so long time)

Two changes:

  1. RDD Node tooltipText is actually showing the <circle> title property, so I set extra attribute in spark-dag-viz.js: .attr("data-html", "true")

<circle x="-5" y="-5" r="5" data-toggle="tooltip" data-placement="bottom" title="" data-original-title="ParallelCollectionRDD [9]\nmakeRDD at QueueStream.scala:49"></circle>

  1. Static <tspan> text of each stage, split by /n, and append an extra <tspan> element to its parentNode

<text><tspan xml:space="preserve" dy="1em" x="1">reduceByKey</tspan><tspan xml:space="preserve" dy="1em" x="1">reduceByKey/n@ 23:34:49</tspan></text>

UI changes

Screenshot before fix, \n is not evaluated in both circle tooltipText and static text:
screen shot 2017-01-19 at 12 21 54 am

Screenshot after fix:
screen shot 2017-01-19 at 12 20 30 am

How was this patch tested?

Tested locally. For Streaming WebUI stages page and job page, on multiple browsers:

  • Chrome
  • Firefox
  • Safari

@@ -19,7 +19,7 @@ package org.apache.spark.ui.jobs

import java.util.concurrent.TimeoutException

import scala.collection.mutable.{HashMap, HashSet, LinkedHashMap, ListBuffer}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although these other changes are not wrong, I don't think they're related. Best to stick to the core change you're making.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should undo these unrelated changes here, and leave them as they are?

@SparkQA
Copy link

SparkQA commented Jan 19, 2017

Test build #71647 has finished for PR 16643 at commit 22861c5.

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

@ajbozarth
Copy link
Member

I checked this out and tested it and it runs well, code LGTM as well, but I agree that the unrelated changes should be removed.

@SparkQA
Copy link

SparkQA commented Jan 20, 2017

Test build #71695 has finished for PR 16643 at commit d1c16e2.

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

* For tag 'tspan', line break '/n' is display in UI as raw for both stage page and job page,
* here this function is to enable line break.
*/
function intepreteLineBreak(svg) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interprete -> interpret

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh 😝

* For tag 'tspan', line break '/n' is display in UI as raw for both stage page and job page,
* here this function is to enable line break.
*/
function intepreteLineBreak(svg) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inteprete -> intepret?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interpret actually lol...

@keypointt
Copy link
Contributor Author

sorry...tricky word "interpret" for a non-native English speaker...

@SparkQA
Copy link

SparkQA commented Jan 20, 2017

Test build #71726 has finished for PR 16643 at commit 2619780.

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

@SparkQA
Copy link

SparkQA commented Jan 20, 2017

Test build #71733 has finished for PR 16643 at commit c53db79.

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

@SparkQA
Copy link

SparkQA commented Jan 20, 2017

Test build #71732 has finished for PR 16643 at commit dfd0bbe.

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

@srowen
Copy link
Member

srowen commented Jan 21, 2017

Merged to master

@asfgit asfgit closed this in bcdabaa Jan 21, 2017
zzcclp added a commit to zzcclp/spark that referenced this pull request Jan 22, 2017
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
…AG Visualization of a job

https://issues.apache.org/jira/browse/SPARK-17724

## What changes were proposed in this pull request?
For unevaluated `\n`, evaluate it and enable line break, for Streaming WebUI `stages` page and `job` page.
(I didn't change Scala source file, since Jetty server has to somehow indicate line break and js to code display it.)
(This PR is a continue from previous PR apache#15353 for the same issue, sorry being so long time)

Two changes:

1. RDD Node tooltipText is actually showing the `<circle>`  `title` property, so I set extra attribute in `spark-dag-viz.js`: `.attr("data-html", "true")`

`<circle x="-5" y="-5" r="5" data-toggle="tooltip" data-placement="bottom" title="" data-original-title="ParallelCollectionRDD [9]\nmakeRDD at QueueStream.scala:49"></circle>`

2. Static `<tspan>` text of each stage, split by `/n`, and append an extra `<tspan>` element to its parentNode

`<text><tspan xml:space="preserve" dy="1em" x="1">reduceByKey</tspan><tspan xml:space="preserve" dy="1em" x="1">reduceByKey/n 23:34:49</tspan></text>
`

## UI changes
Screenshot **before fix**, `\n` is not evaluated in both circle tooltipText and static text:
![screen shot 2017-01-19 at 12 21 54 am](https://cloud.githubusercontent.com/assets/3925641/22098829/53c7f49c-dddd-11e6-9daa-b3ddb6044114.png)

Screenshot **after fix**:
![screen shot 2017-01-19 at 12 20 30 am](https://cloud.githubusercontent.com/assets/3925641/22098806/294910d4-dddd-11e6-9948-d942e09f545e.png)

## How was this patch tested?
Tested locally. For Streaming WebUI `stages` page and `job` page, on multiple browsers:
- Chrome
- Firefox
- Safari

Author: Xin Ren <renxin.ubc@gmail.com>

Closes apache#16643 from keypointt/SPARK-17724-2nd.
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Feb 15, 2017
…AG Visualization of a job

https://issues.apache.org/jira/browse/SPARK-17724

## What changes were proposed in this pull request?
For unevaluated `\n`, evaluate it and enable line break, for Streaming WebUI `stages` page and `job` page.
(I didn't change Scala source file, since Jetty server has to somehow indicate line break and js to code display it.)
(This PR is a continue from previous PR apache#15353 for the same issue, sorry being so long time)

Two changes:

1. RDD Node tooltipText is actually showing the `<circle>`  `title` property, so I set extra attribute in `spark-dag-viz.js`: `.attr("data-html", "true")`

`<circle x="-5" y="-5" r="5" data-toggle="tooltip" data-placement="bottom" title="" data-original-title="ParallelCollectionRDD [9]\nmakeRDD at QueueStream.scala:49"></circle>`

2. Static `<tspan>` text of each stage, split by `/n`, and append an extra `<tspan>` element to its parentNode

`<text><tspan xml:space="preserve" dy="1em" x="1">reduceByKey</tspan><tspan xml:space="preserve" dy="1em" x="1">reduceByKey/n 23:34:49</tspan></text>
`

## UI changes
Screenshot **before fix**, `\n` is not evaluated in both circle tooltipText and static text:
![screen shot 2017-01-19 at 12 21 54 am](https://cloud.githubusercontent.com/assets/3925641/22098829/53c7f49c-dddd-11e6-9daa-b3ddb6044114.png)

Screenshot **after fix**:
![screen shot 2017-01-19 at 12 20 30 am](https://cloud.githubusercontent.com/assets/3925641/22098806/294910d4-dddd-11e6-9948-d942e09f545e.png)

## How was this patch tested?
Tested locally. For Streaming WebUI `stages` page and `job` page, on multiple browsers:
- Chrome
- Firefox
- Safari

Author: Xin Ren <renxin.ubc@gmail.com>

Closes apache#16643 from keypointt/SPARK-17724-2nd.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants