[SPARK-2299] Consolidate various stageIdTo* hash maps in JobProgressListener #1262
Conversation
|
Merged build triggered. |
|
Merged build started. |
| @@ -67,28 +61,28 @@ private[ui] class StagePage(parent: JobProgressTab) extends WebUIPage("stage") { | |||
| <ul class="unstyled"> | |||
| <li> | |||
| <strong>Total task time across all tasks: </strong> | |||
| {UIUtils.formatDuration(listener.stageIdToTime.getOrElse(stageId, 0L) + activeTime)} | |||
| {UIUtils.formatDuration(stageData.executorRunTime)} | |||
There was a problem hiding this comment.
Note that I dropped activeTime here (time taken for currently active tasks) because I'm not sure if the extra data structure required to track this is worth the benefit (I don't know if anybody really looks at this ...)
There was a problem hiding this comment.
For tasks that have already finished, doesn't activeTime already include executorRunTime? If so, aren't we double counting for those tasks? It seems to me that what it was before was plain wrong, though maybe I'm misunderstanding something.
|
Merged build finished. All automated tests passed. |
|
All automated tests passed. |
| @@ -17,6 +17,8 @@ | |||
|
|
|||
| package org.apache.spark.ui.jobs | |||
|
|
|||
| import org.apache.spark.ui.jobs.UIData.StageUIData | |||
|
|
|||
There was a problem hiding this comment.
nit: mind grouping this with other org.apache.spark imports?
|
Should we have only "stageId" as key of these HashMap? |
|
Yeah I think @rxin is going to change this so that we index on both the stage and attempt. Also, we'll need to extend the listener interface to give both the attempt and stage for a task. |
Conflicts: core/src/main/scala/org/apache/spark/ui/jobs/ExecutorSummary.scala core/src/main/scala/org/apache/spark/ui/jobs/ExecutorTable.scala core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala core/src/main/scala/org/apache/spark/ui/jobs/StageTable.scala
|
I pushed a new version. I'd first merge this and then have a separate PR to index the hash table by stageId + attempt. Now it includes @kayousterhout's change. Please take another look. |
|
QA tests have started for PR 1262. This patch merges cleanly. |
|
QA results for PR 1262: |
|
QA tests have started for PR 1262. This patch merges cleanly. |
|
QA results for PR 1262: |
|
Merging in master. Thanks for reviewing. |
…istener This should reduce memory usage for the web ui as well as slightly increase its speed in draining the UI event queue. @andrewor14 Author: Reynold Xin <rxin@apache.org> Closes apache#1262 from rxin/ui-consolidate-hashtables and squashes the following commits: 1ac3f97 [Reynold Xin] Oops. Properly handle description. f5736ad [Reynold Xin] Code review comments. b8828dc [Reynold Xin] Merge branch 'master' into ui-consolidate-hashtables 7a7b6c4 [Reynold Xin] Revert css change. f959bb8 [Reynold Xin] [SPARK-2299] Consolidate various stageIdTo* hash maps in JobProgressListener to speed it up. 63256f5 [Reynold Xin] [SPARK-2320] Reduce <pre> block font size.
This should reduce memory usage for the web ui as well as slightly increase its speed in draining the UI event queue.
@andrewor14