Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20663#discussion_r170407867
  
    --- Diff: core/src/main/scala/org/apache/spark/ui/jobs/AllStagesPage.scala 
---
    @@ -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 ++= _)
    +
    +    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 {
    --- End diff --
    
    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.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to