-
Notifications
You must be signed in to change notification settings - Fork 28k
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-2384] Add tooltips to UI. #1314
Conversation
Merged build triggered. |
Merged build started. |
Merged build finished. |
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16363/ |
Awesome. I've been wanting to do this for a long time! |
import org.apache.spark.util.Utils | ||
|
||
private case class ExecutorInfo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already an ExecutorInfo class... we should call it something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing that -- changed to ExecutorSummaryInfo (happy to do something else if you have a another idea)
Thanks for the review @andrewor14 -- uploaded a commit addressing your comments! The Jenkins build is failing because the jquery file doesn't have a license but it's not obvious to me that we should add one -- hoping @mateiz or @pwendell can weigh in here about what to do. |
Merged build triggered. |
Merged build started. |
Merged build finished. |
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16387/ |
You can exclude the licensing check by editing .rat-excludes file. One thing I'm not sure about is whether it is worth it to introduce jquery and other library just for the sake of a tooltip. We can probably implement the hover tooltip thing in a few lines of JavaScript code ... |
@rxin what if we merged this as-is and then we create a starter jira to write a small library for doing tooltips and remove jquery? I think @kayousterhout probably doesn't have enough time/javascript foo to do that and this is otherwise a very useful feature. |
Definitely we can merge this first. I can submit a PR later to remove jquery. |
In the long run, if we decide to introduce visualization to our UI (e.g. of execution times) we may use another library, possibly on top of jquery. It's not clear to me that we should re-implement all parts of these libraries that we want to use. Maybe instead of including them in Spark we should link them, and only promise these extra features if your SparkUI has internet connection or something. |
Hi @andrewor14 We would need to at least keep a local fallback of the jquery even if we use a CDN version because a lot of production Spark deployments don't actually have access to the external Internet. For visualization, it is not yet clear what we are going to include in the future. And even if we do, most visualization libs work perfectly fine without jquery. I was not advocating re-implementing all the features of all these libraries. Rather, most of the time it is trivial to implement the functionalities we need via vanilla JS. In this case, I bet we can implement a comparable tooltip in dozens of lines. If we need more features of jquery, we can pull it in later. |
Merged build triggered. |
Merged build started. |
Merged build finished. |
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16417/ |
Jenkins, retest this please On Tue, Jul 8, 2014 at 12:31 PM, UCB AMPLab notifications@github.com
|
Jenkins, retest this please |
Merged build triggered. |
Merged build started. |
Merged build finished. All automated tests passed. |
All automated tests passed. |
Merging this in master. Thanks! |
This patch adds tooltips to clarify some points of confusion in the UI. When users mouse over some of the table headers (shuffle read, write, and input size) as well as over the "scheduler delay" metric shown for each stage, a black tool tip (see image below) pops up describing the metric in more detail. After the tooltip mechanism is added by this commit, I imagine others may want to add more tooltips for other things in the UI, but I think this is a good starting point. ![tooltip](https://cloud.githubusercontent.com/assets/1108612/3491905/994e179e-059f-11e4-92f2-c6c12d248d81.jpg) This looks scary-big but much of it is adding the bootstrap tool tip JavaScript. Also I have no idea what to put for the license in tooltip (I left it the same -- the Twitter apache header) or for JQuery (left it as nothing) -- @mateiz what's the right thing here? cc @pwendell @andrewor14 @rxin Author: Kay Ousterhout <kayousterhout@gmail.com> Closes apache#1314 from kayousterhout/tooltips and squashes the following commits: 19981b5 [Kay Ousterhout] Exclude non-licensed javascript files from style check d9ab5a9 [Kay Ousterhout] Response to Andrew's review 7752449 [Kay Ousterhout] [SPARK-2384] Add tooltips to UI.
* Migrate to log4j2 + log4j1 api bridge. * Fix ReplSuite * Fix HiveHadoopDelegationTokenManagerSuite * Remove debug log. * More Co-authored-by: Liang-Chi Hsieh <viirya@gmail.com>
This patch adds tooltips to clarify some points of confusion in the UI. When users mouse over some of the table headers (shuffle read, write, and input size) as well as over the "scheduler delay" metric shown for each stage, a black tool tip (see image below) pops up describing the metric in more detail. After the tooltip mechanism is added by this commit, I imagine others may want to add more tooltips for other things in the UI, but I think this is a good starting point.
This looks scary-big but much of it is adding the bootstrap tool tip JavaScript.
Also I have no idea what to put for the license in tooltip (I left it the same -- the Twitter apache header) or for JQuery (left it as nothing) -- @mateiz what's the right thing here?
cc @pwendell @andrewor14 @rxin