-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-23475][UI] Show also skipped stages #20651
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
Conversation
|
Test build #87587 has finished for PR 20651 at commit
|
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.
It'd be nice to update UISeleniumSuite to add a unit test. I checked an it only tests skipped stages on the jobs page, but not on the stages list.
| } | ||
| if (shouldShowSkippedStages) { | ||
| content ++= | ||
| <span id="skipped" class="collapse-aggregated-allSkippedStages collapse-table" |
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 see things like collapse-aggregated-allCompletedStages are mentioned in webui.js, so probably this one should be added there.
This is kinda past the point where this block should be refactored (it's repeated 5 times now), but probably ok for a 2.3 change.
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, I added the UT. I think we can create a new JIRA/followup PR to resolve the code redundancy issue you rightly pointed out, what do you think?
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.
Sounds good to clean things up in a follow-up.
|
Test build #87615 has finished for PR 20651 at commit
|
|
Jenkins, retest this please |
|
Test build #87617 has finished for PR 20651 at commit
|
|
LGTM, merging to master / 2.3. |
|
Merged to master, but there were conflicts in 2.3; please open a separate PR for the backport. |
|
thanks @vanzin I created the PR for the backport. Thanks. |
SPARK-20648 introduced the status `SKIPPED` for the stages. On the UI, previously, skipped stages were shown as `PENDING`; after this change, they are not shown on the UI. The PR introduce a new section in order to show also `SKIPPED` stages in a proper table. manual tests Author: Marco Gaido <[email protected]> Closes apache#20651 from mgaido91/SPARK-23475.
…code As suggested in #20651, the code is very redundant in `AllStagesPage` and modifying it is a copy-and-paste work. We should avoid such a pattern, which is error prone, and have a cleaner solution which avoids code redundancy. existing UTs Author: Marco Gaido <[email protected]> Closes #20663 from mgaido91/SPARK-23475_followup.
What changes were proposed in this pull request?
SPARK-20648 introduced the status
SKIPPEDfor the stages. On the UI, previously, skipped stages were shown asPENDING; after this change, they are not shown on the UI.The PR introduce a new section in order to show also
SKIPPEDstages in a proper table.How was this patch tested?
manual tests