[GitHub] spark pull request: [SPARK-12221] add cpu time to metrics

[steveloughran <gi...@git.apache.org>]

Github user steveloughran commented on a diff in the pull request:

    https://github.com/apache/spark/pull/10212#discussion_r47083416
  
    --- Diff: core/src/main/scala/org/apache/spark/util/JsonProtocol.scala ---
    @@ -718,6 +719,7 @@ private[spark] object JsonProtocol {
         metrics.setHostname((json \ "Host Name").extract[String])
         metrics.setExecutorDeserializeTime((json \ "Executor Deserialize Time").extract[Long])
         metrics.setExecutorRunTime((json \ "Executor Run Time").extract[Long])
    +    metrics.setExecutorCpuTime((json \ "Executor CPU Time").extract[Long])
    --- End diff --
    
    what happens when the history server is tries to deserialise a history from an earlier spark version, one which doesn't have a CPU time? As it looks to me through my scan through the code that this is going to fail.
    
    HistoryServerSuite is the regression test here —it does have job histories without the relevant metric.
    
    It would benefit from having another reference test run here for playback


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org