Skip to content

[SPARK-4626] Kill a task only if the executorId is (still) registered with the scheduler #3483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[SPARK-4626] Kill a task only if the executorId is (still) registered with the scheduler #3483

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e7fdea
[SPARK-4626] Kill a task only if the executorId is (still) registered…
roxchkplusony Nov 26, 2014
aba9184
replace warning message per review
roxchkplusony Nov 26, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,13 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val actorSyste
makeOffers()

case KillTask(taskId, executorId, interruptThread) =>
executorDataMap(executorId).executorActor ! KillTask(taskId, executorId, interruptThread)
executorDataMap.get(executorId) match {
case Some(executorInfo) =>
executorInfo.executorActor ! KillTask(taskId, executorId, interruptThread)
case None =>
// Ignoring the task kill since the executor is not registered.
logWarning(s"Attempted to kill task $taskId for unknown executor $executorId.")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to avoid pattern matching on Option, so:

executorDataMap.get(executorId).map { executorInfo =>
  executorInfo.executorActor ! KillTask(taskId, executorId, interruptThread)
} getOrElse { 
  logWarning(s"Attempted to kill task $taskId for unknown executor $executorId.")
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the current approach is more explicit and readable, even if someone is not a scala expert. Using getOrElse to just have a side effect of printing a log statement is also a bit awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the general objection (pattern matching is usually a cop-out to better functional style) but that's not the appropriate pattern here. map is specifically designed to apply a morphism from A -> B (in Scala, f: A => B) to describe Option[A] -> Option[B]. What we are doing here is applying a choice of side effect, not a value, depending on the concrete Option. The example is (Option[A] -> Option[Unit]) -> Unit with misuse of monadic operators. Also, this code applies patterns found consistently elsewhere in this class file. If you believe strongly in this pattern, would you mind opening a PR for review?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curiosu what others think on this style point (@rxin)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't feel all that strongly about it, but neither does it strike me as awkward or a misuse of monadic operators. Unit is a perfectly valid return type, and this is just idiomatic Scala -- which I prefer to use consistently over Scala written for non-Scala programmers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

        val executorInfoOpt = executorDataMap.get(executorId)
        if (executorInfoOpt.isDefined) {
          executorInfoOpt.get.executorActor ! KillTask(taskId, executorId, interruptThread)
        } else {
          // Ignoring the task kill since the executor is not registered.
          logWarning(s"Attempted to kill task $taskId for unknown executor $executorId.")
        }

:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it less, but it's a close number two next to the existing pattern. I wouldn't object to keeping them the same or moving to your pattern.


case StopDriver =>
sender ! true
Expand Down