Skip to content

[SPARK-23501][UI] Refactor AllStagesPage in order to avoid redundant code #20663

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.

Sign up for GitHub

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

Closed
mgaido91 wants to merge 3 commits into from
Closed

[SPARK-23501][UI] Refactor AllStagesPage in order to avoid redundant code #20663

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d246df2
[SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in order to avoid …
mgaido91 Feb 23, 2018
9a8e032
address comments
mgaido91 Feb 26, 2018
a5c2f2f
address comment
mgaido91 Feb 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in order to avoid … 
…redundant code
  • Loading branch information
@mgaido91
mgaido91 committed Feb 23, 2018
commit d246df283e9a900cf114fcdf0eee2951b1bd3713
254 changes: 106 additions & 148 deletions core/src/main/scala/org/apache/spark/ui/jobs/AllStagesPage.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,46 +19,22 @@ package org.apache.spark.ui.jobs

import javax.servlet.http.HttpServletRequest

import scala.xml.{Node, NodeSeq}
import scala.xml.{Attribute, Elem, Node, NodeSeq, Null, Text}

import org.apache.spark.scheduler.Schedulable
import org.apache.spark.status.PoolData
import org.apache.spark.status.api.v1._
import org.apache.spark.status.api.v1.StageStatus
import org.apache.spark.ui.{UIUtils, WebUIPage}

/** Page showing list of all ongoing and recently finished stages and pools */
private[ui] class AllStagesPage(parent: StagesTab) extends WebUIPage("") {
private val sc = parent.sc
private lazy val allStages = parent.store.stageList(null)
Copy link
Contributor

@vanzin vanzin Feb 24, 2018

Choose a reason for hiding this comment

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

IIRC the class (AllStagesPage) is only instantiated once, and the render method is called for each request. So this won't really work.

private lazy val appSummary = parent.store.appSummary()
private val subPath = "stages"
private def isFairScheduler = parent.isFairScheduler

def render(request: HttpServletRequest): Seq[Node] = {
val allStages = parent.store.stageList(null)

val activeStages = allStages.filter(_.status == StageStatus.ACTIVE)
val pendingStages = allStages.filter(_.status == StageStatus.PENDING)
val skippedStages = allStages.filter(_.status == StageStatus.SKIPPED)
val completedStages = allStages.filter(_.status == StageStatus.COMPLETE)
val failedStages = allStages.filter(_.status == StageStatus.FAILED).reverse

val numFailedStages = failedStages.size
val subPath = "stages"

val activeStagesTable =
new StageTableBase(parent.store, request, activeStages, "active", "activeStage",
parent.basePath, subPath, parent.isFairScheduler, parent.killEnabled, false)
val pendingStagesTable =
new StageTableBase(parent.store, request, pendingStages, "pending", "pendingStage",
parent.basePath, subPath, parent.isFairScheduler, false, false)
val completedStagesTable =
new StageTableBase(parent.store, request, completedStages, "completed", "completedStage",
parent.basePath, subPath, parent.isFairScheduler, false, false)
val skippedStagesTable =
new StageTableBase(parent.store, request, skippedStages, "skipped", "skippedStage",
parent.basePath, subPath, parent.isFairScheduler, false, false)
val failedStagesTable =
new StageTableBase(parent.store, request, failedStages, "failed", "failedStage",
parent.basePath, subPath, parent.isFairScheduler, false, true)

// For now, pool information is only accessible in live UIs
val pools = sc.map(_.getAllPools).getOrElse(Seq.empty[Schedulable]).map { pool =>
val uiPool = parent.store.asOption(parent.store.pool(pool.name)).getOrElse(
Expand All @@ -67,66 +43,19 @@ private[ui] class AllStagesPage(parent: StagesTab) extends WebUIPage("") {
}.toMap
val poolTable = new PoolTable(pools, parent)

val shouldShowActiveStages = activeStages.nonEmpty
val shouldShowPendingStages = pendingStages.nonEmpty
val shouldShowCompletedStages = completedStages.nonEmpty
val shouldShowSkippedStages = skippedStages.nonEmpty
val shouldShowFailedStages = failedStages.nonEmpty
val allStatuses = Seq(StageStatus.ACTIVE, StageStatus.PENDING, StageStatus.COMPLETE,
StageStatus.SKIPPED, StageStatus.FAILED)

val appSummary = parent.store.appSummary()
val completedStageNumStr = if (appSummary.numCompletedStages == completedStages.size) {
s"${appSummary.numCompletedStages}"
} else {
s"${appSummary.numCompletedStages}, only showing ${completedStages.size}"
}
val (summaries, tables) = allStatuses.map(summaryAndTableForStatus(_, request)).unzip

val summary: NodeSeq =
<div>
<ul class="unstyled">
{
if (shouldShowActiveStages) {
<li>
<a href="#active"><strong>Active Stages:</strong></a>
{activeStages.size}
</li>
}
}
{
if (shouldShowPendingStages) {
<li>
<a href="#pending"><strong>Pending Stages:</strong></a>
{pendingStages.size}
</li>
}
}
{
if (shouldShowCompletedStages) {
<li id="completed-summary">
<a href="#completed"><strong>Completed Stages:</strong></a>
{completedStageNumStr}
</li>
}
}
{
if (shouldShowSkippedStages) {
<li id="completed-summary">
<a href="#skipped"><strong>Skipped Stages:</strong></a>
{skippedStages.size}
</li>
}
}
{
if (shouldShowFailedStages) {
<li>
<a href="#failed"><strong>Failed Stages:</strong></a>
{numFailedStages}
</li>
}
}
{summaries.flatten}
</ul>
</div>

var content = summary ++
var content: NodeSeq = summary ++
{
if (sc.isDefined && isFairScheduler) {
<span class="collapse-aggregated-poolTable collapse-table"
Expand All @@ -143,76 +72,105 @@ private[ui] class AllStagesPage(parent: StagesTab) extends WebUIPage("") {
Seq.empty[Node]
}
}
if (shouldShowActiveStages) {
content ++=
<span id="active" class="collapse-aggregated-allActiveStages collapse-table"
onClick="collapseTable('collapse-aggregated-allActiveStages',
'aggregated-allActiveStages')">
<h4>
<span class="collapse-table-arrow arrow-open"></span>
<a>Active Stages ({activeStages.size})</a>
</h4>
</span> ++
<div class="aggregated-allActiveStages collapsible-table">
{activeStagesTable.toNodeSeq}
</div>
}
if (shouldShowPendingStages) {
content ++=
<span id="pending" class="collapse-aggregated-allPendingStages collapse-table"
onClick="collapseTable('collapse-aggregated-allPendingStages',
'aggregated-allPendingStages')">
<h4>
<span class="collapse-table-arrow arrow-open"></span>
<a>Pending Stages ({pendingStages.size})</a>
</h4>
</span> ++
<div class="aggregated-allPendingStages collapsible-table">
{pendingStagesTable.toNodeSeq}
</div>

tables.flatten.foreach(content ++= _)
Copy link
Contributor

@vanzin vanzin Feb 24, 2018

Choose a reason for hiding this comment

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

content ++= tables.flatten?

But I think this would be better as:

val summary = blah
val pools = if (sc.isDefined && isFairScheduler) ... else ...
val stages = tables.flatten

val content = summary ++ pools ++ stages

Copy link
Contributor Author

@mgaido91 mgaido91 Feb 26, 2018

Choose a reason for hiding this comment

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

that won't really works, since tables.flatten is a Seq[NodeSeq]. But I'll try and do something similar.


UIUtils.headerSparkPage("Stages for All Jobs", content, parent)
}

def summaryAndTableForStatus(
status: StageStatus,
request: HttpServletRequest): (Option[Elem], Option[NodeSeq]) = {
val stages = if (status == StageStatus.FAILED) {
allStages.filter(_.status == status).reverse
} else {
allStages.filter(_.status == status)
}
if (shouldShowCompletedStages) {
content ++=
<span id="completed" class="collapse-aggregated-allCompletedStages collapse-table"
onClick="collapseTable('collapse-aggregated-allCompletedStages',
'aggregated-allCompletedStages')">
<h4>
<span class="collapse-table-arrow arrow-open"></span>
<a>Completed Stages ({completedStageNumStr})</a>
</h4>
</span> ++
<div class="aggregated-allCompletedStages collapsible-table">
{completedStagesTable.toNodeSeq}
</div>

if (stages.isEmpty) {
(None, None)
} else {
val killEnabled = status == StageStatus.ACTIVE && parent.killEnabled
val isFailedStage = status == StageStatus.FAILED

val stagesTable =
new StageTableBase(parent.store, request, stages, tableHeaderID(status), stageTag(status),
parent.basePath, subPath, parent.isFairScheduler, killEnabled, isFailedStage)
val stagesSize = stages.size
(Some(summary(status, stagesSize)), Some(table(status, stagesTable, stagesSize)))
}
if (shouldShowSkippedStages) {
content ++=
<span id="skipped" class="collapse-aggregated-allSkippedStages collapse-table"
onClick="collapseTable('collapse-aggregated-allSkippedStages',
'aggregated-allSkippedStages')">
<h4>
<span class="collapse-table-arrow arrow-open"></span>
<a>Skipped Stages ({skippedStages.size})</a>
</h4>
</span> ++
<div class="aggregated-allSkippedStages collapsible-table">
{skippedStagesTable.toNodeSeq}
</div>
}

private def tableHeaderID(status: StageStatus): String = status match {
Copy link
Contributor

@vanzin vanzin Feb 24, 2018

Choose a reason for hiding this comment

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

All these look very similar. Having a single one that does the mapping and have the others call that method would be nice.

e.g.

def stageTag(status: StageStatus) = s"${statusName(status)}Stage"

Then you could also get rid of classSuffix, for example, since it's only really called in one place, and the new implementation would be much simpler.

case StageStatus.ACTIVE => "active"
case StageStatus.COMPLETE => "completed"
case StageStatus.FAILED => "failed"
case StageStatus.PENDING => "pending"
case StageStatus.SKIPPED => "skipped"
}

private def stageTag(status: StageStatus): String = status match {
case StageStatus.ACTIVE => "activeStage"
case StageStatus.COMPLETE => "completedStage"
case StageStatus.FAILED => "failedStage"
case StageStatus.PENDING => "pendingStage"
case StageStatus.SKIPPED => "skippedStage"
}

private def headerDescription(status: StageStatus): String = status match {
case StageStatus.ACTIVE => "Active"
case StageStatus.COMPLETE => "Completed"
case StageStatus.FAILED => "Failed"
case StageStatus.PENDING => "Pending"
case StageStatus.SKIPPED => "Skipped"
}

private def classSuffix(status: StageStatus): String = status match {
case StageStatus.ACTIVE => "ActiveStages"
case StageStatus.COMPLETE => "CompletedStages"
case StageStatus.FAILED => "FailedStages"
case StageStatus.PENDING => "PendingStages"
case StageStatus.SKIPPED => "SkippedStages"
}

private def summaryContent(status: StageStatus, size: Int): String = {
if (status == StageStatus.COMPLETE
&& appSummary.numCompletedStages != size) {
Copy link
Contributor

@vanzin vanzin Feb 24, 2018

Choose a reason for hiding this comment

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

Fits in previous line.

s"${appSummary.numCompletedStages}, only showing $size"
} else {
s"$size"
}
if (shouldShowFailedStages) {
content ++=
<span id ="failed" class="collapse-aggregated-allFailedStages collapse-table"
onClick="collapseTable('collapse-aggregated-allFailedStages',
'aggregated-allFailedStages')">
<h4>
<span class="collapse-table-arrow arrow-open"></span>
<a>Failed Stages ({numFailedStages})</a>
</h4>
</span> ++
<div class="aggregated-allFailedStages collapsible-table">
{failedStagesTable.toNodeSeq}
</div>
}

private def summary(status: StageStatus, size: Int): Elem = {
val summary =
<li>
<a href={s"#${tableHeaderID(status)}"}>
<strong>{headerDescription(status)} Stages:</strong>
</a>
{summaryContent(status, size)}
</li>

if (status == StageStatus.COMPLETE) {
summary % Attribute(None, "id", Text("completed-summary"), Null)
Copy link
Contributor

@vanzin vanzin Feb 24, 2018

Choose a reason for hiding this comment

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

In the previous code this was also the case for SKIPPED, are you changing that intentionally?

Copy link
Contributor Author

@mgaido91 mgaido91 Feb 26, 2018

Choose a reason for hiding this comment

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

yes, I realized it while doing the refactor. It was a copy-and-paste mistake, sorry.

} else {
summary
}
UIUtils.headerSparkPage("Stages for All Jobs", content, parent)
}

private def table(status: StageStatus, stagesTable: StageTableBase, size: Int): NodeSeq = {
val classSuffixStatus = classSuffix(status)
<span id={tableHeaderID(status)}
class={s"collapse-aggregated-all$classSuffixStatus collapse-table"}
onClick={s"collapseTable('collapse-aggregated-all$classSuffixStatus'," +
s" 'aggregated-all$classSuffixStatus')"}>
<h4>
<span class="collapse-table-arrow arrow-open"></span>
<a>{headerDescription(status)} Stages ({summaryContent(status, size)})</a>
</h4>
</span> ++
<div class={s"aggregated-all$classSuffixStatus collapsible-table"}>
{stagesTable.toNodeSeq}
</div>
}
}