Github user smurakozi commented on a diff in the pull request:
https://github.com/apache/spark/pull/19920#discussion_r155718836
--- Diff:
core/src/main/scala/org/apache/spark/deploy/history/HistoryPage.scala ---
@@ -88,4 +90,8 @@ private[history] class HistoryPage(parent: HistoryServer)
extends WebUIPage("")
private def makePageLink(showIncomplete: Boolean): String = {
UIUtils.prependBaseUri("/?" + "showIncomplete=" + showIncomplete)
}
+
+ private def isApplicationCompleted(appInfo: ApplicationInfo): Boolean = {
--- End diff --
My first commit had this method in ApplicationInfo, called completed
@vanzin:
`+case class ApplicationInfo private[spark](
...
+ def completed: Boolean = {`
Is this used anywhere?
I answered we have it in HistoryPage (forgot to mention usage in the test)
@vanzin: Can we move this logic there instead? That avoids adding a new
visible property in a public object.
I moved it, noting the code duplication in a comment on PR.
The question is which is better: a bit of duplication (used 3 and defined 2
times, once in application and once in test code) or extending the public API?
My gut feeling is that the small duplication is preferable in this case,
but I'm a newbie in Spark development so I'm easy to convince that we have
different preferences :)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
