You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by krcz <gi...@git.apache.org> on 2018/02/20 15:17:18 UTC

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

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 <ma...@iqvia.com>
    


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 <ma...@...>
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 <ma...@iqvia.com>

----


---

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


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

Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    ok, could you rebase this PR



---

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


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

Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @susanxhuynh I tested this. Here is what you get:
    
    ```
    ./bin/spark-submit --deploy-mode cluster --name test --master mesos://spark.marathon.mesos:13830 --driver-cores 1.0 --driver-memory 1000M --class org.apache.spark.examples.SparkPi --conf spark.executor.extraJavaOptions="-Dparam1=\"value 1\" -Dparam2=value\\ 2 -Dpath=\$PATH" --conf spark.mesos.containerizer=mesos  --conf spark.mesos.executor.docker.image=skonto/spark-test:test https://...jar 10000
    ```
    At the driver side in DC/OS:
    
    ```
    /usr/lib/jvm/jre1.8.0_152/bin/java -cp /opt/spark/dist/conf/:/opt/spark/dist/jars/*:/etc/hadoop/ -Dspark.mesos.driver.frameworkId=bddf96ce-45f6-45f7-ac83-5a3622cafc41-0030-driver-20180612154509-0007 -Xmx1000M org.apache.spark.deploy.SparkSubmit --master mesos://zk://master.mesos:2181/mesos --conf spark.driver.memory=1000M --conf spark.driver.cores=1.0 --conf spark.mesos.executor.docker.image=skonto/spark-test:test --conf spark.app.name=test --conf spark.mesos.containerizer=mesos --conf spark.executor.extraJavaOptions=-Dparam1="value 1" -Dparam2=value\ 2 -Dpath=$PATH --conf spark.driver.supervise=false --class org.apache.spark.examples.SparkPi --name test --driver-cores 1.0 /mnt/mesos/sandbox/....jar 10000
    ```



---

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


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

Posted by krcz <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


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

Posted by susanxhuynh <gi...@git.apache.org>.
Github user susanxhuynh commented on a diff in the pull request:

    https://github.com/apache/spark/pull/20641#discussion_r172222399
  
    --- 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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


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

Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @krcz how is your PR different from 21014?


---

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


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

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    Build finished. Test FAILed.


---

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


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

Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    yap, that sounds like a good idea


---

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


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

Posted by krcz <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


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

Posted by felixb <gi...@git.apache.org>.
Github user felixb commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @krcz 
    @felixcheung <- different felix ;)


---

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


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

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    **[Test build #91820 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91820/testReport)** for PR 20641 at commit [`22c6739`](https://github.com/apache/spark/commit/22c67392b9f920af8fae9de1f7c7f5fb5a25abb8).
     * This patch **fails due to an unknown error code, -9**.
     * This patch **does not merge cleanly**.
     * This patch adds no public classes.


---

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


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

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/91820/
    Test FAILed.


---

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


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

Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @krcz yes it should be ok. So I verified it:
    
    
    Dispatcher passes the following command to mesos:
    ```18/06/13 10:30:45 INFO MesosClusterScheduler: Command for the driver (test):./bin/spark-submit --name test --master mesos://zk://master.mesos:2181/mesos --driver-cores 1.0 --driver-memory 1000M --class org.apache.spark.examples.SparkPi --conf spark.executor.extraJavaOptions="-Dexecutor.test.param1=\"value 1\" -Dexecutor.test.param2=value\\ 2 -Dexecutor.test.path=\$PATH" --conf spark.mesos.containerizer=mesos --conf spark.driver.supervise=false --conf spark.app.name=test --conf spark.driver.memory=1000M --conf
    ```
    spark-class will run this at the executor side:
    ```
    exec /usr/lib/jvm/jre1.8.0_152/bin/java -cp '/opt/spark/dist/conf/:/opt/spark/dist/jars/*:/etc/hadoop/' '-Dexecutor.test.param1=value 1' '-Dexecutor.test.param2=value 2' '-Dexecutor.test.path=$PATH' -Xmx1024m org.apache.spark.executor.CoarseGrainedExecutorBackend --driver-url spark://CoarseGrainedScheduler@ip-10-0-0-77.us-west-1.compute.internal:33887 --executor-id 9 --hostname 10.0.0.77 --cores 3 --app-id bddf96ce-45f6-45f7-ac83-5a3622cafc41-0032-driver-20180613103044-0001
    ```


---

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


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

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    Can one of the admins verify this patch?


---

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


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

Posted by krcz <gi...@git.apache.org>.
Github user krcz commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @skonto The one directly below  "At the driver side in DC/OS:" in your comment. But if it's output of ps that's ok, as `ps` wouldn't be able to discern between `command "foo bar"` and `command foo bar`.


---

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


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

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    Can one of the admins verify this patch?


---

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


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

Posted by susanxhuynh <gi...@git.apache.org>.
Github user susanxhuynh commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @skonto Thanks for testing it. Tests results look good.


---

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


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

Posted by krcz <gi...@git.apache.org>.
Github user krcz commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @felixcheung It fixes the same problem so in terms of implementation it is not very different. But when I created this one 21014 didn't exist.
    
    The only difference is that this PR adds an unit test, which 21014 had not. That's why I'm asking if I should modify this PR to just add the test.


---

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


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

Posted by susanxhuynh <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


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

Posted by blue666man <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


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

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    Can one of the admins verify this patch?


---

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


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

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    **[Test build #91820 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/91820/testReport)** for PR 20641 at commit [`22c6739`](https://github.com/apache/spark/commit/22c67392b9f920af8fae9de1f7c7f5fb5a25abb8).


---

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


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

Posted by krcz <gi...@git.apache.org>.
Github user krcz commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @susanxhuynh Thanks for the example! I have added similar test, covering more cases. After you or someone else reviews it, I'll rebase the pull request.


---

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


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

Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @mgummelt too


---

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


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

Posted by susanxhuynh <gi...@git.apache.org>.
Github user susanxhuynh commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    Thanks for the PR! It seems that the previous attempt to fix this (SPARK-18114) was wrong -- I'm not sure why we didn't catch the problem before, maybe lack of testing? @krcz My suggestion for this patch is to add a test, in order to prevent another regression in the future. I've written a unit test for this -- you could do something similar:  https://github.com/mesosphere/spark/commit/4812ba3d10264f6d22ec654fa16b5810d70c27a9 I will also do more testing with my own integration tests. cc @skonto 


---

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


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

Posted by skonto <gi...@git.apache.org>.
Github user skonto commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @krcz which one? Btw both job run successfully although might not have been escaped properly.


---

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


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

Posted by skonto <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


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

Posted by krcz <gi...@git.apache.org>.
Github user krcz commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @felixb @skonto 
    I've triend rebasing, but it looks that the issue has been meanwhile fixed independently in [other pull request](https://github.com/apache/spark/pull/21014)  while someone was working on [SPARK-23941 issue](https://issues.apache.org/jira/browse/SPARK-23941). It looks that there is no unit test there though, so I can modify his PR to incorporate just the test. Do you want that or should I just close the pull request?


---

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


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

Posted by krcz <gi...@git.apache.org>.
Github user krcz commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    @skonto 
    The command you have pasted as "At the driver side in DC/OS:" concerns me. It doesn't seem escaped properly.  Is it the command to be run by shell or is it just space-joined arguments list (as in Spark Dispatcher web UI), which would explain why it is not escaped?


---

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


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

Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    ok to test


---

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


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

Posted by adyatlov <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


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

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/20641
  
    Can one of the admins verify this patch?


---

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


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

Posted by skonto <gi...@git.apache.org>.
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-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org