You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mgaido91 <gi...@git.apache.org> on 2018/02/23 13:49:33 UTC
[GitHub] spark pull request #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPag...
GitHub user mgaido91 opened a pull request:
https://github.com/apache/spark/pull/20663
[SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in order to avoid redundant code
## What changes were proposed in this pull request?
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.
## How was this patch tested?
existing UTs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mgaido91/spark SPARK-23475_followup
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/20663.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #20663
----
commit d246df283e9a900cf114fcdf0eee2951b1bd3713
Author: Marco Gaido <ma...@...>
Date: 2018-02-23T11:01:05Z
[SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in order to avoid redundant code
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in or...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1018/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170407936
--- 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 {
+ 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) {
+ 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)
--- End diff --
In the previous code this was also the case for `SKIPPED`, are you changing that intentionally?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1063/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in or...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87631 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87631/testReport)** for PR 20663 at commit [`d246df2`](https://github.com/apache/spark/commit/d246df283e9a900cf114fcdf0eee2951b1bd3713).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87717 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87717/testReport)** for PR 20663 at commit [`a5c2f2f`](https://github.com/apache/spark/commit/a5c2f2f7368d73ebf405754538bcb8a7fef5e091).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170532241
--- 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 ++= _)
--- End diff --
that won't really works, since `tables.flatten` is a `Seq[NodeSeq]`. But I'll try and do something similar.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/20663
@gengliangwang Mind take a look?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in or...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87631 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87631/testReport)** for PR 20663 at commit [`d246df2`](https://github.com/apache/spark/commit/d246df283e9a900cf114fcdf0eee2951b1bd3713).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87717 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87717/testReport)** for PR 20663 at commit [`a5c2f2f`](https://github.com/apache/spark/commit/a5c2f2f7368d73ebf405754538bcb8a7fef5e091).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in or...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20663
Could you file a separate bug for this cleanup? Thx
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170532005
--- 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 {
+ 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) {
+ 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)
--- End diff --
yes, I realized it while doing the refactor. It was a copy-and-paste mistake, sorry.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in or...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/20663
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87717/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170910979
--- Diff: core/src/main/scala/org/apache/spark/ui/jobs/AllStagesPage.scala ---
@@ -67,152 +41,120 @@ 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 allStages = parent.store.stageList(null)
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(
--- End diff --
since there is some (few) logic which is common to the two (ie. when they are empty), I chose this approach in order to avoid redundancy and enforce coherence. But I am fine also having two separate functions. What do you think @vanzin ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87676/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1103/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in or...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170978622
--- Diff: core/src/main/scala/org/apache/spark/ui/jobs/AllStagesPage.scala ---
@@ -67,152 +41,120 @@ 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 allStages = parent.store.stageList(null)
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(
--- End diff --
I think the current version is fine. It avoids filtering a potentially long list of stages twice.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87670/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in or...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/20663
@vanzin sure, thanks. I am creating a new JIRA. Thank you.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170408040
--- 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 ++= _)
--- End diff --
`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
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170685539
--- Diff: core/src/main/scala/org/apache/spark/ui/jobs/AllStagesPage.scala ---
@@ -67,152 +41,120 @@ 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 allStages = parent.store.stageList(null)
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(allStages, appSummary, _, 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 ++
- {
- if (sc.isDefined && isFairScheduler) {
- <span class="collapse-aggregated-poolTable collapse-table"
- onClick="collapseTable('collapse-aggregated-poolTable','aggregated-poolTable')">
- <h4>
- <span class="collapse-table-arrow arrow-open"></span>
- <a>Fair Scheduler Pools ({pools.size})</a>
- </h4>
- </span> ++
- <div class="aggregated-poolTable collapsible-table">
- {poolTable.toNodeSeq}
- </div>
- } else {
- 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')">
+ val poolsDescription = if (sc.isDefined && isFairScheduler) {
+ <span class="collapse-aggregated-poolTable collapse-table"
+ onClick="collapseTable('collapse-aggregated-poolTable','aggregated-poolTable')">
<h4>
<span class="collapse-table-arrow arrow-open"></span>
- <a>Pending Stages ({pendingStages.size})</a>
+ <a>Fair Scheduler Pools ({pools.size})</a>
</h4>
</span> ++
- <div class="aggregated-allPendingStages collapsible-table">
- {pendingStagesTable.toNodeSeq}
+ <div class="aggregated-poolTable collapsible-table">
+ {poolTable.toNodeSeq}
</div>
+ } else {
+ Seq.empty[Node]
+ }
+
+ val content = summary ++ poolsDescription ++ tables.flatten.flatten
+
+ UIUtils.headerSparkPage("Stages for All Jobs", content, parent)
+ }
+
+ def summaryAndTableForStatus(
+ allStages: Seq[StageData],
+ appSummary: AppSummary,
+ 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, statusName(status), stageTag(status),
+ parent.basePath, subPath, parent.isFairScheduler, killEnabled, isFailedStage)
+ val stagesSize = stages.size
+ (Some(summary(appSummary, status, stagesSize)),
+ Some(table(appSummary, 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 statusName(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 stageTag(status: StageStatus): String = s"${statusName(status)}Stage"
+
+ private def headerDescription(status: StageStatus): String = statusName(status).capitalize
+
+ private def summaryContent(appSummary: AppSummary, status: StageStatus, size: Int): String = {
+ if (status == StageStatus.COMPLETE && appSummary.numCompletedStages != size) {
+ 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(appSummary: AppSummary, status: StageStatus, size: Int): Elem = {
+ val summary =
+ <li>
+ <a href={s"#${statusName(status)}"}>
+ <strong>{headerDescription(status)} Stages:</strong>
+ </a>
+ {summaryContent(appSummary, status, size)}
+ </li>
+
+ if (status == StageStatus.COMPLETE) {
+ summary % Attribute(None, "id", Text("completed-summary"), Null)
+ } else {
+ summary
}
- UIUtils.headerSparkPage("Stages for All Jobs", content, parent)
+ }
+
+ private def table(
+ appSummary: AppSummary,
+ status: StageStatus,
+ stagesTable: StageTableBase, size: Int): NodeSeq = {
--- End diff --
nit: `size` goes in next line
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87670 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87670/testReport)** for PR 20663 at commit [`9a8e032`](https://github.com/apache/spark/commit/9a8e032f6d46cb181e8b775812acdda8805888a8).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87676 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87676/testReport)** for PR 20663 at commit [`9a8e032`](https://github.com/apache/spark/commit/9a8e032f6d46cb181e8b775812acdda8805888a8).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170407811
--- Diff: core/src/main/scala/org/apache/spark/ui/jobs/AllStagesPage.scala ---
@@ -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)
--- End diff --
IIRC the class (`AllStagesPage`) is only instantiated once, and the `render` method is called for each request. So this won't really work.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in or...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87631/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/20663
Jenkins, retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23475][UI][FOLLOWUP] Refactor AllStagesPage in or...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/20663
cc @vanzin
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1111/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87728 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87728/testReport)** for PR 20663 at commit [`a5c2f2f`](https://github.com/apache/spark/commit/a5c2f2f7368d73ebf405754538bcb8a7fef5e091).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20663
Merging to master.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87676 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87676/testReport)** for PR 20663 at commit [`9a8e032`](https://github.com/apache/spark/commit/9a8e032f6d46cb181e8b775812acdda8805888a8).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by gengliangwang <gi...@git.apache.org>.
Github user gengliangwang commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170808031
--- Diff: core/src/main/scala/org/apache/spark/ui/jobs/AllStagesPage.scala ---
@@ -67,152 +41,120 @@ 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 allStages = parent.store.stageList(null)
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(
--- End diff --
Can we have separate functions for `summaries` and `tables`? So that the code is more straight forward.
Overall LGTM.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/20663
Jenkins, retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20663#discussion_r170407883
--- 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 {
+ 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) {
--- End diff --
Fits in previous line.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/1057/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/87728/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20663
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20663: [SPARK-23501][UI] Refactor AllStagesPage in order...
Posted by vanzin <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87728 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87728/testReport)** for PR 20663 at commit [`a5c2f2f`](https://github.com/apache/spark/commit/a5c2f2f7368d73ebf405754538bcb8a7fef5e091).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20663: [SPARK-23501][UI] Refactor AllStagesPage in order to avo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20663
**[Test build #87670 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/87670/testReport)** for PR 20663 at commit [`9a8e032`](https://github.com/apache/spark/commit/9a8e032f6d46cb181e8b775812acdda8805888a8).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org