[SPARK-12994][CORE] It is not necessary to create ExecutorAllocationM… #10914

zjffdu · 2016-01-26T05:03:03Z

…anager in local mode

jerryshao · 2016-01-26T05:18:18Z

core/src/main/scala/org/apache/spark/SparkContext.scala

Remove this empty line.

zjffdu · 2016-01-26T06:22:44Z

Thanks @jerryshao

SparkQA · 2016-01-26T06:48:05Z

Test build #50069 has finished for PR 10914 at commit 90118ca.

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

SparkQA · 2016-01-26T07:47:27Z

Test build #50073 has finished for PR 10914 at commit 0467617.

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

jerryshao · 2016-01-26T07:52:26Z

All the DEA related unit tests are running on local mode, they will be failed with this change, we should fix it.

SparkQA · 2016-01-26T09:52:46Z

Test build #50085 has finished for PR 10914 at commit 69d6086.

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

JoshRosen · 2016-01-26T22:28:36Z

Just curious, what's the compelling reason for not creating this in local mode? Performance?

zjffdu · 2016-01-27T00:16:19Z

@JoshRosen No compelling reason for that. Just saw the following message in pyspark which is a little confusing as Dynamic executor allocation is not supported in local mode.

16/01/27 07:57:56 WARN spark.ExecutorAllocationManager: Unable to reach the cluster manager to kill executor driver,or no executor eligible to kill!

JoshRosen · 2016-01-27T01:38:00Z

Ping @andrewor14; this one-line change should be quick to review I think.

andrewor14 · 2016-02-01T21:55:19Z

core/src/main/scala/org/apache/spark/SparkContext.scala

this is hard to read. Why not just make the change in Utils.isDynamicAllocationEnabled so all usages of it will see this change as well?

Make sense. Update the PR to move it into Utils.isDynamicAllocationEnabled and have some refactoring accordingly.

andrewor14 · 2016-02-01T21:56:22Z

I agree with the change, but I think it could be made in a better place.

zjffdu · 2016-02-02T06:20:00Z

@andrewor14 Make sense to move it into Utils.isDynamicAllocationEnabled (add isLocal as another parameter, set isLocal as false explicitly in yarn related classes), update the patch and have some refactoring accordingly.

jerryshao · 2016-02-02T06:36:54Z

core/src/main/scala/org/apache/spark/util/Utils.scala

Can we add a default value isLocal: Boolean = false to avoid changing several other parts?

I thought about it, but feel it would be better to explicitly set it. Because I feel it doesn't make sense to assume that the default mode is non local.

actually it doesn't look very useful to pass in a flag that always decides the value of the return type. It appears that SparkContext is the only place where we need to read the flag so let's leave it out of this method.

SparkQA · 2016-02-02T08:05:33Z

Test build #50543 has finished for PR 10914 at commit 555a149.

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

SparkQA · 2016-02-02T10:10:14Z

Test build #50562 has finished for PR 10914 at commit fc0a8dd.

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

zjffdu · 2016-02-02T10:24:40Z

Seems the failed tests are unrelated.

andrewor14 · 2016-02-02T23:46:51Z

core/src/main/scala/org/apache/spark/SparkContext.scala

this warning doesn't belong here anymore. You need to move it to Utils too

andrewor14 · 2016-02-02T23:51:43Z

@zjffdu the latest changes look more complicated than before. I think it would be best to achieve the following:

all checks involving the conf should be in Utils.dynamicAllocationEnabled
on second thought the isLocal check should just be in SparkContext; it doesn't really make sense for it to be everywhere like the way it is currently in this patch

Ideally we'd have something like:

// In SparkContext
val dynamicAllocationEnabled = Utils.dynamicAllocationEnabled(_conf)

val executorAllocationManager =
  if (dynamicAllocationEnabled && !isLocal) {
    Some(...)
  } else {
    None
  }

Everything else can go into Utils.dynamicAllocationEnabled. Does that make sense?

zjffdu · 2016-02-03T02:06:12Z

@andrewor14 I add one method Utils.isLocal, and keep the signature of Utils.dynamicAllocationEnabled unchanged and put all the logic in Utils.dynamicAllocationEnabled

SparkQA · 2016-02-03T04:19:35Z

Test build #50633 has finished for PR 10914 at commit 8261c14.

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

srowen · 2016-02-07T12:47:26Z

core/src/test/scala/org/apache/spark/util/UtilsSuite.scala

asserting === true is redundant

JoshRosen · 2016-02-16T16:32:01Z

Hey, any chance that we can update this patch and get it merged? This change seems conceptually simple and I can't imagine that it'll be too much more work to get it across the finish line.

srowen · 2016-02-18T11:04:30Z

Ping @zjffdu ?

srowen · 2016-02-23T12:25:57Z

@zjffdu do you mind closing this PR if you're not going to update it?

zjffdu · 2016-02-23T14:13:19Z

Sorry for late response, patch updated.

srowen · 2016-02-23T14:18:11Z

core/src/main/scala/org/apache/spark/SparkContext.scala

This def is redundant now

Keep using def, because val will cause test fails (val will be evaluated when declared while def will be evaluated when invoked ) Although I can use config instead of _conf, but considering all the variables around isLocal is using _conf, I don't want to make it inconsistent and involve any potential issue.

srowen · 2016-02-23T14:18:35Z

core/src/main/scala/org/apache/spark/util/Utils.scala

This needs a better name and description. It says whether the master is local. Use @return

Where did you feel unclear ? I think it is straightforward. And I think @return is only mandatory when it is public api.

It is just called "isLocal" and is a function of SparkConf. What is local? why be opaque -- "isLocalMaster"? hm, does it even belong as a helper in SparkConf? (I don't feel strongly about that.)

You're already writing "Returns .."; why not just use the actual scaladoc tag?

SparkQA · 2016-02-23T16:33:25Z

Test build #51770 has finished for PR 10914 at commit e3c0f9a.

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

SparkQA · 2016-02-24T02:29:37Z

Test build #51831 has finished for PR 10914 at commit c15212b.

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

…anager in local mode

SparkQA · 2016-02-24T05:11:35Z

Test build #51838 has finished for PR 10914 at commit bffa7a4.

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

srowen · 2016-02-26T19:17:53Z

Jenkins, retest this please

SparkQA · 2016-02-26T21:51:35Z

Test build #52071 has finished for PR 10914 at commit bffa7a4.

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

srowen · 2016-02-27T13:58:15Z

core/src/main/scala/org/apache/spark/util/Utils.scala

+
+  /**
+   *
+   * @return whether it is local mode


No need to change this, but you have an extra blank line here

srowen · 2016-02-27T14:00:52Z

isLocalMaster could be private, but that's fairly minor

srowen · 2016-02-29T12:08:54Z

Merged to master

…anager in local mode Author: Jeff Zhang <[email protected]> Closes apache#10914 from zjffdu/SPARK-12994.

jerryshao reviewed Jan 26, 2016
View reviewed changes

core/src/main/scala/org/apache/spark/SparkContext.scala Outdated

Copy link

Contributor

jerryshao Jan 26, 2016

Choose a reason for hiding this comment

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

Remove this empty line.

andrewor14 reviewed Feb 1, 2016
View reviewed changes

jerryshao reviewed Feb 2, 2016
View reviewed changes

andrewor14 reviewed Feb 2, 2016
View reviewed changes

srowen reviewed Feb 7, 2016
View reviewed changes

core/src/test/scala/org/apache/spark/util/UtilsSuite.scala Outdated

Copy link

Member

srowen Feb 7, 2016

Choose a reason for hiding this comment

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

asserting === true is redundant

srowen reviewed Feb 23, 2016
View reviewed changes

zjffdu added 9 commits February 24, 2016 10:37

[SPARK-12994][CORE] It is not necessary to create ExecutorAllocationM…

61c6cb9

…anager in local mode

address review comments

20f975d

Fix unit test failure

bcd4139

move to Utils.isDynamicAllocationEnabled

81e8e39

fix test failure

a124458

refactoring

d0d0a49

address comments

710e48e

address comments

eb832f2

fix test

bffa7a4

zjffdu force-pushed the SPARK-12994 branch from c15212b to bffa7a4 Compare February 24, 2016 02:53

srowen reviewed Feb 27, 2016
View reviewed changes

asfgit closed this in 99fe899 Feb 29, 2016

roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016

[SPARK-12994][CORE] It is not necessary to create ExecutorAllocationM…

830e53f

…anager in local mode Author: Jeff Zhang <[email protected]> Closes apache#10914 from zjffdu/SPARK-12994.

[SPARK-12994][CORE] It is not necessary to create ExecutorAllocationM… #10914

[SPARK-12994][CORE] It is not necessary to create ExecutorAllocationM… #10914

Uh oh!

Conversation

zjffdu commented Jan 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zjffdu commented Jan 26, 2016

Uh oh!

SparkQA commented Jan 26, 2016

Uh oh!

SparkQA commented Jan 26, 2016

Uh oh!

jerryshao commented Jan 26, 2016

Uh oh!

SparkQA commented Jan 26, 2016

Uh oh!

JoshRosen commented Jan 26, 2016

Uh oh!

zjffdu commented Jan 27, 2016

Uh oh!

JoshRosen commented Jan 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Feb 1, 2016

Uh oh!

zjffdu commented Feb 2, 2016

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 Feb 2, 2016

Uh oh!

SparkQA commented Feb 2, 2016

Uh oh!

zjffdu commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Feb 2, 2016

Uh oh!

zjffdu commented Feb 3, 2016

Uh oh!

SparkQA commented Feb 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Feb 16, 2016

Uh oh!

srowen commented Feb 18, 2016

Uh oh!

srowen commented Feb 23, 2016

Uh oh!

zjffdu commented Feb 23, 2016

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 Feb 23, 2016

Uh oh!

SparkQA commented Feb 24, 2016

Uh oh!

SparkQA commented Feb 24, 2016

Uh oh!

srowen commented Feb 26, 2016

Uh oh!