[SPARK-794][Core] Remove sleep() in ClusterScheduler.stop

[brennonyork]

Removed sleep() from the stop() method of the TaskSchedulerImpl class which, from the JIRA ticket, is believed to be a legacy artifact slowing down testing originally introduced in the ClusterScheduler class.

[AmplabJenkins]

Can one of the admins verify this patch?

[rxin]

Jenkins, test this please.

[SparkQA]

Test build #24921 has started for PR 3851 at commit 04c3e64.

• This patch merges cleanly.

[SparkQA]

Test build #24921 has finished for PR 3851 at commit 04c3e64.

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

[AmplabJenkins]

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

[JoshRosen]

Jenkins, retest this please.

[SparkQA]

Test build #24926 has started for PR 3851 at commit 04c3e64.

• This patch merges cleanly.

[SparkQA]

Test build #24926 has finished for PR 3851 at commit 04c3e64.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class GaussianMixtureModel(
  • case class Sort(
  • case class BroadcastLeftSemiJoinHash(

[AmplabJenkins]

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

[JoshRosen]

Jenkins, retest this please.

[SparkQA]

Test build #24968 has started for PR 3851 at commit 04c3e64.

• This patch merges cleanly.

[SparkQA]

Test build #24968 has finished for PR 3851 at commit 04c3e64.

• 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/24968/
Test PASSed.

[JoshRosen]

Ping @mridulm, since it looks like you might have introduced the first version of this sleep call back in 2013: d90d2af#diff-4e188f32951dc989d97fa7577858bc7cR397

I don't see a good reason to keep this sleep; the tests pass without it and its removal seems to have given us a big test performance boost.

[JoshRosen]

@tdas, what do you think about merging this? It looks like you were the last one to touch this line in 27311b1. This fixed sleep() seems race-prone enough that I suppose we would have noticed if it was necessary for anything because it would have caused test flakiness.

[JoshRosen]

I'm inclined to merge this into master now and not perform any backports right away (maybe it's still serving some purpose in older branches?).

[tdas]

@JoshRosen I dont remember much of the context around this. From the commit comment, I think I had introduced it for testing out metadata cleaning in Spark 1.0, so that all messages gets sent if the scheduler and everything is shutdown. Off the top of my head, I am not sure if removing this would cause flakiness due to unsent messages. If tests pass without it without flakiness, I am good with it.

[JoshRosen]

Alright, I'm going to merge this into master (1.3.0); after I do that, let's wait a bit and see if it adds any flakiness. If this does serve some purpose, then we'll be able to implement a better solution that doesn't rely on fixed timeouts.