[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/10212 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user jisookim0513 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10212#discussion_r80184799
--- Diff: core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala
---
@@ -1097,7 +1100,9 @@ private[spark] object JsonProtocolSuite extends
Assertions {
| },
| "Task Metrics": {
|"Executor Deserialize Time": 300,
+ |"Executor Deserialize CPU Time": 0,
--- End diff --
Yeah I tested it on my testing cluster, but this makes sense. I will add
non-zero CPU times by setting the CPU times same as given wall times.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user jisookim0513 commented on a diff in the pull request: https://github.com/apache/spark/pull/10212#discussion_r80184744 --- Diff: core/src/test/resources/HistoryServerExpectations/complete_stage_list_json_expectation.json --- @@ -6,6 +6,7 @@ "numCompleteTasks" : 8, "numFailedTasks" : 0, "executorRunTime" : 162, + "executorCpuTime" : 0, --- End diff -- Oh no, these are expected outputs. I think the inputs are stored under `src/test/resources/spark-events`. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/10212#discussion_r80165213
--- Diff: core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala
---
@@ -1097,7 +1100,9 @@ private[spark] object JsonProtocolSuite extends
Assertions {
| },
| "Task Metrics": {
|"Executor Deserialize Time": 300,
+ |"Executor Deserialize CPU Time": 0,
--- End diff --
I still think it would be worth to fix this; just find some way around the
style check.
Otherwise, you're not really testing whether the parsing code is actually
parsing the field (what if there's a typo somewhere?).
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/10212#discussion_r80165121 --- Diff: core/src/test/resources/HistoryServerExpectations/complete_stage_list_json_expectation.json --- @@ -6,6 +6,7 @@ "numCompleteTasks" : 8, "numFailedTasks" : 0, "executorRunTime" : 162, + "executorCpuTime" : 0, --- End diff -- Oh, I though these were input to the history server, not "golden files" that it checks against... if that's the case, ignore my comment. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user jisookim0513 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10212#discussion_r80156532
--- Diff: core/src/main/scala/org/apache/spark/util/JsonProtocol.scala ---
@@ -759,7 +761,15 @@ private[spark] object JsonProtocol {
return metrics
}
metrics.setExecutorDeserializeTime((json \ "Executor Deserialize
Time").extract[Long])
+metrics.setExecutorDeserializeCpuTime((json \ "Executor Deserialize
CPU Time") match {
+ case JNothing => 0
+ case x => x.extract[Long]}
+)
metrics.setExecutorRunTime((json \ "Executor Run Time").extract[Long])
+metrics.setExecutorCpuTime((json \ "Executor CPU Time") match {
+ case JNothing => 0
+ case x => x.extract[Long]}
--- End diff --
Will fix this.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user jisookim0513 commented on a diff in the pull request: https://github.com/apache/spark/pull/10212#discussion_r80156386 --- Diff: core/src/test/resources/HistoryServerExpectations/complete_stage_list_json_expectation.json --- @@ -6,6 +6,7 @@ "numCompleteTasks" : 8, "numFailedTasks" : 0, "executorRunTime" : 162, + "executorCpuTime" : 0, --- End diff -- Hmm I thought HistoryServerSuite runs with included log files (that don't have CPU time). So this is an expected result since those logs don't have cpu time fields. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user jisookim0513 commented on a diff in the pull request:
https://github.com/apache/spark/pull/10212#discussion_r80155278
--- Diff: core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala
---
@@ -1097,7 +1100,9 @@ private[spark] object JsonProtocolSuite extends
Assertions {
| },
| "Task Metrics": {
|"Executor Deserialize Time": 300,
+ |"Executor Deserialize CPU Time": 0,
--- End diff --
AFAIK, JsonProtolSuite creates a JSON string from the event created by
`makeTaskMetrics()`:
'makeTaskMetrics(300L, 400L, 500L, 600L, 700, 800, hasHadoopInput = true,
hasOutput = false))'.
I tried changing `makeTaskMetrics()` to accept deserialize CPU time and CPU
time as arguments , but that ended up violating scalaStyle by having more than
10 parameters..
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/10212#discussion_r80137783
--- Diff: core/src/main/scala/org/apache/spark/util/JsonProtocol.scala ---
@@ -759,7 +761,15 @@ private[spark] object JsonProtocol {
return metrics
}
metrics.setExecutorDeserializeTime((json \ "Executor Deserialize
Time").extract[Long])
+metrics.setExecutorDeserializeCpuTime((json \ "Executor Deserialize
CPU Time") match {
+ case JNothing => 0
+ case x => x.extract[Long]}
+)
metrics.setExecutorRunTime((json \ "Executor Run Time").extract[Long])
+metrics.setExecutorCpuTime((json \ "Executor CPU Time") match {
+ case JNothing => 0
+ case x => x.extract[Long]}
--- End diff --
nit: move '}' to next line (with the ')'). Also above.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/10212#discussion_r80137728 --- Diff: core/src/test/resources/HistoryServerExpectations/complete_stage_list_json_expectation.json --- @@ -6,6 +6,7 @@ "numCompleteTasks" : 8, "numFailedTasks" : 0, "executorRunTime" : 162, + "executorCpuTime" : 0, --- End diff -- Do you need to add these values to all these files? The code should be able to handle the old logs that don't have the value, and not adding these would be a good test case for that. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #10212: [SPARK-12221] add cpu time to metrics
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/10212#discussion_r80137957
--- Diff: core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala
---
@@ -1097,7 +1100,9 @@ private[spark] object JsonProtocolSuite extends
Assertions {
| },
| "Task Metrics": {
|"Executor Deserialize Time": 300,
+ |"Executor Deserialize CPU Time": 0,
--- End diff --
Could you use values other than `0` so that you're sure the code is
actually parsing the value, instead of falling into the default case?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
