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

erenavsarogullari · 2017-02-12T22:20:48Z

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, vars 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 #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.

erenavsarogullari · 2017-02-12T22:24:22Z

cc @kayousterhout @markhamstra @squito

srowen

I wonder if @squito or @kayousterhout has thoughts on these tweaks? I do like cleaning up and tightening the internal impl if there isn't a downside.

srowen · 2017-02-13T11:10:22Z

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

This should be in a companion object, but really, maybe not worth it.

srowen · 2017-02-13T11:12:38Z

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

Maintaining these messages with the various options is error-prone. Can you just use its .values and print that?

We're not likely to add or remove SchedulingModes with any frequency, if at all, so this isn't likely to cause much opportunity for error -- but I agree with the principle that extracting from .values is a better approach.

SchedulingMode possible values are FIFO, FAIR and NONE but NONE is unsupported value. I agree to support for potential values and it can be achieved by adding following logic to SchedulingMode object and can be used required places(2 times here and 1 time at Pool)
def getSupportedValuesAsString(): String = values.filter(_ != NONE).mkString(", ") SchedulingMode.getSupportedValuesAsString() // returns FIFO, FAIR
WDYT?

NONE is only used in tests, and doesn't really need to be used there -- see the commit in my repo that I pointed you to earlier. Technically, removing NONE is a change in the public interface, but the docs say that only FAIR and FIFO are allowed (http://spark.apache.org/docs/latest/job-scheduling.html#configuring-pool-properties), and trying to use NONE will lead you to the "unsupported" failure, so we probably can get rid of it.

Yep, TaskSetManager also uses NONE to override schedulingMode (from parent Schedulable trait). However, it does not use schedulingMode. I think if NONE is removed, then FIFO will be used as the default value(e.g. TaskManager), right?

srowen · 2017-02-13T11:14:06Z

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

These appear to be pretty much redundant then? if they're just set once to another existing variable's value?

Pool extends Schedulable trait and needs to override weight. It is also used for taskToWeightRatio calculation at FairSchedulingAlgorithm level.

srowen · 2017-02-13T11:14:32Z

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

This is duplicated

markhamstra · 2017-02-13T16:26:05Z

@srowen These refactorings of unnecessary vars to vals is something that we've noted in the discussions of a few other PRs as something that could and probably should be done in a separate PR (i.e. this one).

@erenavsarogullari There is another such refactoring that can be rolled into this PR. See the changes that I've made to rootPool in TaskSchedulerImpl, DAGSchedulerSuite and ExternalClusterManagerSuite here: markhamstra@e11fe2a

markhamstra · 2017-02-13T16:30:51Z

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

I'd really rather not see this kind of change. Other than the missing string-interpolation s in msg, the prior code was at least as good (and arguably better) than the new style, and making such an inconsequential style change just adds complication to future investigations of the git history.

erenavsarogullari · 2017-02-14T23:56:30Z

Thanks @markhamstra and @srowen for the quick reviews.
I will address them asap by covering the additional patch suggested by Mark.

erenavsarogullari · 2017-02-16T00:53:09Z

Hi All,
All comments are addressed and this is ready for re-review.

@markhamstra, if it is suitable, i plan to address removing SchedulingMode.NONE via separated PR. In the light of this, the following new added unit test cases(using NONE) can be removed from this PR:

PoolSuite: Pool should throw IllegalArgumentException when schedulingMode is not supported
TaskSchedulerImplSuite: TaskScheduler should throw IllegalArgumentException when schedulingMode is not supported

WDYT?

erenavsarogullari · 2017-02-20T18:51:29Z

Hi @markhamstra,
Do you have any comment about this PR and my latest comment?
All feedbacks are welcome. Thanks in advance ;)

markhamstra · 2017-02-21T16:17:10Z

ok to test

erenavsarogullari · 2017-02-21T16:57:18Z

Thanks @markhamstra.
I think jenkins needs to be triggered.

markhamstra · 2017-02-21T17:13:32Z

If Jenkins is listening to me, that should have allowed you to trigger test for this PR.

test this please

erenavsarogullari · 2017-02-21T20:19:58Z

Thanks @markhamstra.

I encounter the same problem in my every PR and tech-ops team helps to trigger Jenkins. Problem may be related with Github Jenkins PR Builder plugin.

Also it still looks not triggered.

kayousterhout · 2017-02-21T20:30:41Z

Jenkins test this please

kayousterhout · 2017-02-21T20:32:23Z

@erenavsarogullari the reason this was happening on past PRs is that Imran and I weren't on the Jenkins whitelist (which neither of us realized -- and has since been fixed). I think Mark is on the whitelist so this is probably a transient issue... we'll see if my triggering works.

SparkQA · 2017-02-21T20:34:12Z

Test build #73232 has finished for PR 16905 at commit 589ef36.

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

kayousterhout · 2017-02-21T20:34:25Z

@shaneknapp is @markhamstra on the Jenkins whitelist? Wanted to double check since his attempt to trigger the build above didn't work

shaneknapp · 2017-02-21T20:38:06Z

nope, he's not in the admin list. i'll add him now.

kayousterhout · 2017-02-21T20:39:23Z

Awesome thanks Shane!

markhamstra · 2017-02-21T20:40:43Z

Thanks, Shane & Kay!

erenavsarogullari · 2017-02-21T21:17:32Z

Many thanks @kayousterhout and @markhamstra.
Also minor scala style issue is fixed with new patch.

kayousterhout · 2017-02-22T21:20:15Z

Jenkins this is OK to test

erenavsarogullari · 2017-02-22T22:38:12Z

Thanks @kayousterhout. However, it still looks not triggered.

vanzin · 2017-02-22T22:39:35Z

retest this please

SparkQA · 2017-02-23T01:09:37Z

Test build #73299 has finished for PR 16905 at commit a580a2f.

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

erenavsarogullari · 2017-03-12T20:56:33Z

Hi @markhamstra, @kayousterhout and @squito
This PR is ready for final review and merge if there is no concern.
All feedbacks are welcome. Thanks in advance ;)

squito

lgtm

kayousterhout · 2017-03-15T18:16:46Z

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

one quick comment here if Imran didn't already merge: can you un-do these changes to add private? It's not useful / necessary to make test classes private (they're already hidden by the build), and this change will make git blames more confusing in the future.

erenavsarogullari · 2017-03-15T22:14:51Z

Thanks @kayousterhout and @squito for review this ;)
Last comment is also addressed via last commit: 18657d4.

kayousterhout · 2017-03-15T22:15:59Z

Jenkins test this please

erenavsarogullari · 2017-03-15T23:54:13Z

Thanks again @kayousterhout and @squito.
Merge conflict has been fixed and this PR is ready to build on jenkins and merge (if there is no concern).

kayousterhout · 2017-03-15T23:56:28Z

Jenkins retest this please

SparkQA · 2017-03-16T00:16:45Z

Test build #74620 has finished for PR 16905 at commit 18657d4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FakeSchedulerBackend extends SchedulerBackend

SparkQA · 2017-03-16T02:05:11Z

Test build #74629 has finished for PR 16905 at commit 479c01d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FakeSchedulerBackend extends SchedulerBackend

squito · 2017-03-16T02:55:44Z

reopened https://issues.apache.org/jira/browse/SPARK-7420 for the failure

Jenkins, retest this please

erenavsarogullari · 2017-03-16T17:41:11Z

Thanks again @kayousterhout and @squito ;)
Jenkins needs to be retriggered.

squito · 2017-03-16T18:26:13Z

@shaneknapp I had to trigger jenkins manually via spark-prs. Every once in a while I encounter a pr for which tests are never triggered via comments. Its pretty rare, so its not a big deal, but I thought I'd point you at this in case you want to take a look.

SparkQA · 2017-03-16T18:53:42Z

Test build #3600 has finished for PR 16905 at commit 479c01d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FakeSchedulerBackend extends SchedulerBackend

SparkQA · 2017-03-16T21:23:02Z

Test build #3601 has finished for PR 16905 at commit 479c01d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FakeSchedulerBackend extends SchedulerBackend

erenavsarogullari · 2017-03-22T20:02:13Z

Hi @squito,

I think jenkins can be triggered again. Last unrelated build - UT issue looks fixed(https://issues.apache.org/jira/browse/SPARK-19988)

Thanks in advance ;)

kayousterhout · 2017-03-22T23:56:42Z

Jenkins, this is ok to test

kayousterhout · 2017-03-22T23:56:50Z

Jenkins test this please

kayousterhout · 2017-03-23T00:16:38Z

Jenkins add to whitelist

SparkQA · 2017-03-23T02:34:49Z

Test build #75070 has finished for PR 16905 at commit 479c01d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FakeSchedulerBackend extends SchedulerBackend

SparkQA · 2017-03-23T02:54:53Z

Test build #75072 has finished for PR 16905 at commit 479c01d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FakeSchedulerBackend extends SchedulerBackend

kayousterhout · 2017-03-24T00:21:14Z

LGTM merged into master

erenavsarogullari changed the title ~~[SPARK-19567] [CORE] [SCHEDULER] Support some Schedulable variables immutability and access~~ [SPARK-19567][CORE][SCHEDULER] Support some Schedulable variables immutability and access Feb 12, 2017

srowen requested changes Feb 13, 2017

View reviewed changes

markhamstra reviewed Feb 13, 2017

View reviewed changes

squito approved these changes Mar 15, 2017

View reviewed changes

kayousterhout reviewed Mar 15, 2017

View reviewed changes

Support some Schedulable variables immutability and access

7bc511f

erenavsarogullari added 4 commits March 15, 2017 23:42

Review comments are addressed.

7f4efec

The patch suggested by markhamstra is also added.

fc7f00c

Scala style issue is fixed.

da702d6

Review comment is addressed.

479c01d

asfgit closed this in b7be05a Mar 24, 2017

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

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

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

Uh oh!

Conversation

erenavsarogullari commented Feb 12, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

erenavsarogullari commented Feb 12, 2017

Uh oh!

srowen 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!

Choose a reason for hiding this comment

Uh oh!

markhamstra commented Feb 13, 2017

Uh oh!

markhamstra Feb 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erenavsarogullari commented Feb 14, 2017

Uh oh!

erenavsarogullari commented Feb 16, 2017

Uh oh!

erenavsarogullari commented Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markhamstra commented Feb 21, 2017

Uh oh!

erenavsarogullari commented Feb 21, 2017

Uh oh!

markhamstra commented Feb 21, 2017

Uh oh!

erenavsarogullari commented Feb 21, 2017

Uh oh!

kayousterhout commented Feb 21, 2017

Uh oh!

kayousterhout commented Feb 21, 2017

Uh oh!

SparkQA commented Feb 21, 2017

Uh oh!

kayousterhout commented Feb 21, 2017

Uh oh!

shaneknapp commented Feb 21, 2017

Uh oh!

kayousterhout commented Feb 21, 2017

Uh oh!

markhamstra commented Feb 21, 2017

Uh oh!

erenavsarogullari commented Feb 21, 2017

Uh oh!

kayousterhout commented Feb 22, 2017

Uh oh!

erenavsarogullari commented Feb 22, 2017

Uh oh!

vanzin commented Feb 22, 2017

Uh oh!

SparkQA commented Feb 23, 2017

Uh oh!

erenavsarogullari commented Mar 12, 2017

Uh oh!

squito left a comment

Choose a reason for hiding this comment

markhamstra Feb 13, 2017 •

edited

Loading

erenavsarogullari commented Feb 20, 2017 •

edited

Loading

kayousterhout Mar 15, 2017 •

edited

Loading