[SPARK-9375] Make sure the total number of executor(s) requested by the driver is not negative

[KaiXinXiaoLei]

In the code:
if (!replace) {
doRequestTotalExecutors(numExistingExecutors + numPendingExecutors
- executorsPendingToRemove.size - knownExecutors.size)
}

The value of “(numExistingExecutors + numPendingExecutors - executorsPendingToRemove.size - knownExecutors.size)” maybe negative if there is the same executorId in knownExecutors and executorsPendingToRemove. And knownExecutors and executorsPendingToRemove should not have the same executorId.

[SparkQA]

Test build #38653 has finished for PR 7716 at commit a6fb8aa.

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

[KaiXinXiaoLei]

I think this patch has no association with the failed unit tests. And in my machine, this test passed..please retest.

[KaiXinXiaoLei]

Jenkins, retest this please.

[SparkQA]

Test build #133 has finished for PR 7716 at commit a6fb8aa.

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

[SparkQA]

Test build #38684 has finished for PR 7716 at commit a6fb8aa.

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

[srowen]

OK, that looks possibly legitimate -- CC @andrewor14 for a look at this.

[sryza]

@KaiXinXiaoLei we fixed a couple issues that had this symptom in 1.3. Are you definitely running with a version that's 1.4 or later?

[KaiXinXiaoLei]

i run in the latest version.

[sryza]

@andrewor14, the code that @KaiXinXiaoLei suggests fixing seems to be code most recently updated in SPARK-8119.

[KaiXinXiaoLei]

I think it's not the same. There maybe the same executorId in knownExecutors and executorsPendingToRemove.

[andrewor14]

I've bumped the priority of this issue in case it is a regression.

@KaiXinXiaoLei I'm trying to understand how it's possible for an executor ID to be in both knownExecutors and executorsPendingToRemove. In the drive logs of the application that had this negative number, can you search for the this message with no recent heartbeats? I wonder whether this is actually related to the heartbeat receiver mechanism.

[andrewor14]

I guess one way to reproduce this would be to call sc.killExecutor("1") twice. Since it's the same executor both times, we may double count the number of executors to remove, which could result in the -1 thing you saw.

However, I believe this problem already existed in 1.4 well before SPARK-8119 and related changes. The old code didn't guard against that either:

spark/core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala

Line 391 in 4b5bbc5

val newTotal = (numExistingExecutors + numPendingExecutors - executorsPendingToRemove.size

.

[KaiXinXiaoLei]

@andrewor14 I use the code before SPARK-8119. Now i use the latest code, test again, and not found this problem. Now i close PR, Thanks.

[andrewor14]

@KaiXinXiaoLei I think this patch is good to have, however. It does guard against the scenario I discussed earlier, where the user calls sc.killExecutor twice on the same executor. There are many potentially places even within Spark where we might do this, and the result is that we lower the executors target too much. I even suspect this may be the cause of SPARK-9745.

I would like to merge a patch with this change + a regression test. Would you mind re-opening this? If not I can also do it myself.

[andrewor14]

I've opened #8078, which should be functionally the same as this patch + has a regression test.