[SPARK-18066] [CORE] [TESTS] Add Pool usage policies test coverage for FIFO & FAIR Schedulers #15604

erenavsarogullari · 2016-10-23T21:04:31Z

What changes were proposed in this pull request?

The following FIFO & FAIR Schedulers Pool usage cases need to have unit test coverage :

FIFO Scheduler just uses root pool so even if spark.scheduler.pool property is set, related pool is not created and TaskSetManagers are added to root pool.
FAIR Scheduler uses default pool when spark.scheduler.pool property is not set. This can be happened when
- Properties object is null,
- Properties object is empty(new Properties()),
- default pool is set(spark.scheduler.pool=default).
FAIR Scheduler creates a new pool with default values when spark.scheduler.pool property points a non-existent pool. This can be happened when scheduler allocation file is not set or it does not contain related pool.

How was this patch tested?

New Unit tests are added.

erenavsarogullari · 2016-10-23T21:07:06Z

cc @kayousterhout @markhamstra

erenavsarogullari · 2016-12-24T20:46:29Z

Hi @kayousterhout @markhamstra @squito,
This PR aims to extend Pool usage policies unit test coverage and is also ready for review.
Thanks and all feedbacks are welcome in advance ;)

squito · 2017-01-03T22:19:26Z

Jenkins, ok to test

squito

Thanks for this @erenavsarogullari , great to have more tests. I have super trivial suggested changes, but one bigger question for other committers I'd like to wait a bit to hear back on.

squito · 2017-01-03T22:26:55Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

I'm all for more tests, but this final case here seems pretty unnecessary

squito · 2017-01-03T22:39:30Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

minor nit: rename to "FAIR Scheduler creates a new pool when spark.scheduler.pool property points to a non-existent pool"

bigger question:
@markhamstra @kayousterhout Is this really the desired behavior? Or is there a bug -- should it fail fast? It looks like this behavior was intentional. Then again, that comment makes it sound like the user is going to directly modify the weight and minShare of the constructed pool, but though they are vars, its private [spark] and there isn't anything else exposing it to the user.

I prefer fail-fast behavior, but it seems like fair scheduler configuration prefers to assume defaults when there is misconfiguration. If you think this the right behavior, than there is probably some minor cleanup to do in Pool & SchedulableBuilder to clarify.

I tracked this down and looks like it was changed back in 2013. @xiajunluan do you remember the motivation for this?

I'm torn because in general I prefer failing fast (e.g., this is better in the case where someone has a small typo they might not notice, and is intending to use a configured pool). On the other hand, I'm pretty sure I've abused this behavior in the past to avoid configuring pools when I want a bunch of equally weighted pools. If others are doing that, we shouldn't change the behavior (although I think that, in that case, we should add a big warning / future-deprecation message).

yeah I prefer failing fast for the same reason. But I guess we can't actually change this, since there is a valid use case for this behavior. We don't really know if others are using this, and I don't think we should change it under them in the next release.

In any case, I still think we should also change Pool.weight and Pool.minShare to vals.

In this case, at least, i think we can inform the user by emphasizing that the related pool does not exist and it is created with default settings.
Our current logging just points pool creation. WDYT?

Also Pool var weight / minShare have been addressed ;)

squito · 2017-01-03T22:41:25Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

super nit: the jira doesn't really say anything other than "add these tests", so not much point in listing it here, just name test "FIFO Scheduler just uses root pool".

+1. Also how about "FIFO scheduler uses root pool and not spark.scheduler.pool" and then adding a comment at the top of the test that says something like "spark.scheduler.pool should be ignored for the FIFO scheduler, because pools are only needed for fair scheduling"? Took me a minute to figure out what was going on here.

squito · 2017-01-03T23:15:48Z

hmm, so I'm thinking about this again, along with your other pr #15237

and it got me thinking -- there also aren't any tests for a mix of FIFO and FAIR (at least, I don't see it in "Nested Pool Test" in PoolSuite). While you are doing this, would you like to add some tests for that as well? (to be perfectly honest, I need to spend a little bit of time thinking about what that behavior should be anyway, but I will have to do that a little later.)

kayousterhout

Thanks for adding these tests!

kayousterhout · 2017-01-03T23:11:33Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

+1. Also how about "FIFO scheduler uses root pool and not spark.scheduler.pool" and then adding a comment at the top of the test that says something like "spark.scheduler.pool should be ignored for the FIFO scheduler, because pools are only needed for fair scheduling"? Took me a minute to figure out what was going on here.

kayousterhout · 2017-01-03T23:13:30Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

Can you re-phrase this as what should happen, which makes these comments easier to read? e.g.

"When task sets are submitted, they should be added to the root pool, and no additional pools should be created (even though there's a configured default pool)."

kayousterhout · 2017-01-03T23:13:53Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

nit: +2 spaces here

kayousterhout · 2017-01-03T23:16:21Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

super nit with commenting, but can you eliminate the blank lines on 219 and 221 (which don't help readability) and change this comment to something like

"Submit a new task set manager with pool properties set to null. This should result in the task set manager getting added to the default pool."

(it's helpful in test comments to be clear about what the test is doing itself, versus the expected behavior it's verifying)

kayousterhout · 2017-01-03T23:17:42Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

Similar to the above, can you eliminate the blank lines here, and change this to "When a task set manager is submitted with spark.scheduler.pool unset, it should be added to the default pool (as above)."

kayousterhout · 2017-01-03T23:18:04Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

kayousterhout · 2017-01-03T23:26:05Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

I tracked this down and looks like it was changed back in 2013. @xiajunluan do you remember the motivation for this?

I'm torn because in general I prefer failing fast (e.g., this is better in the case where someone has a small typo they might not notice, and is intending to use a configured pool). On the other hand, I'm pretty sure I've abused this behavior in the past to avoid configuring pools when I want a bunch of equally weighted pools. If others are doing that, we shouldn't change the behavior (although I think that, in that case, we should add a big warning / future-deprecation message).

kayousterhout · 2017-01-03T23:28:14Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

can you clean up this comment a bit too?

"The fair scheduler should create a new pool with default values when spark.scheduler.pool points to a pool that doesn't exist yet (this can happen when the file that pools are read from isn't set, or when that file doesn't contain the pool name specified by spark.scheduler.pool)."

SparkQA · 2017-01-04T00:49:57Z

Test build #70832 has finished for PR 15604 at commit f630cff.

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

kayousterhout · 2017-02-07T00:16:25Z

@erenavsarogullari what's the status with this PR? I know you have at least one other fair scheduler PR outstanding, and it makes reviews easier to minimize the number of outstanding things (and helps avoid merge conflicts).

SparkQA · 2017-02-07T23:10:23Z

Test build #72544 has finished for PR 15604 at commit f1cfc9e.

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

erenavsarogullari · 2017-02-08T23:05:27Z

@kayousterhout thanks for query this PR. I will be submitting this and the other one( #15326) once #16813 is merged.

kayousterhout · 2017-02-08T23:09:17Z

Ok thanks for the update!

SparkQA · 2017-02-10T23:24:19Z

Test build #72722 has finished for PR 15604 at commit 84513b1.

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

erenavsarogullari · 2017-02-10T23:34:37Z

Jenkins, retest this please

erenavsarogullari · 2017-02-10T23:43:39Z

Hi @kayousterhout and @squito,
Firstly, many thanks for the review. All comments are addressed. This is ready to re-review ;)

SparkQA · 2017-02-11T01:43:26Z

Test build #72725 has finished for PR 15604 at commit f84abe7.

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

erenavsarogullari · 2017-02-11T01:51:20Z

build failure looks unrelated (due to timeout at KafkaSourceSuite level)

Jenkins, retest this please

erenavsarogullari · 2017-02-11T10:51:51Z

Jenkins, retest this please

SparkQA · 2017-02-11T13:49:55Z

Test build #72736 has finished for PR 15604 at commit 9eafad5.

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

SparkQA · 2017-02-13T01:12:02Z

Test build #72792 has finished for PR 15604 at commit d68bb0c.

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

erenavsarogullari · 2017-02-16T23:37:51Z

Hi @kayousterhout and @squito,
This PR is ready to re-review / merge if it is possible.
All feedbacks are welcome and thanks in advance ;)

erenavsarogullari · 2017-02-20T22:38:34Z

Hi @kayousterhout and @squito,
This PR is ready to re-review.
All feedbacks are welcome and thanks in advance ;)

kayousterhout

One last small commeent

kayousterhout · 2017-02-23T21:59:02Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

It's not necessary to make test functions private -- they're not accessible anyway (and we don't do this elsewhere in tests)

kayousterhout · 2017-02-23T22:05:19Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

JOOC does this result in a warning being printed to the user? (I know you can't test for that -- but it does seem like useful behavior to have)

Currently, this behavior is not highlighted via logs and we just points the pool creation as
Created pool: ..., schedulingMode: ..., minShare: ..., weight: ... so i agree it can be useful for the user and it is addressed with the latest commits.

Thanks for updating this!

erenavsarogullari · 2017-03-12T20:18:04Z

Hi @kayousterhout and @squito,
Last comments are addressed so this PR is ready to re-review / merge if it is possible.
Thanks in advance ;)

SparkQA · 2017-03-12T20:52:33Z

Test build #74408 has finished for PR 15604 at commit 4793ea8.

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

SparkQA · 2017-03-12T21:28:18Z

Test build #74410 has finished for PR 15604 at commit e9bb2b8.

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

SparkQA · 2017-03-12T22:13:00Z

Test build #74412 has finished for PR 15604 at commit 91f3814.

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

SparkQA · 2017-03-12T22:18:58Z

Test build #74413 has finished for PR 15604 at commit 17e11f0.

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

erenavsarogullari · 2017-03-13T16:49:28Z

The following unit tests look failed due to timeout and irrelevant so triggered again.

1- org.apache.spark.storage.BlockManagerProactiveReplicationSuite.proactive block replication - 5 replicas - 4 block manager deletions

Error Message: org.scalatest.exceptions.TestFailedDueToTimeoutException: The code passed to eventually never returned normally. Attempted 493 times over 5.007521253999999 seconds. Last failure message: 4 did not equal 5.

2- org.apache.spark.deploy.SparkSubmitSuite.includes jars passed in through --packages

Error Message: org.scalatest.exceptions.TestFailedDueToTimeoutException: The code passed to failAfter did not complete within 60 seconds.

erenavsarogullari · 2017-03-13T16:49:58Z

Jenkins, retest this please

kayousterhout · 2017-03-15T00:59:17Z

@erenavsarogullari please file a JIRA when you see test failures instead of ignoring them. I updated https://issues.apache.org/jira/browse/SPARK-19803 for the first failure, but please file a JIRA for the second one.

kayousterhout · 2017-03-15T00:59:31Z

Jenkins retest this please

SparkQA · 2017-03-15T03:40:39Z

Test build #74575 has finished for PR 15604 at commit 17e11f0.

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

kayousterhout

This looks great -- one last comment update and then I can merge this!

kayousterhout · 2017-03-15T17:08:52Z

core/src/main/scala/org/apache/spark/scheduler/SchedulableBuilder.scala

It's not totally clear what the first part of this error message means. How about changing the message to:

logWarning(s"A job was submitted with scheduler pool $poolName, which has not been configured." + "This can happen when the file that pools are read from isn't set, or when that file " + s"doesn't contain $poolName. Created $poolName with default configuration (" + s"schedulingMode: $DEFAULT_SCHEDULING_MODE, minShare: $DEFAULT_MINIMUM_SHARE, " + s"weight: $DEFAULT_WEIGHT)")

Sure, it is updated ;)

kayousterhout · 2017-03-15T17:15:06Z

core/src/test/scala/org/apache/spark/scheduler/PoolSuite.scala

Thanks for updating this!

erenavsarogullari · 2017-03-15T20:55:56Z

Many thanks @kayousterhout for reviewing this again. Last comment has also been addressed.

Also the following Jira has been created for previous second UT failure.
https://issues.apache.org/jira/browse/SPARK-19964

SparkQA · 2017-03-15T22:42:24Z

Test build #74616 has finished for PR 15604 at commit f13ad3d.

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

kayousterhout · 2017-03-15T23:00:06Z

Thanks for your work on this @erenavsarogullari. I've merged this into master.

SparkQA · 2017-03-15T23:31:00Z

Test build #74618 has finished for PR 15604 at commit b7f2629.

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

…utability and access ## What changes were proposed in this pull request? Some `Schedulable` Entities(`Pool` and `TaskSetManager`) variables need refactoring for _immutability_ and _access modifiers_ levels as follows: - From `var` to `val` (if there is no requirement): This is important to support immutability as much as possible. - Sample => `Pool`: `weight`, `minShare`, `priority`, `name` and `taskSetSchedulingAlgorithm`. - Access modifiers: Specially, `var`s access needs to be restricted from other parts of codebase to prevent potential side effects. - `TaskSetManager`: `tasksSuccessful`, `totalResultSize`, `calculatedTasks` etc... This PR is related with apache#15604 and has been created seperatedly to keep patch content as isolated and to help the reviewers. ## How was this patch tested? Added new UTs and existing UT coverage. Author: erenavsarogullari <[email protected]> Closes apache#16905 from erenavsarogullari/SPARK-19567.

erenavsarogullari changed the title ~~Add Pool usage policies test coverage for FIFO & FAIR Schedulers~~ [SPARK-18066] [CORE] [TESTS] Add Pool usage policies test coverage for FIFO & FAIR Schedulers Oct 23, 2016

squito reviewed Jan 3, 2017

View reviewed changes

kayousterhout reviewed Jan 3, 2017

View reviewed changes

erenavsarogullari mentioned this pull request Feb 12, 2017

[SPARK-19567][CORE][SCHEDULER] Support some Schedulable variables immutability and access #16905

Closed

kayousterhout suggested changes Feb 23, 2017

View reviewed changes

kayousterhout reviewed Mar 15, 2017

View reviewed changes

erenavsarogullari added 10 commits March 15, 2017 19:56

Add Pool usage policies test coverage for FIFO & FAIR Schedulers

0d5b025

Minor refactoring for previous commit

fd9e3f3

Review comments are addressed.

be133c1

Minor style is fixed.

ad16e53

Minor redundant check is removed.

e292923

Pool variables are addressed via separated PR

fc3deea

Review comment is addressed.

e2d2e59

Add additional logs to highlight pool creation to the user.

63ee0c8

Logging level is updated from info to warning.

f13ad3d

Warning message is updated.

b7f2629

asfgit closed this in 046b8d4 Mar 15, 2017

erenavsarogullari deleted the SPARK-18066 branch March 25, 2017 10:59

[SPARK-18066] [CORE] [TESTS] Add Pool usage policies test coverage for FIFO & FAIR Schedulers #15604

[SPARK-18066] [CORE] [TESTS] Add Pool usage policies test coverage for FIFO & FAIR Schedulers #15604

Uh oh!

Conversation

erenavsarogullari commented Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

erenavsarogullari commented Oct 23, 2016

Uh oh!

erenavsarogullari commented Dec 24, 2016

Uh oh!

squito commented Jan 3, 2017

Uh oh!

squito left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squito commented Jan 3, 2017

Uh oh!

kayousterhout left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 4, 2017

Uh oh!

kayousterhout commented Feb 7, 2017

Uh oh!

SparkQA commented Feb 7, 2017

Uh oh!

erenavsarogullari commented Feb 8, 2017

Uh oh!

kayousterhout commented Feb 8, 2017

Uh oh!

SparkQA commented Feb 10, 2017

Uh oh!

erenavsarogullari commented Feb 10, 2017

Uh oh!

erenavsarogullari commented Feb 10, 2017

Uh oh!

SparkQA commented Feb 11, 2017

Uh oh!

erenavsarogullari commented Feb 11, 2017

Uh oh!

erenavsarogullari commented Feb 11, 2017

Uh oh!

SparkQA commented Feb 11, 2017

Uh oh!

SparkQA commented Feb 13, 2017

erenavsarogullari commented Oct 23, 2016 •

edited

Loading