Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/16189#discussion_r91235005
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -161,12 +163,7 @@ private[spark] class Executor(
* @param interruptThread whether to interrupt the task thread
*/
def killAllTasks(interruptThread: Boolean) : Unit = {
- // kill all the running tasks
- for (taskRunner <- runningTasks.values().asScala) {
- if (taskRunner != null) {
- taskRunner.kill(interruptThread)
- }
- }
+ runningTasks.keys().asScala.foreach(t => killTask(t, interruptThread =
interruptThread))
--- End diff --
A careful reviewer will notice that it's possible for `killTask` to be
called twice for the same task, either via multiple calls to `killTask` here or
via a call to `killTask` followed by a later `killAllTasks` call. I think that
this should technically be okay as of the code in this first draft of this
patch since having multiple TaskReapers for the same task should be fine, but I
can also appreciate how this could cause resource exhaustion issues in the
pathological case where killTask is spammed continuously. If we think it's
important to avoid multiple reapers in this case then a simple solution would
be to add a `synchronized` method on `TaskRunner` which submits a `TaskReaper`
on the first kill request and is a no-op on subsequent requests.
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
