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

[roxchkplusony]

No description provided.

[roxchkplusony]

I largely stole the structure from the status update message handler.

[AmplabJenkins]

Can one of the admins verify this patch?

[andrewor14]

add to whitelist

[andrewor14]

This message is unclear. How about just

"Attempted to kill task $taskId for unknown executor $executorId."

[roxchkplusony]

I have no problem doing that. Do you think StatusUpdate's message is clear as-is?

[SparkQA]

Test build #23902 has started for PR 3483 at commit 5e7fdea.

• This patch merges cleanly.

[SparkQA]

Test build #23902 has finished for PR 3483 at commit 5e7fdea.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23902/
Test PASSed.

[SparkQA]

Test build #23907 has started for PR 3483 at commit aba9184.

• This patch merges cleanly.

[SparkQA]

Test build #23907 has finished for PR 3483 at commit aba9184.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23907/
Test PASSed.

[pwendell]

LGTM

[markhamstra]

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.")
}

[pwendell]

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.

[roxchkplusony]

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?

[pwendell]

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

[markhamstra]

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.

[rxin]

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.")
        }

:)

[roxchkplusony]

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.

[roxchkplusony]

At this point, the merits of the change have disappeared from discussion and now we're onto style questions. Since this change does not diverge from existing patterns, can we move forward? Anyone who cares enough about the style question is free to make a separate PR. Is there anything left to do before accepting or rejecting this PR?

[markhamstra]

Separating style from substance and making purely stylistic PRs is not really a good idea. That only serves to complicate the revision history and make the backtracking of substantive changes more difficult. It's better to get the form and the content right at the same time in a single PR.

[rxin]

I'm fine with the current style actually.

[rxin]

Merging in master & branch-1.2 so this can make the first 1.2 rc.

[rxin]

Victor - would you be interested in submitting a patch against branch-1.1 as well?

[roxchkplusony]

Thanks @rxin! Style-wise I agree. Funny that you put up another alternative :-P

[roxchkplusony]

Gladly, after a little break and a chance to figure out upstream branches... lol.