[SPARK-17062][MESOS] add conf option to mesos dispatcher #14650

skonto · 2016-08-15T17:27:32Z

What changes were proposed in this pull request?

Adds --conf option to set spark configuration properties in mesos dispacther.
Properties provided with --conf take precedence over properties within the properties file.
The reason for this PR is that for simple configuration or testing purposes we need to provide a property file (ideally a shared one for a cluster) even if we just provide a single property.

How was this patch tested?

Manually tested.

skonto · 2016-08-15T17:28:44Z

@mgummelt pls review.

SparkQA · 2016-08-15T19:44:52Z

Test build #63790 has finished for PR 14650 at commit db42e31.

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

rxin · 2016-08-16T08:19:52Z

Does the existing spark-submit not work on Mesos?

skonto · 2016-08-16T09:51:16Z

@rxin:
MesosDispacther uses spark-daemon.sh which uses class not submit.
It also creates the sparkConfig from scratch using its own logic, so no luck there I guess.
I have not checked the implications of launching it with spark-submit and whether it can re-use its logic, isn't the case that class is more suitable here for consistency reasons (at least)? For example spark executors on Mesos are started with class. I think we are just starting a class within the project not submitting something for example an app.

skonto · 2016-08-16T14:54:22Z

@srowen @mgummelt what do you think?

srowen · 2016-08-17T10:23:21Z

Seems OK to me, to the limits of my understanding, and given the logic of #14650 (comment)

skonto · 2016-08-17T16:02:34Z

@mgummelt?

skonto · 2016-08-19T15:40:22Z

@srowen could you merge it pls?

mgummelt · 2016-08-19T18:21:58Z

I'm generally fine with this, though one downside is that it introduces a consistency with other daemon classes such as Master.scala, which only accepts a properties file. Maybe we should make a JIRA to add this to the other classes.

Not that Spark configuration has any sort of sane consistency to be upheld. There are ~10 ways of setting config properties. It's one of the most confusing things about operations.

@srowen Do you know why even have a separate set of Spark config properties, rather than just using Java System properties? I know you can load Spark conf from Java properties, but you can also load them via --conf (and only in spark-submit), which seems like an unnecessary nonstandard interface.

mgummelt · 2016-08-19T18:29:33Z

core/src/main/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcherArguments.scala

@@ -73,6 +82,10 @@ private[mesos] class MesosClusterDispatcherArguments(args: Array[String], conf:
      propertiesFile = value
      parse(tail)

+    case ("--conf") :: value :: tail =>
+      cliSparkConfig += value


Let's make this consistent with SparkSubmitArguments, and parse these into a HashMap here.

srowen · 2016-08-20T10:55:38Z

Responding to #14650 (comment) , IMHO it's best if configuration never uses environment variables or system properties. They're global to a process and that eventually ends in some tears, despite its convenience. Hence I personally support standardizing towards explicit configuration.

In practice some env variables are used here and there for legacy reasons. And yes you can set props via system properties. I think being consistent is OK, right or wrong, but would generally move away from env / sys properties if given the choice.

skonto · 2016-08-29T09:03:43Z

WIP

SparkQA · 2016-09-13T13:42:42Z

Test build #65311 has finished for PR 14650 at commit 3532f5d.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

skonto · 2016-09-13T21:38:24Z

@mgummelt @srowen I refactored a bit MesosSchedulerDispatcherArguments to follow the SparkSubmit pattern. Added several tests.
My thinking is that all objects with a main method (SparkSubmit, MesosClusterDispatcher, Yarn stuff..) which utilize spark conf and other args should have a common structure for parsing args and testing. So I re-used the spark submit design to drive this, minimizing technical debt in a sense.
It is a minor thing but it is good to have consistency across the code base. If it is acceptable I can further refactor all remaining stuff in another pr.

SparkQA · 2016-09-13T22:59:35Z

Test build #65333 has finished for PR 14650 at commit 497e517.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- trait SparkConfigParseUtils

skonto · 2016-09-14T06:56:25Z

WIP fixing test, removing public class...

SparkQA · 2016-09-14T09:45:05Z

Test build #65358 has finished for PR 14650 at commit 342d8d3.

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

skonto · 2016-09-14T10:02:46Z

@srowen @mgummelt pls review...

skonto · 2016-09-19T08:45:06Z

@srowen what do you think?

srowen

CC @vanzin because this is touching spark submit.

srowen · 2016-09-19T08:46:41Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala

 import org.apache.spark.{SPARK_BRANCH, SPARK_BUILD_DATE, SPARK_BUILD_USER, SPARK_REPO_URL}
+import org.apache.spark.{SPARK_REVISION, SPARK_VERSION, SparkException, SparkUserAppException}


This is trivial, but I think at this point a "_" import is fine and called for. It's split over two imports now.

Ok cool It was there before my PR but I will fix it no problem.

srowen · 2016-09-19T08:49:09Z

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

@@ -27,28 +27,18 @@ import org.scalatest.{BeforeAndAfterEach, Matchers}
 import org.scalatest.concurrent.Timeouts
 import org.scalatest.time.SpanSugar._

-import org.apache.spark._
+import org.apache.spark.{SparkFunSuite, _}


Not needed? it already imports everything

srowen · 2016-09-19T08:50:25Z

mesos/src/main/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcherArguments.scala

        // scalastyle:on println
        printUsageAndExit(1)
      }

-    case _ =>
+    case value@_ =>


case value => or maybe I misunderstand why it's written this way

Its the same thing case value => less verbose. Will change it.

srowen · 2016-09-19T08:54:41Z

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

+/*
+ * Adds main method to a class or object
+ */
+private[spark] trait Executable {


This trait feels funny. I didn't see what it is needed for though. It's used in tests only, but isn't used in a place where you need to abstract over both implementations?

I needed to mark specific classes with main support, I could have used a different implementation to find out about it like duck typing, or reflection etc but these are either not permitted (eg. scala style) or are too complex. It is not bad to tag a class that way. Maybe sound redundant even funny (I would not call it that way) but it serves a purpose to make things easy for the test code.

I don't think you need any of that; it seems like this is only called where SparkSubmit is definitely being invoked. I didn't see where a different implementation would even be used.

Ok so in the code there is no reason as you said. In the test code I tried to re-use this function here doing some refactoring first. In order this to work, I need to pass an object for testing and that object needs to implement some specific functions.
Spark project does not allow duck typing and I thought reflection is too much. I could have avoided the trait in the first place by not reusing the function but I thought I could improve test code too.

Anyway I can eliminate it with reflection, it is test code anyway, is it ok @srowen?

Agree that this trait seems unnecessary. For the test, you can use two arguments: a "SparkConfigParseUtils" and a second one that's a main function (main: Array[String] => Unit).

This trait still exists and still looks out of place. It could be merged with CommandLineUtils, which then could be called something like SparkEntryPoint (although CommandLineUtils is also probably fine).

srowen · 2016-09-19T08:56:43Z

mesos/src/main/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcherArguments.scala

  propertiesFile = Utils.loadDefaultSparkProperties(conf, propertiesFile)
+  Utils.updateSparkConfigFromProperties(conf, confProperties)


This seems like it differs from Mesos to non-Mesos implementations. Here you can update props with --conf but wouldn't this be identical elsewhere? I may have missed the purpose.

Logic differs wrt to others (I would have to refactor everything otherwise which is not wise given code size for now). I just added support for the --conf option, that is the same across implementations, and re-used common code for parsing stuff.

I guess I mean, why isn't this same code then called elsewhere? You know more about this than I, but I suppose I'd expect more parallel similarity between the Mesos and non-Mesos code, for supporting the same functionality. There's a method factored out here but not reused. Marcelo may understand this more anyway.

Non-mesos code either does not support --conf or it does not need in all cases to set Spark confguration. Check here and here. If you compare the same functionality (the one about loading the properties file) with the MesosClusterDispatcherArguments file, things are completely different, even before my PR. Check here.

So I dont expect any parallel similarity since for example in this case ApplicationMaster does not need to get the spark configuration in a Spark Config.

SparkQA · 2016-09-19T14:10:09Z

Test build #65594 has finished for PR 14650 at commit d40b928.

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

vanzin

I need to do a second pass (have to run now), but I really would like that Executable trait to go away.

vanzin · 2016-10-19T22:50:27Z

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

+/*
+ * Adds main method to a class or object
+ */
+private[spark] trait Executable {


This trait still exists and still looks out of place. It could be merged with CommandLineUtils, which then could be called something like SparkEntryPoint (although CommandLineUtils is also probably fine).

vanzin · 2016-10-19T22:56:02Z

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

+    exitFn(1)
+  }
+
+  private[spark] def parseSparkConfProperty(pair: String): Either[Unit, (String, String)] = {


Returning Either here is weird. It should return (String, String). I know you did this because of the error handling case, but that's better handled by:

printErrorAndExit(s"Spark config without '=': $pair") throw new SparkException(s"Spark config without '=': $pair")

That way if exitFn doesn't really exit, you get the exception. Hopefully only tests override exitFn and those shouldn't really trigger the exception, so it's there just to make the compiler happy.

Ok cool I was not aware if I can throw a SparkException at that point (if the rest of the code base follows a similar approach)... exception is much better I was just wondering about it...

Ok I fixed that although throw new SparkException(s"Spark config without '=': $pair") is a bit redundant, it is there only to make compiler happy which is again a bit strange to me, anyway.
User of Either is a bit too much as well. Not so happy in general.

vanzin · 2016-10-19T22:56:31Z

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

+ */
+private[spark] trait CommandLineUtils {
+
+  // scalastyle:off println


nit: you're missing // scalastyle:on println

vanzin · 2016-10-19T22:56:49Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala

-        }
+        SparkSubmit.parseSparkConfProperty(value)
+          .right
+          .foreach{case (k, v) => sparkProperties(k) = v}


nit: space before and after {

vanzin · 2016-10-19T22:57:09Z

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

+  def updateSparkConfigFromProperties(
+      conf: SparkConf,
+      properties: Map[String, String])
+    : Unit = {


nit: move to previous line

vanzin · 2016-10-19T22:57:26Z

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

+      input: Array[String],
+      searchString: String,
+      mainObject: Executable with CommandLineUtils = SparkSubmit)
+    : Unit = {


nit: move to previous line

vanzin · 2016-10-19T22:57:56Z

mesos/src/test/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcherArgumentsSuite.scala

+import org.apache.spark.deploy.TestPrematureExit
+
+class MesosClusterDispatcherArgumentsSuite extends SparkFunSuite
+  with TestPrematureExit{


nit: space before '{'

skonto · 2016-10-20T14:43:14Z

@vanzin & @srowen I will update the PR asap thnx for the review.

skonto · 2016-11-15T13:09:52Z

WIP

skonto · 2016-11-16T12:02:08Z

@vanzin @srowen @mgummelt pls review. I am sorry for the late response.

SparkQA · 2016-11-16T12:10:03Z

Test build #68714 has finished for PR 14650 at commit 19dbd73.

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

skonto · 2016-11-16T12:23:24Z

Jenkins, retest this please.

SparkQA · 2016-11-16T12:37:46Z

Test build #68715 has finished for PR 14650 at commit 19dbd73.

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

SparkQA · 2016-11-16T15:31:15Z

Test build #68718 has finished for PR 14650 at commit cfadc06.

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

skonto · 2016-11-17T12:38:07Z

@srowen could I get a merge please?

rxin · 2016-11-17T18:08:12Z

@vanzin did you do your 2nd pass?

vanzin · 2016-11-17T18:24:49Z

Not yet, I'll try to find some time today.

vanzin

I'm not sure updateSparkConfigFromProperties should be touching system properties. We really should avoid adding more code that messes with those.

Other than that, looks ok.

vanzin · 2016-11-17T20:37:45Z

core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala

-          case Seq(k, v) => sparkProperties(k) = v
-          case _ => SparkSubmit.printErrorAndExit(s"Spark config without '=': $value")
-        }
+        val pair = SparkSubmit.parseSparkConfProperty(value)


nit: val (confName, confValue) = SparkSubmit.parseSparkConfProperty(value)

vanzin · 2016-11-17T20:38:48Z

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

+ */
+private[spark] trait CommandLineUtils {
+
+  // scalastyle:on println


nit: you don't need this

vanzin · 2016-11-17T20:39:09Z

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

+        throw new SparkException(s"Spark config without '=': $pair")
+    }
+  }
+  // scalastyle:on println


nit: you can move this after printErrorAndExit.

vanzin · 2016-11-17T20:39:20Z

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

+
+  // scalastyle:off println
+  private[spark] def printWarning(str: String): Unit = printStream.println("Warning: " + str)
+


nit: remove extra empty line

removed the empty line it still shows one which is used to separate statements there.

vanzin · 2016-11-17T20:43:13Z

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

+      k.startsWith("spark.")
+    }.foreach { case (k, v) =>
+      conf.set(k, v)
+      sys.props.getOrElseUpdate(k, v)


Why is this method updating system properties? The name nor the scaladoc mention that fact, and this is a pretty important thing the method does. Not only that but the behavior is different from what it does to the SparkConf (set if not yet set, instead of always set).

loadDefaultSparkProperties method a few lines above does that so I thought this is a common practice to load stuff there because they might be reused elsewhere. Also this method is used at the Dispatcher side only. I can remove it for sure.

skonto · 2016-11-17T23:44:27Z

@vanzin I updated the PR. Thank you for the review.
@rxin its ready I guess.

SparkQA · 2016-11-18T02:07:58Z

Test build #68805 has finished for PR 14650 at commit 946202d.

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

skonto · 2016-11-18T22:25:41Z

@rxin @srowen could I get merge pls if there are no other issues?

srowen · 2016-11-19T12:07:03Z

Since @vanzin has looked at this most closely I'd defer to him for a merge when you both think it's ready.

vanzin · 2016-11-20T00:02:28Z

LGTM. There are a few style nits that I'll fix during the merge to avoid another round.

Merging to master.

skonto · 2016-11-20T16:55:55Z

@vanzin if there is anything to fix I can do it if you want I can do another parse to check it against the style guide. Eg. avoid imports such as import org.apache.spark.util._

I thought when you did the 2nd parse you we were ok with the result.
For all the style nit issues I guess sometimes is a matter of taste (although technically correct)...

vanzin · 2016-11-20T19:56:49Z

This is already merged. Maybe there's something wrong with the apache / github sync.

https://git-wip-us.apache.org/repos/asf?p=spark.git;a=commit;h=ea77c81ec0db27ea4709f71dc080d00167505a7d

rxin · 2016-11-20T21:52:43Z

Guys this broke the build. Please don't use Option.contains in the future. It is both confusing and not working for Scala 2.10.

vanzin · 2016-11-20T22:25:34Z

I see you already pushed a fix, thanks.

Adds --conf option to set spark configuration properties in mesos dispacther. Properties provided with --conf take precedence over properties within the properties file. The reason for this PR is that for simple configuration or testing purposes we need to provide a property file (ideally a shared one for a cluster) even if we just provide a single property. Manually tested. Author: Stavros Kontopoulos <st.kontopoulos@gmail.com> Author: Stavros Kontopoulos <stavros.kontopoulos@lightbend.com> Closes apache#14650 from skonto/dipatcher_conf.

add conf option to mesos dispatcher

db42e31

mgummelt reviewed Aug 19, 2016
View reviewed changes

Merge branch 'master' into dipatcher_conf

3532f5d

refactor

497e517

srowen requested changes Sep 19, 2016

View reviewed changes

fix test

d40b928

skonto force-pushed the dipatcher_conf branch from 342d8d3 to d40b928 Compare September 19, 2016 11:59

vanzin reviewed Oct 19, 2016

View reviewed changes

skonto force-pushed the dipatcher_conf branch 2 times, most recently from e089939 to 19dbd73 Compare November 16, 2016 12:01

skonto force-pushed the dipatcher_conf branch from 19dbd73 to cfadc06 Compare November 16, 2016 12:46

vanzin reviewed Nov 17, 2016

View reviewed changes

fixes

946202d

skonto force-pushed the dipatcher_conf branch from cfadc06 to 946202d Compare November 17, 2016 23:36

asfgit closed this in ea77c81 Nov 20, 2016

		import org.apache.spark.{SPARK_BRANCH, SPARK_BUILD_DATE, SPARK_BUILD_USER, SPARK_REPO_URL}
		import org.apache.spark.{SPARK_REVISION, SPARK_VERSION, SparkException, SparkUserAppException}

		propertiesFile = Utils.loadDefaultSparkProperties(conf, propertiesFile)
		Utils.updateSparkConfigFromProperties(conf, confProperties)


		// scalastyle:off println
		private[spark] def printWarning(str: String): Unit = printStream.println("Warning: " + str)

[SPARK-17062][MESOS] add conf option to mesos dispatcher #14650

[SPARK-17062][MESOS] add conf option to mesos dispatcher #14650

Uh oh!

Conversation

skonto commented Aug 15, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

skonto commented Aug 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Aug 15, 2016

Uh oh!

rxin commented Aug 16, 2016

Uh oh!

skonto commented Aug 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skonto commented Aug 16, 2016

Uh oh!

srowen commented Aug 17, 2016

Uh oh!

skonto commented Aug 17, 2016

Uh oh!

skonto commented Aug 19, 2016

Uh oh!

mgummelt commented Aug 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Aug 20, 2016

Uh oh!

skonto commented Aug 29, 2016

Uh oh!

SparkQA commented Sep 13, 2016

Uh oh!

skonto commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Sep 13, 2016

Uh oh!

skonto commented Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Sep 14, 2016

Uh oh!

skonto commented Sep 14, 2016

Uh oh!

skonto commented Sep 19, 2016

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!

skonto Sep 19, 2016 • 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!

skonto Sep 19, 2016 • 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!

skonto Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

skonto commented Aug 15, 2016 •

edited

Loading

skonto commented Aug 16, 2016 •

edited

Loading

skonto commented Sep 13, 2016 •

edited

Loading

skonto commented Sep 14, 2016 •

edited

Loading

skonto Sep 19, 2016 •

edited

Loading

skonto Sep 19, 2016 •

edited

Loading

skonto Sep 21, 2016 •

edited

Loading

skonto Sep 21, 2016 •

edited

Loading

vanzin Sep 21, 2016 •

edited

Loading

skonto Sep 19, 2016 •

edited

Loading

skonto Sep 21, 2016 •

edited

Loading

skonto Oct 20, 2016 •

edited

Loading

skonto Oct 20, 2016 •

edited

Loading

skonto Nov 16, 2016 •

edited

Loading