[SPARK-5337][Standalone] respect spark.task.cpus when scheduling Applications #8610

CodingCat · 2015-09-05T06:00:03Z

https://issues.apache.org/jira/browse/SPARK-5337

Currently, we didn't consider spark.task.cpus when scheduling the applications in Master, so that we may fall into one of the following cases

the executor gets N cores but we need M cores to run a single task, where N < M

the executor gets N cores, we need M cores to run a single task, where N % M != 0 && N > M; so that we waste some cores in the executor

Patch for YARN is in submitted by @WangTaoTheTonic : #4123

SparkQA · 2015-09-05T07:32:00Z

Test build #42037 has finished for PR 8610 at commit d3289fc.

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

CodingCat · 2015-09-05T12:06:30Z

retest this please

CodingCat · 2015-09-05T12:07:13Z

Jenkins, retest this please.

CodingCat · 2015-09-05T12:08:03Z

....first time to know I have the permission to retest stuffs..............

SparkQA · 2015-09-05T14:42:59Z

Test build #42055 has finished for PR 8610 at commit d3289fc.

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

SparkQA · 2015-09-05T14:49:55Z

Test build #42054 has finished for PR 8610 at commit d3289fc.

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

SparkQA · 2015-09-05T19:11:50Z

Test build #42059 has finished for PR 8610 at commit 9c16dc7.

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

SparkQA · 2015-09-05T22:48:15Z

Test build #42060 has finished for PR 8610 at commit 0871c37.

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

CodingCat · 2015-09-07T13:05:49Z

Hi, @andrewor14 , I just added some test cases here, would you mind taking the review?

SparkQA · 2015-09-07T16:04:31Z

Test build #42094 has finished for PR 8610 at commit 44c6a03.

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

holdenk · 2015-09-08T21:37:17Z

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

I don't think this error message is super clear, what is meant by "no less than and folds of spark.task.cups"?

@holdenk how about now?

clearer, thanks :)

SparkQA · 2015-09-09T20:24:12Z

Test build #42211 has finished for PR 8610 at commit 169141a.

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

CodingCat · 2015-09-09T20:26:16Z

Jenkins, retest this please

SparkQA · 2015-09-09T22:50:05Z

Test build #42217 has finished for PR 8610 at commit 169141a.

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

CodingCat · 2015-09-10T01:55:58Z

Jenkins, retest this please

CodingCat · 2015-09-10T02:01:19Z

Jenkins, retest this please

SparkQA · 2015-09-10T04:54:54Z

Test build #42229 has finished for PR 8610 at commit 169141a.

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

CodingCat · 2015-10-30T11:55:48Z

@andrewor14 would you have some chance to review this?

dragos · 2015-11-21T22:41:34Z

core/src/main/scala/org/apache/spark/deploy/master/Master.scala

nitpick: coreNumPerTask sounds weird. coresPerTaks is what you've used everywhere else, and the name above is coresPerExecutor.

SparkQA · 2015-11-23T16:55:17Z

Test build #46536 has finished for PR 8610 at commit f27f5d8.

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

CodingCat · 2015-11-23T16:58:45Z

Jenkins, retest this please

CodingCat · 2015-11-23T18:13:38Z

Jenkins, retest this please

CodingCat · 2015-11-23T19:22:09Z

Hi, @dragos , thanks for the comments, just sync the patch with master and addressed your comments

@andrewor14 any plan to fix this in the coming 1.6?

andrewor14 · 2015-11-23T19:43:50Z

The 1.6 preview is already cut. This will have to come later. Until then, it would be good for @tnachen and @dragos to have a look.

CodingCat · 2015-11-23T20:23:53Z

OK, thx

One more question, am I supposed to have the permission to trigger Jenkins to retest the patch, I thought I was once able to do that...or it is due to the weird status of the bot?

CodingCat · 2015-11-23T20:24:46Z

Jenkins, retest this please

CodingCat · 2015-11-23T20:28:38Z

nvm, the bot fixed the relationship with me .....

tnachen · 2015-11-23T20:32:03Z

core/src/main/scala/org/apache/spark/deploy/master/Master.scala

IMO it's not clear what the relationship between coresPerTask and coresPerExecutor. Looking at the scheduling requirement we just need the maximum of the set to be able to schedule, and I thought cores per task is additional cpu resources on top of the executor.
Can we perhaps comment where this is introduced what these two are? Or point to documentation?

SparkQA · 2015-11-23T22:18:39Z

Test build #46549 has finished for PR 8610 at commit f27f5d8.

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

dragos · 2015-11-25T12:47:38Z

I'll have a look at it tomorrow, thanks for pinging me.

andrewor14 · 2016-02-02T00:04:48Z

core/src/main/scala/org/apache/spark/deploy/ApplicationDescription.scala

don't remove this...

andrewor14 · 2016-02-02T00:05:46Z

By the way, I think it's totally fine to just do this for standalone mode first, since Mesos doesn't yet read spark.executor.cores. We can fix it for Mesos later.

SparkQA · 2016-02-04T01:17:00Z

Test build #50715 has finished for PR 8610 at commit db57a2e.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

CodingCat · 2016-02-04T01:26:39Z

@andrewor14 thanks for reviewing this, I'm currently in traveling, I will move #4123 and the check here to SparkSubmit later (tomorrow)

I think we need to check whether CoresPerExecutor is no less than CoresPerTask in SparkSubmit only when the user has an explicit configuration on coresPerExecutor

The reason is that when the user does not explicitly specify coresPerExecutor (the worker will start an executor whenever there is a free core), there are still many chances that executors has more cores than coresPerTask...if we simply throw exceptions to the user in this case, the user has to set an explicit value of coresPerExecutor whenever they set a value on coresPerTask which is not an ideal situtation in terms of user-friendly, your thoughts?

SparkQA · 2016-02-04T01:30:09Z

Test build #50719 has finished for PR 8610 at commit ae3e5e4.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

CodingCat · 2016-02-04T07:19:29Z

Jenkins, retest it please

CodingCat · 2016-02-04T07:45:49Z

Jenkins, retest this please

SparkQA · 2016-02-04T09:34:09Z

Test build #50743 has finished for PR 8610 at commit e50e705.

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

SparkQA · 2016-02-05T14:47:16Z

Test build #50817 has finished for PR 8610 at commit 8286e97.

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

CodingCat · 2016-02-06T00:00:42Z

Hi, @andrewor14, when I had a second think about your suggestion that we shall put parameter checking in SparkSubmit, I found that it may not be the right way to do this

Because the users are always able to set the parameters by code, instead of through spark-submit. Reading the parameters set in a programmable way happens after reading the parameters set through spark-submit. In this case, we have to check whether the parameters are making sense right before we use them to create the application descriptor, e.g. ApplicationDescription in standalone mode. Otherwise, we may miss the parameters which are set after spark-submit parameter reading...

SparkQA · 2016-02-06T00:35:17Z

Test build #50847 has finished for PR 8610 at commit d644654.

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

CodingCat · 2016-02-06T01:10:55Z

Jenkins, retest this please

SparkQA · 2016-02-06T03:18:17Z

Test build #50853 has finished for PR 8610 at commit d644654.

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

rxin · 2016-06-15T22:32:35Z

Thanks for the pull request. I'm going through a list of pull requests to cut them down since the sheer number is breaking some of the tooling we have. Due to lack of activity on this pull request, I'm going to push a commit to close it. Feel free to reopen it or create a new one. We can also continue the discussion on the JIRA ticket.

holdenk reviewed Sep 8, 2015
View reviewed changes

dragos reviewed Nov 21, 2015
View reviewed changes

tnachen reviewed Nov 23, 2015
View reviewed changes

andrewor14 reviewed Feb 2, 2016
View reviewed changes

core/src/main/scala/org/apache/spark/deploy/ApplicationDescription.scala Outdated

Copy link

Contributor

andrewor14 Feb 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't remove this...

CodingCat force-pushed the SPARK-5337-1 branch from 8232a80 to 0526706 Compare February 4, 2016 00:42

CodingCat changed the title ~~[SPARK-5337][Mesos][Standalone] respect spark.task.cpus when scheduling Applications~~ [SPARK-5337][Standalone] respect spark.task.cpus when scheduling Applications Feb 4, 2016

CodingCat added 11 commits February 5, 2016 20:52

respect spark.task.cpus when scheduling Applications

2cea799

fix compilation error

65024bb

fix Mesos issue by assigning CPUS_PER_TASK a valid value

47b3ee7

add test cases

0179645

clearer comments

486c9b3

rename var

1a307e3

add comments about Spark-5337

7c4a0ab

stylistic fix

b391a96

do not start executors with changed perCore Configuration

55c2802

compilation fix

9020737

distinguish for oneExecutorPerWorker and multiExecutorsPerWorker

8286e97

CodingCat force-pushed the SPARK-5337-1 branch from 2b3fef5 to 8286e97 Compare February 5, 2016 12:53

recover initialExecutorLimit

d644654

asfgit closed this in 1a33f2e Jun 15, 2016

[SPARK-5337][Standalone] respect spark.task.cpus when scheduling Applications #8610

[SPARK-5337][Standalone] respect spark.task.cpus when scheduling Applications #8610

Uh oh!

Conversation

CodingCat commented Sep 5, 2015

Uh oh!

SparkQA commented Sep 5, 2015

Uh oh!

CodingCat commented Sep 5, 2015

Uh oh!

CodingCat commented Sep 5, 2015

Uh oh!

CodingCat commented Sep 5, 2015

Uh oh!

SparkQA commented Sep 5, 2015

Uh oh!

SparkQA commented Sep 5, 2015

Uh oh!

SparkQA commented Sep 5, 2015

Uh oh!

SparkQA commented Sep 5, 2015

Uh oh!

CodingCat commented Sep 7, 2015

Uh oh!

SparkQA commented Sep 7, 2015

Uh oh!

holdenk Sep 8, 2015

Choose a reason for hiding this comment

Uh oh!

CodingCat Sep 9, 2015

Choose a reason for hiding this comment

Uh oh!

holdenk Oct 30, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 9, 2015

Uh oh!

CodingCat commented Sep 9, 2015

Uh oh!

SparkQA commented Sep 9, 2015

Uh oh!

CodingCat commented Sep 10, 2015

Uh oh!

CodingCat commented Sep 10, 2015

Uh oh!

SparkQA commented Sep 10, 2015

Uh oh!

CodingCat commented Oct 30, 2015

Uh oh!

dragos Nov 21, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 23, 2015

Uh oh!

CodingCat commented Nov 23, 2015

Uh oh!

CodingCat commented Nov 23, 2015

Uh oh!

CodingCat commented Nov 23, 2015

Uh oh!

andrewor14 commented Nov 23, 2015

Uh oh!

CodingCat commented Nov 23, 2015

Uh oh!

CodingCat commented Nov 23, 2015

Uh oh!

CodingCat commented Nov 23, 2015

Uh oh!

tnachen Nov 23, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 23, 2015

Uh oh!

dragos commented Nov 25, 2015

Uh oh!

andrewor14 Feb 2, 2016

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Feb 2, 2016

Uh oh!