[GitHub] spark pull request #20641: [SPARK-23464][MESOS] Fix mesos cluster scheduler ...

[skonto]

Github user skonto commented on a diff in the pull request:

https://github.com/apache/spark/pull/20641#discussion_r193437370
  
--- Diff: 
resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala
 ---
@@ -199,6 +199,38 @@ class MesosClusterSchedulerSuite extends SparkFunSuite 
with LocalSparkContext wi
 })
   }
 
+  test("properly wraps and escapes parameters passed to driver command") {
--- End diff --

@susanxhuynh I can do a test for this.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20641: [SPARK-23464][MESOS] Fix mesos cluster scheduler ...

[susanxhuynh]

Github user susanxhuynh commented on a diff in the pull request:

https://github.com/apache/spark/pull/20641#discussion_r182271196
  
--- Diff: 
resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala
 ---
@@ -199,6 +199,38 @@ class MesosClusterSchedulerSuite extends SparkFunSuite 
with LocalSparkContext wi
 })
   }
 
+  test("properly wraps and escapes parameters passed to driver command") {
--- End diff --

Sorry for the delay. I was going to test this in DC/OS and haven't gotten a 
chance to do so.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20641: [SPARK-23464][MESOS] Fix mesos cluster scheduler ...

[adyatlov]

Github user adyatlov commented on a diff in the pull request:

https://github.com/apache/spark/pull/20641#discussion_r182013144
  
--- Diff: 
resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala
 ---
@@ -199,6 +199,38 @@ class MesosClusterSchedulerSuite extends SparkFunSuite 
with LocalSparkContext wi
 })
   }
 
+  test("properly wraps and escapes parameters passed to driver command") {
--- End diff --

That would be great to merge it if you think it's ready.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20641: [SPARK-23464][MESOS] Fix mesos cluster scheduler ...

[blue666man]

Github user blue666man commented on a diff in the pull request:

https://github.com/apache/spark/pull/20641#discussion_r176475736
  
--- Diff: 
resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala
 ---
@@ -199,6 +199,38 @@ class MesosClusterSchedulerSuite extends SparkFunSuite 
with LocalSparkContext wi
 })
   }
 
+  test("properly wraps and escapes parameters passed to driver command") {
--- End diff --

This looks ready to merge; anything else needed @skonto or @susanxhuynh ?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20641: [SPARK-23464][MESOS] Fix mesos cluster scheduler ...

[krcz]

Github user krcz commented on a diff in the pull request:

https://github.com/apache/spark/pull/20641#discussion_r175863304
  
--- Diff: 
resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala
 ---
@@ -199,6 +199,38 @@ class MesosClusterSchedulerSuite extends SparkFunSuite 
with LocalSparkContext wi
 })
   }
 
+  test("properly wraps and escapes parameters passed to driver command") {
--- End diff --

It does.

```
[info] - properly wraps and escapes parameters passed to driver command *** 
FAILED *** (154 milliseconds)
[info]   "test/./bin/spark-submit --name test --master 
mesos://mesos://localhost:5050 --driver-cores 1.0 --driver-memory 1000M --class 
mainClass --conf "spark.app.name=test" --conf "spark.mesos.executor.home=tes
t" --conf "spark.executor.extraJavaOptions="-Dparam1=\"value 1\" 
-Dparam2=value\\ 2 -Dpath=\$PATH"" --conf 
"spark.driver.extraJavaOptions="-XX+PrintGC -Dparam1=val1 -Dparam2=val2"" ./jar 
arg" did not contain "--
conf spark.driver.extraJavaOptions="-XX+PrintGC -Dparam1=val1 
-Dparam2=val2"" (MesosClusterSchedulerSuite.scala:227)
[info]   org.scalatest.exceptions.TestFailedException:
[info]   at 
org.scalatest.Assertions$class.newAssertionFailedException(Assertions.scala:528)
[info]   at 
org.scalatest.FunSuite.newAssertionFailedException(FunSuite.scala:1560)
[info]   at 
org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:501)
[info]   at 
org.apache.spark.scheduler.cluster.mesos.MesosClusterSchedulerSuite$$anonfun$16.apply(MesosClusterSchedulerSuite.scala:227)
[info]   at 
org.apache.spark.scheduler.cluster.mesos.MesosClusterSchedulerSuite$$anonfun$16.apply(MesosClusterSchedulerSuite.scala:202)
```


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20641: [SPARK-23464][MESOS] Fix mesos cluster scheduler ...

[skonto]

Github user skonto commented on a diff in the pull request:

https://github.com/apache/spark/pull/20641#discussion_r175804550
  
--- Diff: 
resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala
 ---
@@ -199,6 +199,38 @@ class MesosClusterSchedulerSuite extends SparkFunSuite 
with LocalSparkContext wi
 })
   }
 
+  test("properly wraps and escapes parameters passed to driver command") {
--- End diff --

Does this test fail with the old code?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20641: [SPARK-23464][MESOS] Fix mesos cluster scheduler ...

[krcz]

Github user krcz commented on a diff in the pull request:

https://github.com/apache/spark/pull/20641#discussion_r173909575
  
--- Diff: 
resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala
 ---
@@ -199,6 +199,38 @@ class MesosClusterSchedulerSuite extends SparkFunSuite 
with LocalSparkContext wi
 })
   }
 
+  test("properly wraps and escapes parameters passed to driver command") {
+setScheduler()
+
+val mem = 1000
+val cpu = 1
+
+val response = scheduler.submitDriver(
+  new MesosDriverDescription("d1", "jar", mem, cpu, true,
+command,
+Map("spark.mesos.executor.home" -> "test",
+  "spark.app.name" -> "test",
+  // no special characters, wrap only
+  "spark.driver.extraJavaOptions" ->
+"-XX+PrintGC -Dparam1=val1 -Dparam2=val2",
+  // special characters, to be escaped
+  "spark.executor.extraJavaOptions" ->
+"""-Dparam1="value 1" -Dparam2=value\ 2 -Dpath=$PATH"""),
--- End diff --

@susanxhuynh 
I have checked it and it worked. There don't need to be quotes, as the 
space has been escaped. Backslash stops it from being interpreted as a boundary 
between arguments and makes it being understood as simple space in the value.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20641: [SPARK-23464][MESOS] Fix mesos cluster scheduler ...

[susanxhuynh]

Github user susanxhuynh commented on a diff in the pull request:

https://github.com/apache/spark/pull/20641#discussion_r17399
  
--- Diff: 
resource-managers/mesos/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala
 ---
@@ -199,6 +199,38 @@ class MesosClusterSchedulerSuite extends SparkFunSuite 
with LocalSparkContext wi
 })
   }
 
+  test("properly wraps and escapes parameters passed to driver command") {
+setScheduler()
+
+val mem = 1000
+val cpu = 1
+
+val response = scheduler.submitDriver(
+  new MesosDriverDescription("d1", "jar", mem, cpu, true,
+command,
+Map("spark.mesos.executor.home" -> "test",
+  "spark.app.name" -> "test",
+  // no special characters, wrap only
+  "spark.driver.extraJavaOptions" ->
+"-XX+PrintGC -Dparam1=val1 -Dparam2=val2",
+  // special characters, to be escaped
+  "spark.executor.extraJavaOptions" ->
+"""-Dparam1="value 1" -Dparam2=value\ 2 -Dpath=$PATH"""),
--- End diff --

I'm not sure the second param (param2) would be parsed correctly if you 
actually ran the command. Doesn't there need to be quotes around the space? 
Have you tested it and checked if the executor gets the correct value for 
param2?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20641: [SPARK-23464][MESOS] Fix mesos cluster scheduler ...

[krcz]

GitHub user krcz opened a pull request:

https://github.com/apache/spark/pull/20641

[SPARK-23464][MESOS] Fix mesos cluster scheduler options double-escaping

## What changes were proposed in this pull request?

Don't enclose --conf option value with "", as they are already escaped by 
shellEscape
and additional wrapping cancels out this escaping, making the driver fail 
to start.

This reverts commit 9b377aa49f14af31f54164378d60e0fdea2142e5 [SPARK-18114].

## How was this patch tested?

Manual test with driver command logging added.

Author: Marcin Kurczych 



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/krcz/spark mesos-double-escaping

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/20641.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #20641


commit e4c5fb12d732029727ba6772732734c511c1f6fa
Author: Marcin Kurczych 
Date:   2018-02-20T15:12:09Z

[SPARK-23464][MESOS] Fix mesos cluster scheduler options double-escaping

Don't enclose --conf option value with "", as they are already escaped by 
shellEscape
and additional wrapping cancels out this escaping, causing driver fail to 
start.

This reverts commit 9b377aa49f14af31f54164378d60e0fdea2142e5 [SPARK-18114].

Manual test with driver command logging added.

Author: Marcin Kurczych 




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org