You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by noodle-fb <gi...@git.apache.org> on 2017/03/25 00:20:56 UTC
[GitHub] spark pull request #17422: Attach accumulators / metrics to 'TaskKilled' end...
GitHub user noodle-fb opened a pull request:
https://github.com/apache/spark/pull/17422
Attach accumulators / metrics to 'TaskKilled' end reason
## What changes were proposed in this pull request?
The ultimate goal is for listeners to `onTaskEnd` to receive metrics when a task is killed intentionally, since the data is currently just thrown away. This is already done for ExceptionFailure, so this just copies the same approach.
## How was this patch tested?
The unit test in DAGSchedulerSuite that tests this for ExceptionFailure was modified to test the same thing for TaskKilled. I also re-tested all the unit tests modified by the [last change to TaskKilled](https://github.com/apache/spark/commit/8e558041aa0c41ba9fb2ce242daaf6d6ed4d85b7), and made sure they all still pass.
For integration tests, I ran a query that caused a speculative task retry on our deployment, and verified that the metrics showed up in our logging for that retry when it was killed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/noodle-fb/spark task-killed-metrics
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/17422.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #17422
----
commit 0625dc3187c9c8fa8d507adc0da75747c30f0ebc
Author: Charles Lewis <no...@fb.com>
Date: 2017-03-22T20:33:55Z
report metrics for killed tasks
commit ee883b2f3da10a4e4a48f4a98910ccadceac461c
Author: Charles Lewis <no...@fb.com>
Date: 2017-03-24T19:06:46Z
add task killed to exception accum test
commit 25ffbf49b2779d4fa795d754ee20fbe3542dd57d
Author: Charles Lewis <no...@fb.com>
Date: 2017-03-24T23:20:59Z
extra fixes for task killed reason merge
----
---
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
[GitHub] spark issue #17422: Attach accumulators / metrics to 'TaskKilled' end reason
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the issue:
https://github.com/apache/spark/pull/17422
/cc @ericl as FYI.
---
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
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17422
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by advancedxy <gi...@git.apache.org>.
Github user advancedxy commented on the issue:
https://github.com/apache/spark/pull/17422
@noodle-fb are you still working on this? If not, I may work on it based on your current impl.
I am facing same issue here. The accumulator updates are lost for killed tasks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17422: [SPARK-20087] Attach accumulators / metrics to 'T...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/17422#discussion_r165772194
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -429,15 +429,42 @@ private[spark] class Executor(
case t: TaskKilledException =>
logInfo(s"Executor killed $taskName (TID $taskId), reason: ${t.reason}")
+
+ // Collect latest accumulator values to report back to the driver
+ val accums: Seq[AccumulatorV2[_, _]] =
+ if (task != null) {
+ task.metrics.setExecutorRunTime(System.currentTimeMillis() - taskStart)
+ task.metrics.setJvmGCTime(computeTotalGcTime() - startGCTime)
+ task.collectAccumulatorUpdates(taskFailed = true)
+ } else {
+ Seq.empty
+ }
+ val accUpdates = accums.map(acc => acc.toInfo(Some(acc.value), None))
--- End diff --
this should be refactored, and not repeated 3 times.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/17422
add to whitelist
---
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
[GitHub] spark pull request #17422: [SPARK-20087] Attach accumulators / metrics to 'T...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/17422#discussion_r165772055
--- Diff: core/src/main/scala/org/apache/spark/TaskEndReason.scala ---
@@ -212,9 +212,19 @@ case object TaskResultLost extends TaskFailedReason {
* Task was killed intentionally and needs to be rescheduled.
*/
@DeveloperApi
-case class TaskKilled(reason: String) extends TaskFailedReason {
- override def toErrorString: String = s"TaskKilled ($reason)"
+case class TaskKilled(
+ reason: String,
+ accumUpdates: Seq[AccumulableInfo] = Seq.empty,
+ private[spark] var accums: Seq[AccumulatorV2[_, _]] = Nil)
+ extends TaskFailedReason {
+
+ override def toErrorString: String = "TaskKilled ($reason)"
override def countTowardsTaskFailures: Boolean = false
+
+ private[spark] def withAccums(accums: Seq[AccumulatorV2[_, _]]): TaskKilled = {
--- End diff --
I don't think this method is really necessary at all, you could just pass it in the constructor in the places its used.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/17422
@advancedxy this has been quiet for a long time, so I suggest you just take it over. I actually think this is so close to complete that very little would need to be done, and credit would most likely go to @noodle-fb . That said, we may need a little more input on whether or not this is desirable, as it will change the meaning of the aggregated metrics.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17422
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17422: [SPARK-20087] Attach accumulators / metrics to 'T...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/17422
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/17422
retest this please
---
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
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17422
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/17422
@noodle-fb could you rebase this so we can review it? Thanks!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by noodle-fb <gi...@git.apache.org>.
Github user noodle-fb commented on the issue:
https://github.com/apache/spark/pull/17422
@advancedxy, feel free to take this over! @squito, as I remember this, it seemed inconsistent to count metrics for tasks that fail, but not tasks that were killed, machines are doing work in either case. But others might interpret the metrics differently.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by noodle-fb <gi...@git.apache.org>.
Github user noodle-fb commented on the issue:
https://github.com/apache/spark/pull/17422
@JoshRosen ping? not sure how to github correctly
---
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
[GitHub] spark issue #17422: Attach accumulators / metrics to 'TaskKilled' end reason
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/17422
Hi @noodle-fb, it seems not a trivial change that does not need a JIRA. Could we create a JIRA and put this in the title (see http://spark.apache.org/contributing.html)?
---
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
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by noodle-fb <gi...@git.apache.org>.
Github user noodle-fb commented on the issue:
https://github.com/apache/spark/pull/17422
@HyukjinKwon edited with Jira tag, didn't realize that was the naming convention
---
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
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/17422
ok to test
---
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
[GitHub] spark issue #17422: Attach accumulators / metrics to 'TaskKilled' end reason
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17422
Can one of the admins verify this patch?
---
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
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17422
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/17422
retest this please
---
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
[GitHub] spark issue #17422: [SPARK-20087] Attach accumulators / metrics to 'TaskKill...
Posted by advancedxy <gi...@git.apache.org>.
Github user advancedxy commented on the issue:
https://github.com/apache/spark/pull/17422
All right then, I will take it over. Of course the credit should go to @noodle-fb.
We can discuss whether this behaviour is desirable or not in the JIRA or the new PR.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org