[SPARK-15176][Core] Add maxShares setting to Pools

[njwhite]

What changes were proposed in this pull request?

Help guarantee resource availablity by (hierarchically) limiting the amount of tasks a given pool can run. The maximum number of tasks for a given pool can be configured by the allocation XML file, and child pools are limited to at most the number of tasks of their parent.

How was this patch tested?

Unit tests run and new unit tests added for functionality.

[HyukjinKwon]

I believe this line exceeds 100 characters.

[njwhite]

@HyukjinKwon I've run run-tests and fixed all the style issues. Could you take another look? Thanks -

[HyukjinKwon]

@njwhite I am not a committer but just one of contributors. I guess most of codes were written by @kayousterhout in this part.

[ash211]

Ping, anything more needed on this PR before merging?

[rxin]

cc @kayousterhout for review

[kayousterhout]

I commented on the JIRA.

[squito]

Hi @njwhite, I'm not sure I see a strong need for this -- I posted a msg on jira (as Kay had earlier). We should keep discussion about the feature in general there, for archive / searchability.

In any case I did look at the code, so a couple of comments about that, if we do decide we want this feature. Unless I'm missing something, it doesn't seem like maxShare is being used anywhere (except for one trivial spot to check its > 0). I was expecting a change to FairSchedulingAlgorithm at least, though I think it might actually be a more complicated change to TaskSchedulerImpl that is required.

Also to go along with that, we'd want new test cases demonstrating how maxShare gets used.

[squito]

as long as you're touching this, switch to using string interpolation. (eg. s"Created default pool $DEFAULT_POOL_NAME, ...). Also since this is repeated a few times, you might just add a helper logPoolCreated(pool) or something.

[njwhite]

Thanks for the review @squito - I've commented on the JIRA about why this feature would be useful. As for the implementation - maybe "maxShare" is the wrong word, as the change doesn't relate to the fair scheduler at all. Instead it limits the maximum number of tasks a Schedulable can have running at any one time. It really is just a one line change - resourceOffer now won't accept any more resources (i.e. won't run any of its tasks) if the calculated current value of maxShares means there isn't any free space in the pool. The maxShares method just returns the maximum number of tasks allowed in the pool, minus the number of tasks running in the pool. You can see the propagation of the maxShares limit in the assertions I added to the PoolSuite test.

[squito]

ah, I completely overlooked Pool.maxShare referencing runningTasks, makes more sense now.

The added tests verify that maxShare is getting propagated correctly, and limited by the parent's max share. But there aren't any tests which show that scheduling is actually limited by it at all. We'd want tests to cover that. The code needs more comments explaining what is going on. I'd even add comments on the tests explaining what is being tested (eg., the line where the maxShare is limited by the parent pool).

Calling it "maxShare" is pretty confusing -- with this implementation it should probably be called "maxRunningTasks" or something. It also seems pretty hard to configure, though, I wonder if users really do want maxShare. We should be sure that whatever we add is what want long-term, so we're not stuck with complexity from a legacy setting.

honestly I am still uncertain about adding the feature, need to think about it more -- I'm just giving my comments on the code here. A very clean, well-tested PR can help make your case, but OTOH can also turn into wasted effort ...

[squito]

Jenkins, ok to test

[SparkQA]

Test build #59030 has finished for PR 12951 at commit c4082c5.

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

[njwhite]

Thanks @squito; I've renamed the setting to maxRunningTasks and added the tests you asked for to TaskSetManagerSuite. I've also added support (& tests) for configuring the parent pool in the XML config file, as that came in useful.

[SparkQA]

Test build #59497 has finished for PR 12951 at commit 122b122.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #59498 has finished for PR 12951 at commit e100683.

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

[SparkQA]

Test build #59501 has finished for PR 12951 at commit 0669b49.

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

[markhamstra]

Added my comments to the JIRA. In short, I think there is a legitimate use case for this, and there is a significant gap in our current fair-scheduling pool API. Implementing a maxShare property is actually something that has been on my todo list for awhile.

[njwhite]

Thanks @markhamstra! The Jenkins build failed because a single test, ExternalAppendOnlyMapSuite#spilling with compression, failed. It seems unrelated (and passes locally for me) - are there known issues with it?

[squito]

The test case should include scheduling another taskset to a different pool which does not share the limitation, and making sure it can still schedule tasks even when the first task set gets limited.

[njwhite]

@squito thanks - I've expanded the Scheduler respects maxRunningTasks setting of its pool test to cover the cases you mention (and a couple of others).

[SparkQA]

Test build #60036 has finished for PR 12951 at commit b8eaaef.

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

[njwhite]

@squito is this OK?

[kayousterhout]

Does it make sense to move this to Schedulable.scala? It looks like Pool and TaskSetManager both have the same implementation (assuming that Int.MAX_VALUE is the default).

[kayousterhout]

The naming on this PR is somewhat confusing, because it looks like maxShares is supposed to return the maximum number of remaining tasks that can be run, rather than the maximum number of tasks that can be running at a time. The current name implies the latter. Is it possible to use a more descriptive name for this? maxRemainingTasks? I don't have a great idea here but maybe others do?

[kayousterhout]

Also, once naming is settled on, this PR should include a documentation update to this page: https://spark.apache.org/docs/latest/job-scheduling.html to describe this.

[SparkQA]

Test build #61724 has finished for PR 12951 at commit 156f711.

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

[njwhite]

Hi @kayousterhout - I've renamed all references to maxRunningTasks and updated the Markdown documentation in the repo. Is this OK? Thanks -

[njwhite]

ping?

[kayousterhout]

@njwhite sorry to let this idle for so long. I just read through the comments here and on the JIRA and it looks like the consensus on the JIRA was that it would be better to implement maxShare instead of maxRunningTasks, because it's likely easier to configure, and also is less brittle to the cluster size. Can you implement that change?

Alternately if you think this should remain maxRunningTasks, comment on the JIRA and we can continue the discussion there.

[kayousterhout]

@njwhite do you have time to work on this and implement maxShares? If not, can you close the PR?

[njwhite]

@kayousterhout minShares is a configuration parameter for the fair scheduler algorithm only - what would the semantics of a maxShares setting for the FIFO algorithm be?

[njwhite]

Actually, @kayousterhout - I'm not entirely sure what you expect for the semantics of maxShares in general. Maybe a worked example would help: if I have a pool X with 5 running tasks from Taskset A and a maxShares of 7. Pool X is a child of pool Y which has a maxShares of 8. I want to the schedule another task from Taskset A, so should the scheduler allow it or not? Do you need to know how many executors are currently running (and so the maximum number of tasks that could be run)?

[jiangxb1987]

Should we process with this PR or should we close this? @kayousterhout @njwhite

[AmplabJenkins]

Can one of the admins verify this patch?