[SPARK-23501][UI] Refactor AllStagesPage in order to avoid redundant code #20663
Conversation
|
cc @vanzin |
|
Test build #87631 has finished for PR 20663 at commit
|
|
Could you file a separate bug for this cleanup? Thx |
|
@vanzin sure, thanks. I am creating a new JIRA. Thank you. |
mgaido91
changed the title
| /** 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) |
There was a problem hiding this comment.
IIRC the class (AllStagesPage) is only instantiated once, and the render method is called for each request. So this won't really work.
| </div> | ||
| } | ||
|
|
||
| private def tableHeaderID(status: StageStatus): String = status match { |
There was a problem hiding this comment.
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.
|
|
||
| private def summaryContent(status: StageStatus, size: Int): String = { | ||
| if (status == StageStatus.COMPLETE | ||
| && appSummary.numCompletedStages != size) { |
| </li> | ||
|
|
||
| if (status == StageStatus.COMPLETE) { | ||
| summary % Attribute(None, "id", Text("completed-summary"), Null) |
There was a problem hiding this comment.
In the previous code this was also the case for SKIPPED, are you changing that intentionally?
There was a problem hiding this comment.
yes, I realized it while doing the refactor. It was a copy-and-paste mistake, sorry.
| {pendingStagesTable.toNodeSeq} | ||
| </div> | ||
|
|
||
| tables.flatten.foreach(content ++= _) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
that won't really works, since tables.flatten is a Seq[NodeSeq]. But I'll try and do something similar.
|
@gengliangwang Mind take a look? |
|
Test build #87670 has finished for PR 20663 at commit
|
|
Jenkins, retest this please |
| private def table( | ||
| appSummary: AppSummary, | ||
| status: StageStatus, | ||
| stagesTable: StageTableBase, size: Int): NodeSeq = { |
There was a problem hiding this comment.
nit: size goes in next line
|
Test build #87676 has finished for PR 20663 at commit
|
| s"${appSummary.numCompletedStages}, only showing ${completedStages.size}" | ||
| } | ||
|
|
||
| val (summaries, tables) = allStatuses.map( |
There was a problem hiding this comment.
Can we have two separate functions for getting summaries and tables? So that the code is more straight forward.
Overall LGTM.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
I think the current version is fine. It avoids filtering a potentially long list of stages twice.
|
Test build #87717 has finished for PR 20663 at commit
|
|
Jenkins, retest this please |
|
Test build #87728 has finished for PR 20663 at commit
|
|
Merging to master. |
5 participants
What changes were proposed in this pull request?
As suggested in #20651, the code is very redundant in
AllStagesPageand 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