[SPARK-26463][CORE] Use ConfigEntry for hardcoded configs for scheduler categories. by kiszk · Pull Request #23416 · apache/spark

kiszk · 2018-12-30T18:35:00Z

What changes were proposed in this pull request?

The PR makes hardcoded spark.dynamicAllocation, spark.scheduler, spark.rpc, spark.task, spark.speculation, and spark.cleaner configs to use ConfigEntry.

How was this patch tested?

Existing tests

SparkQA · 2018-12-30T18:38:12Z

Test build #100568 has finished for PR 23416 at commit d426930.

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

SparkQA · 2018-12-30T18:49:44Z

Test build #100569 has finished for PR 23416 at commit 8efb913.

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

SparkQA · 2018-12-31T00:08:44Z

Test build #100576 has finished for PR 23416 at commit 4fae0f6.

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

SparkQA · 2018-12-31T03:17:40Z

Test build #100581 has finished for PR 23416 at commit c2f1d9e.

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

SparkQA · 2018-12-31T06:04:31Z

Test build #100578 has finished for PR 23416 at commit 1e95d0c.

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

SparkQA · 2018-12-31T08:05:02Z

Test build #100583 has finished for PR 23416 at commit 7ae645e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-12-31T10:36:26Z

retest this please

viirya · 2018-12-31T13:25:09Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

I feels that SPECULATION_ENABLED is better than SPECULATION.

SparkQA · 2018-12-31T15:33:43Z

Test build #100589 has finished for PR 23416 at commit 7ae645e.

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

dongjoon-hyun · 2019-01-01T05:49:35Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

MAX_TASK_MAX_DIRECT_RESULT_SIZE -> MAX_TASK_DIRECT_RESULT_SIZE?

vanzin

Seems like we could also have a new Scheduler object with the settings you're adding?

vanzin · 2019-01-03T23:39:39Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

Could you add a new Network.scala for these? If there are any existing network settings in this file you could also move them over.

I see. I will create Network.scala

SparkQA · 2019-01-06T16:09:49Z

Test build #100836 has finished for PR 23416 at commit 4be75ff.

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

kiszk · 2019-01-06T18:37:24Z

core/src/main/scala/org/apache/spark/internal/config/Network.scala

This seems to be convervative movements.

SparkQA · 2019-01-06T23:05:15Z

Test build #100841 has finished for PR 23416 at commit 5b8ccbb.

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

ueshin · 2019-01-07T07:18:18Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

fallbackConf(DYN_ALLOCATION_SCHEDULER_BACKLOG_TIMEOUT)? Otherwise seems like the behavior will change in ExecutorAllocationManager.

ueshin · 2019-01-07T07:32:26Z

core/src/main/scala/org/apache/spark/ui/jobs/JobsTab.scala

We need .key?

ueshin · 2019-01-07T07:35:30Z

core/src/main/scala/org/apache/spark/ui/jobs/StagesTab.scala

ueshin · 2019-01-07T07:41:22Z

core/src/test/scala/org/apache/spark/deploy/StandaloneDynamicAllocationSuite.scala

Shall we use config. prefix here instead of import config._ just for these?

ueshin · 2019-01-07T07:41:52Z

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

ueshin · 2019-01-07T07:43:35Z

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

ditto. and some more in this file.

ueshin · 2019-01-07T07:44:04Z

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

ditto. and some more in this file.

ueshin · 2019-01-07T07:44:59Z

sql/core/src/test/scala/org/apache/spark/sql/RuntimeConfigSuite.scala

SparkQA · 2019-01-11T05:30:17Z

Test build #101045 has finished for PR 23416 at commit 0706d9a.

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

dongjoon-hyun · 2019-01-11T05:33:12Z

Retest this please

SparkQA · 2019-01-11T08:05:02Z

Test build #101062 has finished for PR 23416 at commit 0706d9a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-01-11T08:10:15Z

Retest this please.

kiszk · 2019-01-11T08:24:05Z

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

Here requires #23447 to set default value in Network.scala.

SparkQA · 2019-01-11T12:18:49Z

Test build #101072 has finished for PR 23416 at commit 0de0e76.

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

SparkQA · 2019-01-11T12:25:28Z

Test build #101070 has finished for PR 23416 at commit 0706d9a.

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

SparkQA · 2019-01-12T07:12:09Z

Test build #101118 has finished for PR 23416 at commit 2854b76.

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

SparkQA · 2019-01-12T08:05:02Z

Test build #101119 has finished for PR 23416 at commit e0f231a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

…/speculation/cleaner categories

SparkQA · 2019-01-19T09:43:45Z

Test build #101431 has finished for PR 23416 at commit fbf9953.

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

SparkQA · 2019-01-19T14:25:23Z

Test build #101432 has finished for PR 23416 at commit 1a8d84b.

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

dongjoon-hyun · 2019-01-19T19:21:07Z

Retest this please.

SparkQA · 2019-01-19T23:37:10Z

Test build #101438 has finished for PR 23416 at commit 1a8d84b.

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

srowen

A few more questions but this is looking good. I appreciate all the work to finish out this important refactoring.

srowen · 2019-01-20T16:24:00Z

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

 private[spark] object TaskSchedulerImpl {

-  val SCHEDULER_MODE_PROPERTY = "spark.scheduler.mode"
+  val SCHEDULER_MODE_PROPERTY = SCHEDULER_MODE.key


I think this becomes unused after your change on line 135?

Unfortunally, this is still used in two files.

srowen · 2019-01-20T16:26:21Z

...rc/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala


  protected override val minRegisteredRatio =
-    if (conf.getOption("spark.scheduler.minRegisteredResourcesRatio").isEmpty) {
+    if (conf.get(SCHEDULER_MIN_REGISTERED_RESOURCES_RATIO).isEmpty) {


getOrElse?

Because this is very interesting code that does not use it value even if SCHEDULER_MIN_REGISTERED_RESOURCES_RATIO is declared, we cannot use getOrElse. I am not sure why it is.

This code just checks whether SCHEDULER_MIN_REGISTERED_RESOURCES_RATIO is declared or not.

srowen · 2019-01-20T16:26:57Z

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

  // is equal to at least this value, that is double between 0 and 1.
  private val _minRegisteredRatio =
-    math.min(1, conf.getDouble("spark.scheduler.minRegisteredResourcesRatio", 0))
+    math.min(1, conf.get(SCHEDULER_MIN_REGISTERED_RESOURCES_RATIO).getOrElse(0.0))


Can this prop just have a default value of 0 where it's declared?

We cannot have a defalut value 0 where it is declared due to the interesting code at KubernetesClusterSchedulerBackend.scala.
We need to know SCHEDULER_MIN_REGISTERED_RESOURCES_RATIO is declared or not.

core/src/main/scala/org/apache/spark/ui/jobs/JobsTab.scala

srowen · 2019-01-20T16:30:45Z

core/src/test/scala/org/apache/spark/JobCancellationSuite.scala


  test("cluster mode, FIFO scheduler") {
-    val conf = new SparkConf().set("spark.scheduler.mode", "FIFO")
+    val conf = new SparkConf().set(SCHEDULER_MODE, "FIFO")


Do you want to use SchedulingMode.FIFO.toString in cases like this? no big deal, it doesn't matter much

srowen · 2019-01-20T16:31:09Z

core/src/test/scala/org/apache/spark/MapOutputTrackerSuite.scala

-    newConf.set("spark.rpc.message.maxSize", "1")
-    newConf.set("spark.rpc.askTimeout", "1") // Fail fast
+    newConf.set(RPC_MESSAGE_MAX_SIZE, 1)
+    newConf.set(RPC_ASK_TIMEOUT, "1") // Fail fast


In cases like this can you set the value to 1 instead of a string?

SparkQA · 2019-01-20T21:03:57Z

Test build #101447 has finished for PR 23416 at commit 2b8a923.

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

srowen · 2019-01-22T13:44:10Z

OK, the rest looks OK to me. Merging to master

…er categories. ## What changes were proposed in this pull request? The PR makes hardcoded `spark.dynamicAllocation`, `spark.scheduler`, `spark.rpc`, `spark.task`, `spark.speculation`, and `spark.cleaner` configs to use `ConfigEntry`. ## How was this patch tested? Existing tests Closes apache#23416 from kiszk/SPARK-26463. Authored-by: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Signed-off-by: Sean Owen <sean.owen@databricks.com>

kiszk changed the title ~~[SPARK-26463][WIP][CORE] Use ConfigEntry for hardcoded configs for scheduler categories.~~ [SPARK-26463][CORE] Use ConfigEntry for hardcoded configs for scheduler categories. Dec 31, 2018

viirya reviewed Dec 31, 2018

View reviewed changes

dongjoon-hyun reviewed Jan 1, 2019

View reviewed changes

vanzin reviewed Jan 3, 2019

View reviewed changes

kiszk force-pushed the SPARK-26463 branch from 7ae645e to 4be75ff Compare January 6, 2019 15:58

kiszk commented Jan 6, 2019

View reviewed changes

kiszk mentioned this pull request Jan 7, 2019

[SPARK-24920][Core] Allow sharing Netty's memory pool allocators #23278

Closed

ueshin reviewed Jan 7, 2019

View reviewed changes

kiszk force-pushed the SPARK-26463 branch from 5b8ccbb to 0706d9a Compare January 11, 2019 01:21

kiszk commented Jan 11, 2019

View reviewed changes

kiszk force-pushed the SPARK-26463 branch from 0de0e76 to 2854b76 Compare January 12, 2019 07:01

kiszk force-pushed the SPARK-26463 branch from e0f231a to 21f95a9 Compare January 13, 2019 02:50

kiszk added 16 commits January 19, 2019 16:22

Use ConfigEntry for hardcoded configs for dynamicAllocation/scheduler…

6fc73e4

…/speculation/cleaner categories

fix style error

266b798

fix build error

01ddf19

fix build failure

1e1694c

Use ConfigEntry for handcoded configs for rpc category

b76437c

fix style error

5cb7a02

Use ConfigEntry for handcoded configs for task category

c284f2e

address review comments

c3580c5

fix build error

809e46f

address review comments

0538f07

address review comments

9e4bd5e

fix build failure

f2a071e

address review comment

39bffce

rebase with master

8cb92ff

change crlf to lf

3334e57

address review comment

fbf9953

kiszk force-pushed the SPARK-26463 branch from af8e003 to fbf9953 Compare January 19, 2019 09:38

fix scala style error

1a8d84b

kiszk added 2 commits January 20, 2019 16:02

fix test failure

740581b

create a new config entry

2b8a923

srowen reviewed Jan 20, 2019

View reviewed changes

srowen approved these changes Jan 22, 2019

View reviewed changes

srowen closed this in 7bf0794 Jan 22, 2019

Conversation

kiszk commented Dec 30, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Dec 30, 2018

Uh oh!

SparkQA commented Dec 30, 2018

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

kiszk commented Dec 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

SparkQA commented Jan 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 6, 2019

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 11, 2019

Uh oh!

dongjoon-hyun commented Jan 11, 2019

Uh oh!

SparkQA commented Jan 11, 2019

Uh oh!

dongjoon-hyun commented Jan 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 11, 2019

Uh oh!

SparkQA commented Jan 11, 2019

Uh oh!

SparkQA commented Jan 12, 2019

Uh oh!

SparkQA commented Jan 12, 2019

Uh oh!

SparkQA commented Jan 19, 2019

Uh oh!

SparkQA commented Jan 19, 2019

Uh oh!

dongjoon-hyun commented Jan 19, 2019

Uh oh!