[SPARK-19757][CORE] DriverEndpoint#makeOffers race against CoarseGrainedSchedulerBackend#killExecutors

[jxiang]

What changes were proposed in this pull request?

While some executors are being killed due to idleness, if some new tasks come in, driver could assign them to some executors are being killed. These tasks will fail later when the executors are lost. This patch is to make sure CoarseGrainedSchedulerBackend#killExecutors and DriverEndpoint#makeOffers are properly synchronized.

How was this patch tested?

manual tests

[vanzin]

Can you fix the title and description?

The title should summarize the change, not the bug. The description should explain better why this is necessary, and you left parts of the template in it.

[vanzin]

What is this racing against?

If it's requestTotalExecutors you're using the wrong lock, since that method is in a different class. It would be good to add comments mentioning the race (like the code handling the RegisterExecutor message).

[jxiang]

Good question. Updated the description and the title, and added comment to mention the race.
Fixed the synchronized statement.

[vanzin]

ok to test

[SparkQA]

Test build #73859 has finished for PR 17091 at commit 6a04ff7.

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

[vanzin]

Can you add back the bug to the PR title?

[jxiang]

Sure. Thanks.

[jxiang]

@vanzin I updated the patch a little so as not to kill an executor if it is busy.

[vanzin]

I don't think this should change. This is the user's code explicitly asking for executors to be killed, so there's no reason to not honor that request. It would also change the behavior of a public API, and there's no way for the user to know which executors were killed or not from the method's return value.

[jxiang]

Just found out that SPARK-16554 just fixed the issue of killing busy executors. Reverted this change and updated the patch. Thanks.

[SparkQA]

Test build #74046 has finished for PR 17091 at commit 67b874e.

• This patch fails from timeout after a configured wait of `250m`.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #74117 has finished for PR 17091 at commit 9ade0fd.

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

[vanzin]

Did you mean to close this?

[jxiang]

Oh, sorry, I closed it by mistake. I'd like to get it in. Thanks.

[vanzin]

ok to test

[SparkQA]

Test build #74214 has finished for PR 17091 at commit 9ade0fd.

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

[vanzin]

Merging to master.