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