[SPARK-9795] Dynamic allocation: avoid double counting when killing same executor twice

[andrewor14]

This is based on @KaiXinXiaoLei's changes in #7716.

The issue is that when someone calls sc.killExecutor("1") on the same executor twice quickly, then the executor target will be adjusted downwards by 2 instead of 1 even though we're only actually killing one executor. In certain cases where we don't adjust the target back upwards quickly, we'll end up with jobs hanging.

This is a common danger because there are many places where this is called:

• HeartbeatReceiver kills an executor that has not been sending heartbeats
• ExecutorAllocationManager kills an executor that has been idle
• The user code might call this, which may interfere with the previous callers

While it's not clear whether this fixes SPARK-9745, fixing this potential race condition seems like a strict improvement. I've added a regression test to illustrate the issue.

[andrewor14]

@vanzin @srowen could you have a look?

[harishreedharan]

Nit: It looks like unknownExecutors is useful only for that one log statement. If we don't need that log line, we could reduce the number of set copies and traversals. How about something like this (this is less scala-like, but reduces the number of traversals and copies):

val knownExecutors = new HashSet[String]
executorsIds.foreach { id =>
  if (executorDataMap.contains(id)) {
    knownExecutors += id
  }
}

This also makes the other changes in this file unnecessary.

[andrewor14]

That looks fine, but in this patch I wanted to limit the scope of the changes so I'm going to leave this as is.

[harishreedharan]

LGTM

[vanzin]

LGTM

[SparkQA]

Test build #40318 has finished for PR 8078 at commit fb149da.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[andrewor14]

retest this please

[SparkQA]

Test build #1430 timed out for PR 8078 at commit fb149da after a configured wait of 175m.

[SparkQA]

Test build #1429 timed out for PR 8078 at commit fb149da after a configured wait of 175m.

[andrewor14]

retest this please

[SparkQA]

Test build #40479 has finished for PR 8078 at commit fb149da.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[andrewor14]

retest this please

[andrewor14]

retest this please

[SparkQA]

Test build #40508 has finished for PR 8078 at commit fb149da.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1460 has finished for PR 8078 at commit fb149da.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[JoshRosen]

Jenkins, retest this please.

[andrewor14]

retest this please, just in case

[SparkQA]

Test build #40593 has finished for PR 8078 at commit fb149da.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[andrewor14]

retest this please. Pretty sure I didn't change any SQL...

[SparkQA]

Test build #40596 has finished for PR 8078 at commit fb149da.

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

[SparkQA]

Test build #40608 has finished for PR 8078 at commit fb149da.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[andrewor14]

Alright, the latest commit actually passed tests so I'm going to merge this into master 1.5.