[SPARK-19606][MESOS] Support constraints in spark-dispatcher #19543

pmackles · 2017-10-20T15:12:37Z

What changes were proposed in this pull request?

A discussed in SPARK-19606, the addition of a new config property named "spark.mesos.constraints.driver" for constraining drivers running on a Mesos cluster

How was this patch tested?

Corresponding unit test added also tested locally on a Mesos cluster

Please review http://spark.apache.org/contributing.html before opening a pull request.

felixcheung · 2017-10-20T17:31:32Z

test this please

SparkQA · 2017-10-20T17:34:11Z

Test build #82937 has finished for PR 19543 at commit 11d5859.

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

pmackles · 2017-10-20T22:09:30Z

@felixcheung - fixed scala-style issues and also updated the docs to include the new property

felixcheung · 2017-10-21T07:08:28Z

Jenkins, retest this please

SparkQA · 2017-10-21T07:14:30Z

Test build #82946 has finished for PR 19543 at commit efdc09c.

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

pmackles · 2017-10-21T11:15:29Z

My apologies. Reread the docs and ran style checks locally this time. Fingers crossed. Thanks @felixcheung

ArtRand · 2017-10-22T13:45:04Z

docs/running-on-mesos.md

+  <td><code>spark.mesos.driver.constraints</code></td>
+  <td>(none)</td>
+  <td>
+    Same as <code>spark.mesos.constraints</code> except applied to drivers.


Maybe add the this is only valid when launching jobs with the Mesos Dispatcher?

ArtRand · 2017-10-22T13:46:07Z

...sos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala

+        command,
+        Map("spark.mesos.executor.home" -> "test",
+          "spark.app.name" -> "test",
+          "spark.mesos.driver.constraints" -> "c:v"),


maybe test multiple constraints? (just to catch if someone breaks this code later)

felixcheung · 2017-10-22T20:02:38Z

Jenkins, retest this please

SparkQA · 2017-10-22T20:20:19Z

Test build #82963 has finished for PR 19543 at commit c587946.

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

felixcheung · 2017-10-24T06:44:22Z

@susanxhuynh would you like to review?
also looks like @ArtRand has some comments

pmackles · 2017-10-24T12:53:34Z

@ArtRand - thanks for the feedback. I made the suggested changes to the docs and added some more tests - including a test w/ multiple constraints

ArtRand · 2017-10-25T20:05:15Z

docs/running-on-mesos.md

+  <td>(none)</td>
+  <td>
+    Same as <code>spark.mesos.constraints</code> except applied to drivers when launched through the dispatcher. By default,
+    all resource offers will be accepted.


maybe "all resource offers with sufficient resources will be accepted" ?

Maybe clarify in the description for spark.mesos.constraints in the above cell that it only applies to executors.

ArtRand · 2017-10-25T20:07:42Z

Just one super-nit.

ArtRand · 2017-10-25T20:07:54Z

LGTM.

ttashi-rms · 2017-11-06T16:50:47Z

which release version is this intended for ?

pmackles · 2017-11-07T01:39:57Z

@ttashi-rms v2.2 and up

pmackles · 2017-11-09T13:38:31Z

@felixcheung - any chance of getting this merged into the upcoming 2.2.1 release? I cleaned up the merge conflict

felixcheung · 2017-11-09T17:51:33Z

@susanxhuynh or anyone from the mesos side would you please review?

susanxhuynh

@pmackles Thanks for the PR! A few nits, otherwise, LGTM.

susanxhuynh · 2017-11-09T21:52:51Z

docs/running-on-mesos.md

+  <td>(none)</td>
+  <td>
+    Same as <code>spark.mesos.constraints</code> except applied to drivers when launched through the dispatcher. By default,
+    all resource offers will be accepted.


Maybe clarify in the description for spark.mesos.constraints in the above cell that it only applies to executors.

susanxhuynh · 2017-11-09T22:05:17Z

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

@@ -601,10 +602,14 @@ private[spark] class MesosClusterScheduler(
    for (submission <- candidates) {
      val driverCpu = submission.cores
      val driverMem = submission.mem
-      logTrace(s"Finding offer to launch driver with cpu: $driverCpu, mem: $driverMem")
+      val driverConstraints =
+        parseConstraintString(submission.conf.get("spark.mesos.driver.constraints", ""))


New config properties would ideally be defined in the config object: https://github.com/apache/spark/blob/master/resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/config.scala#L24 and referenced by the variable name.

susanxhuynh · 2017-11-09T22:10:25Z

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

@@ -556,7 +556,8 @@ private[spark] class MesosClusterScheduler(

  private class ResourceOffer(
      val offer: Offer,
-      var remainingResources: JList[Resource]) {
+      var remainingResources: JList[Resource],
+      var attributes: JList[Attribute]) {
    override def toString(): String = {
      s"Offer id: ${offer.getId}, resources: ${remainingResources}"


Include the attributes in this string?

ttashi-rms · 2017-11-09T22:32:42Z

@pmackles your changes works great. we had to update SJS (spark-job-server) side to support the new param for dispatcher. Ended up updating the manager_start.sh incase anyone is trying to figure it out.

…e errors

…s and tests

…ended changes from PR

pmackles · 2017-11-10T12:54:00Z

@susanxhuynh, @ttashi-rms, @ArtRand - thanks for the feedback. I was able to make all of the suggested changes.

felixcheung

any other comment/feedback?
would you check within one day so that we could resolve this before cutting 2.2.1?

felixcheung · 2017-11-11T06:49:02Z

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

+      .doc("Attribute based constraints on mesos resource offers. Applied by the dispatcher " +
+        "when launching drivers. Default is to accept all offers with sufficient resources.")
+      .stringConf
+      .createWithDefault("")


should this be default to ""? looks like it might still match something

spark/resource-managers/mesos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerUtils.scala

Line 267 in fc45c2c

def matchesAttributeRequirements(

@felixcheung I think it's okay. There's a utility function, parseConstraintString, which parses the input string into a Map. If the string is empty, it returns an empty Map:

spark/resource-managers/mesos/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerUtils.scala

Line 321 in fc45c2c

def parseConstraintString(constraintsVal: String): Map[String, Set[String]] = {

This is also consistent with how the executor constraint string is parsed.

felixcheung · 2017-11-11T06:57:25Z

add to white list

felixcheung · 2017-11-11T06:57:28Z

Jenkins, retest this please

SparkQA · 2017-11-11T07:15:02Z

Test build #83718 has finished for PR 19543 at commit 158b6b9.

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

felixcheung · 2017-11-12T00:44:19Z

@susanxhuynh, @ttashi-rms, @ArtRand please take a look

susanxhuynh · 2017-11-12T18:48:52Z

LGTM

A discussed in SPARK-19606, the addition of a new config property named "spark.mesos.constraints.driver" for constraining drivers running on a Mesos cluster Corresponding unit test added also tested locally on a Mesos cluster Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Paul Mackles <pmackles@adobe.com> Closes #19543 from pmackles/SPARK-19606. (cherry picked from commit b3f9dbf) Signed-off-by: Felix Cheung <felixcheung@apache.org>

felixcheung · 2017-11-12T19:42:06Z

thanks @susanxhuynh
merged this to master.
the cherry pick to 2.2 was non-trivial, but merged, please keep an eye for this and please help test 2.2.1!

A discussed in SPARK-19606, the addition of a new config property named "spark.mesos.constraints.driver" for constraining drivers running on a Mesos cluster Corresponding unit test added also tested locally on a Mesos cluster Please review http://spark.apache.org/contributing.html before opening a pull request. Author: Paul Mackles <pmackles@adobe.com> Closes apache#19543 from pmackles/SPARK-19606. (cherry picked from commit b3f9dbf) Signed-off-by: Felix Cheung <felixcheung@apache.org>

ArtRand reviewed Oct 22, 2017

View reviewed changes

ArtRand reviewed Oct 25, 2017

View reviewed changes

pmackles force-pushed the SPARK-19606 branch from 7764618 to 4022c4a Compare November 9, 2017 13:34

susanxhuynh reviewed Nov 9, 2017

View reviewed changes

Paul Mackles added 6 commits November 10, 2017 05:57

[SPARK-19606] Support constraints in spark-dispatcher

eef42e6

[SPARK-19606] Support constraints in spark-dispatcher; update docs

256e649

[SPARK-19606] Support constraints in spark-dispatcher; fix scala styl…

f94f11e

…e errors

[SPARK-19606] Support constraints in spark-dispatcher; fix imports

92508ce

[SPARK-19606] Support constraints in spark-dispatcher; updates to doc…

8e7eed8

…s and tests

[SPARK-19606] Support constraints in spark-dispatcher; applied recomm…

158b6b9

…ended changes from PR

pmackles force-pushed the SPARK-19606 branch from 4022c4a to 158b6b9 Compare November 10, 2017 12:51

felixcheung reviewed Nov 11, 2017

View reviewed changes

asfgit closed this in b3f9dbf Nov 12, 2017

[SPARK-19606][MESOS] Support constraints in spark-dispatcher #19543

[SPARK-19606][MESOS] Support constraints in spark-dispatcher #19543

Conversation

pmackles commented Oct 20, 2017

What changes were proposed in this pull request?

How was this patch tested?

felixcheung commented Oct 20, 2017

SparkQA commented Oct 20, 2017

pmackles commented Oct 20, 2017

felixcheung commented Oct 21, 2017

SparkQA commented Oct 21, 2017

pmackles commented Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

ArtRand Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

felixcheung commented Oct 22, 2017

SparkQA commented Oct 22, 2017

felixcheung commented Oct 24, 2017

pmackles commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArtRand commented Oct 25, 2017

ArtRand commented Oct 25, 2017

ttashi-rms commented Nov 6, 2017

pmackles commented Nov 7, 2017

pmackles commented Nov 9, 2017

felixcheung commented Nov 9, 2017

susanxhuynh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttashi-rms commented Nov 9, 2017

pmackles commented Nov 10, 2017

felixcheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixcheung commented Nov 11, 2017

felixcheung commented Nov 11, 2017

SparkQA commented Nov 11, 2017

felixcheung commented Nov 12, 2017

susanxhuynh commented Nov 12, 2017

felixcheung commented Nov 12, 2017

pmackles commented Oct 21, 2017 •

edited

Loading

ArtRand Oct 22, 2017 •

edited

Loading