[SPARK-26192][MESOS][2.4] Retrieve enableFetcherCache option from submission for driver URIs #24713

mwlon · 2019-05-26T19:55:10Z

What changes were proposed in this pull request?

Retrieve enableFetcherCache option from submission conf rather than dispatcher conf. This resolves some confusing behavior where Spark drivers currently get this conf from the dispatcher, whereas Spark executors get this conf from the submission. After this change, the conf will only need to be specified once.

How was this patch tested?

With (updated) existing tests.

HyukjinKwon

What this PR targets? we don't add new confs in backports.

dongjoon-hyun · 2019-05-27T04:23:04Z

ok to test

dongjoon-hyun · 2019-05-27T04:35:22Z

Hi, @mwlon .
Thank you for making a PR, but this is not a backport of 5fd4d74 (SPARK-26192). Please revert the other parts. Otherwise, we cannot proceed this backport as @HyukjinKwon 's comment.

Hi, @HyukjinKwon .
I requested this backport of SPARK-26192 at JIRA (https://issues.apache.org/jira/browse/SPARK-26192). But, as you see, this is not a correct backport. The original scope of SPARK-26192 is 5fd4d74 which Retrieve enableFetcherCache option from submission conf rather than dispatcher conf.. There is no new configuration there.

SparkQA · 2019-05-27T04:44:36Z

Test build #105815 has finished for PR 24713 at commit d8b4f1e.

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

dongjoon-hyun · 2019-05-27T04:50:57Z

Ur, wait. It seems that I confused at the commit history. I'll comment back here.

dongjoon-hyun · 2019-05-27T05:00:14Z

The following is the full history.

SPARK-15994 add this configuration spark.mesos.fetcherCache.enable at Spark 2.1.0.
However, it made a typo at MesosClusterScheduler.scala
SPARK-26082 fixes the typo at Spark 2.4.1. (Also, Spark 2.3.4 will have the fix).
Now, SPARK-26192 fixes the configuration loading at Spark 3.0.0.

dongjoon-hyun · 2019-05-27T05:00:42Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala

      .createWithDefault("")
+
+  private[spark] val MAX_DRIVERS =
+    ConfigBuilder("spark.mesos.maxDrivers").intConf.createWithDefault(200)


This is not a part of SPARK-26192.

It's part of later additions for SPARK-26082 that SPARK-26192 built on in master. I think it's good practice to use these config objects rather than magic strings, and something we want to keep.

Backporting the improvement is not allowed. @mwlon . In addition, it's not a good practice to disguise one PR(SPARK-26082) into another PR (SPARK-26192) silently.

Although it's not crazy to backport small enhancements, I'd not backport this change. 3.0 changes most all of the configs to use ConfigBuilder, but I don't necessarily think we want to change just a few of them in 2.4, just out of general consistency.

dongjoon-hyun · 2019-05-27T05:00:50Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala

+    ConfigBuilder("spark.mesos.maxDrivers").intConf.createWithDefault(200)
+
+  private[spark] val RETAINED_DRIVERS =
+    ConfigBuilder("spark.mesos.retainedDrivers").intConf.createWithDefault(200)


dongjoon-hyun · 2019-05-27T05:00:58Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala

+  private[spark] val CLUSTER_RETRY_WAIT_MAX_SECONDS =
+    ConfigBuilder("spark.mesos.cluster.retry.wait.max")
+        .intConf
+        .createWithDefault(60) // 1 minute


dongjoon-hyun · 2019-05-27T05:01:26Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala

+        .doc("If set to true, all URIs (example: `spark.executor.uri`, `spark.mesos.uris`) will " +
+            "be cached by the Mesos Fetcher Cache.")
+        .booleanConf
+        .createWithDefault(false)


Let's not add this. What we need is just the functional logic of SPARK-26192.

So use magic strings instead of config objects?

Of course, that's the backport. You should not pull the master branch code blindly.

dongjoon-hyun · 2019-05-27T05:01:46Z

resource-managers/mesos/src/main/scala/org/apache/spark/deploy/rest/mesos/MesosRestServer.scala

+    val driverMemoryOverhead = sparkProperties.get(config.DRIVER_MEMORY_OVERHEAD.key)
    val driverCores = sparkProperties.get("spark.driver.cores")
-    val name = request.sparkProperties.getOrElse("spark.app.name", mainClass)
+    val name = sparkProperties.getOrElse("spark.app.name", mainClass)


Please revert all the changes in this file.

dongjoon-hyun · 2019-05-27T05:02:17Z

...rs/mesos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala

-  private val useFetchCache = conf.getBoolean("spark.mesos.fetcherCache.enable", false)
+  private val queuedCapacity = conf.get(config.MAX_DRIVERS)
+  private val retainedDrivers = conf.get(config.RETAINED_DRIVERS)
+  private val maxRetryWaitTime = conf.get(config.CLUSTER_RETRY_WAIT_MAX_SECONDS)


Please revert the above.

dongjoon-hyun · 2019-05-27T05:04:12Z

...rs/mesos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala

  }

  private def getDriverUris(desc: MesosDriverDescription): List[CommandInfo.URI] = {
+    val useFetchCache = desc.conf.get(config.ENABLE_FETCHER_CACHE)


This is completely wrong. The following is the Apache Spark patch.

val useFetchCache = desc.conf.get(config.ENABLE_FETCHER_CACHE) || conf.get(config.ENABLE_FETCHER_CACHE)

My mistake - I missed a couple commits in my cherry-pick. Will fix.

dongjoon-hyun

@mwlon . I left a few comments. Could you address them?

mwlon · 2019-05-28T19:21:40Z

@HyukjinKwon note that this doesn't add any new confs. It only adds existing confs to the relevant config object rather than using magic strings.

dongjoon-hyun · 2019-05-28T20:00:54Z

That confs don't exist in branch-2.4.

existing confs

mwlon · 2019-05-28T20:36:21Z

@dongjoon-hyun not sure what you mean - in 2.4 we have

private val queuedCapacity = conf.getInt("spark.mesos.maxDrivers", 200)
private val retainedDrivers = conf.getInt("spark.mesos.retainedDrivers", 200)
private val maxRetryWaitTime = conf.getInt("spark.mesos.cluster.retry.wait.max", 60) // 1 minute

in MesosClusterScheduler. So the confs already exist.

dongjoon-hyun · 2019-05-28T23:13:01Z

What I mean is ConfigBuilder("spark.mesos.maxDrivers"). We didn't create the config with ConfigBuilder in branch-2.4. It's just strings in branch-2.4.

dongjoon-hyun · 2019-05-28T23:13:31Z

Please follow the exiting way of branch-2.4. Otherwise, I cannot help you.

mwlon · 2019-05-29T18:05:16Z

Ok, if we don't want the other associated changes of SPARK-26192 and only the functional part, then close this PR and I'll make a new one later.

dongjoon-hyun · 2019-05-29T18:09:38Z

Yep. Thanks.

mwlon added 2 commits May 26, 2019 12:46

merged cherry-picked SPARK-26192 commits into 2.4

982358c

don't add unnecessary conf to Mesos config

d8b4f1e

mwlon changed the title ~~Spark 26192 2.4~~ [SPARK-26192][MESOS][2.4] May 26, 2019

HyukjinKwon reviewed May 27, 2019

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-26192][MESOS][2.4]~~ [SPARK-26192][MESOS][2.4] Retrieve enableFetcherCache option from submission for driver URIs May 27, 2019

dongjoon-hyun reviewed May 27, 2019

View reviewed changes

dongjoon-hyun requested changes May 27, 2019

View reviewed changes

dongjoon-hyun closed this May 29, 2019

[SPARK-26192][MESOS][2.4] Retrieve enableFetcherCache option from submission for driver URIs #24713

[SPARK-26192][MESOS][2.4] Retrieve enableFetcherCache option from submission for driver URIs #24713

Uh oh!

Conversation

mwlon commented May 26, 2019 • edited by dongjoon-hyun 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!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented May 27, 2019

Uh oh!

dongjoon-hyun commented May 27, 2019

Uh oh!

SparkQA commented May 27, 2019

Uh oh!

dongjoon-hyun commented May 27, 2019

Uh oh!

dongjoon-hyun commented May 27, 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!

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!

dongjoon-hyun May 28, 2019 • 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!

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!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

mwlon commented May 28, 2019

Uh oh!

dongjoon-hyun commented May 28, 2019

Uh oh!

mwlon commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mwlon commented May 29, 2019

Uh oh!

dongjoon-hyun commented May 29, 2019

Uh oh!

Reviewers

Assignees

mwlon commented May 26, 2019 •

edited by dongjoon-hyun

Loading

dongjoon-hyun May 28, 2019 •

edited

Loading

mwlon commented May 28, 2019 •

edited

Loading

dongjoon-hyun commented May 28, 2019 •

edited

Loading

dongjoon-hyun commented May 28, 2019 •

edited

Loading