-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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-2250: show stage RDDs in UI #1188
Conversation
Can one of the admins verify this patch? |
Hey @nevillelyh, can you attach a screenshot or two? |
@andrewor14 I resolved merge conflicts and posted screenshot above. |
@andrewor14 ping |
Jenkins, test this please |
QA tests have started for PR 1188. This patch merges cleanly. |
QA results for PR 1188: |
Fixed scalastyle warning. I thought I was whitelisted in AmplabJenkins... |
Jenkins, test this please |
QA tests have started for PR 1188. This patch merges cleanly. |
QA results for PR 1188: |
Sorry missed one. Fixed the style warning. |
@andrewor14 ping again? |
Hi @nevillelyh, this seems a little too much information, and the use cases aren't immediately clear to me. If we show only persisted RDDs, however, that might be helpful to some people, but even then we still need to run the idea by other people to see if we want this feature. |
add to whitelist |
@andrewor14 it's useful in apps with many iterations, i.e. ALS. I can filter out RDDs with (default) numeric names or 0 cached partitions if that make more sense? |
QA tests have started for PR 1188. This patch merges cleanly. |
QA results for PR 1188: |
@nevillelyh As a first step it would be interesting to see a version that only includes persisted RDD's. In that case the RDD name could link to the relevant page in the storage tab page. I'm not 100% sure on this, but seems like that might be useful. |
QA tests have started for PR 1188. This patch merges cleanly. |
QA results for PR 1188: |
Sorry I should have said this earlier, but given that there can be only one cached RDD per stage, what about just saying "Persisted RDD: XX"? Having a drop down isn't necessary any more. That will also make this diff a lot smaller I think, which is an added bonus! |
Sounds good, I'll revert back to "show details" and include RDD in drop down. |
@nevillelyh I like "show call stack" actually (it's more accurate). So fine if you keep that. Or maybe it could even just say "call stack". Anyways fine either way. |
} | ||
|
||
val stageDataOption = listener.stageIdToData.get(s.stageId) | ||
// Too many nested map/flatMaps with options are just annoying to read. Do this imperatively. | ||
if (stageDataOption.isDefined && stageDataOption.get.description.isDefined) { | ||
val desc = stageDataOption.get.description | ||
<div><em>{desc}</em></div><div>{nameLink} {killLink}</div> | ||
<div><em>{desc}</em></div><div>{killLink} {nameLink} {details}</div> |
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.
Is this a separate fix here? (looks like we didn't correctly add details in this case)
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.
Yeah I reused SPARK-2035, the ticket that introduced "details" tab.
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.
ah okay cool
{if (cachedRddInfos.nonEmpty) { | ||
<strong>Persisted RDD:</strong> ++ | ||
cachedRddInfos.map { i => | ||
<a href={"%s/storage/rdd?id=%d".format(UIUtils.prependBaseUri(basePath), i.id)}> |
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.
I tested this locally and it would be good to pull this onto one line, because otherwise the hyperlink renders with a space at the beginning. If you need to violate the style guidelines you can disable scalastyle using //scalastyle off
and //scalastyle on
(see where we do this elsewhere in the UI code).
Built and tested it locally. I was actually proposing pulling the Persisted RDD part outside of the toggle component. I think it's fine either way, but if we decide to put it in the toggle box, I agree the name should be more generically "details". If we decide to pull it outside of the toggle box, then we can leave the box as "stack trace". I slightly prefer leaving it out, but maybe if the name is really long that will be annoying? I'm worried people will never see it if it's hidden in the toggle. |
One more thing - after playing in my browser I think just saying "RDD: XX" is sufficient. Not sure Persistent adds much, and it will save space. Still prefer it non-bold. |
Addressed most of the comments. I kept it inside the box to avoid clutter in case name gets too long. Also renamed link to "+details" |
QA tests have started for PR 1188. This patch merges cleanly. |
QA results for PR 1188: |
<div class="stage-details collapsed"> | ||
{if (cachedRddInfos.nonEmpty) { | ||
Text("RDD: ") ++ | ||
// scalastyle:off |
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.
Oh fancy! It would also have been okay to just comment out the entire code block :P
Okay I tested locally - this is cool! Merging it. |
Author: Neville Li <neville@spotify.com> Closes apache#1188 from nevillelyh/neville/ui and squashes the following commits: d3ac425 [Neville Li] SPARK-2250: show persisted RDD in stage UI f075db9 [Neville Li] SPARK-2035: show call stack even when description is available
No description provided.