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

[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

GitHub user mgummelt opened a pull request:

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

    [SPARK-16637] Unified containerizer

    ## What changes were proposed in this pull request?
    
    New config var: spark.mesos.docker.containerizer={"mesos","docker" (default)}
    
    This adds support for running docker containers via the Mesos unified containerizer: http://mesos.apache.org/documentation/latest/container-image/
    
    The benefit is losing the dependency on `dockerd`, and all the costs which it incurs.
    
    I've also updated the supported Mesos version to 0.28.2 for support of the required protobufs.
    
    ## How was this patch tested?
    
    - manually testing jobs submitted with both "mesos" and "docker" settings for the new config var.
    - spark/mesos integration test suite


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

    $ git pull https://github.com/mesosphere/spark unified-containerizer

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

    https://github.com/apache/spark/pull/14275.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 #14275
    
----
commit 77bd01a89400fcf1a1481e134ea9900efb661b15
Author: Michael Gummelt <mg...@mesosphere.io>
Date:   2016-07-12T22:42:40Z

    refactor

commit 5d37547f5c25dc42d2b0ecc299f1f8966b762eee
Author: Michael Gummelt <mg...@mesosphere.io>
Date:   2016-07-13T01:04:00Z

    tests

commit f113ef7eb461b6230d195f92482a0580423719a1
Author: Michael Gummelt <mg...@mesosphere.io>
Date:   2016-07-13T19:00:43Z

    whitespace fixes

commit c1f29689476490c2b59a895e69dba0f2236e1f3a
Author: Michael Gummelt <mg...@mesosphere.io>
Date:   2016-07-14T18:15:45Z

    style changes

commit 8e0e4110ea8d1e2abe16340404e45e3266366dc6
Author: Michael Gummelt <mg...@mesosphere.io>
Date:   2016-07-14T19:08:16Z

    rename to spark.mesos.driverEnv

commit 4aac0bafd820a66c5038d9dbec32f5366c3e623f
Author: Michael Gummelt <mg...@mesosphere.io>
Date:   2016-07-15T22:08:39Z

    style

commit e0fe77363f6b050393389c532859ccc00529ce91
Author: Michael Gummelt <mg...@mesosphere.io>
Date:   2016-07-15T22:55:05Z

    unified containerizer

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71868244
  
    --- Diff: core/src/main/scala/org/apache/spark/TaskState.scala ---
    @@ -49,5 +49,6 @@ private[spark] object TaskState extends Enumeration {
         case MesosTaskState.TASK_KILLED => KILLED
         case MesosTaskState.TASK_LOST => LOST
         case MesosTaskState.TASK_ERROR => LOST
    +    case MesosTaskState.TASK_KILLING => RUNNING
    --- End diff --
    
    Is it maybe clearer to use `case x | y | z => foo` syntax here to show clearly which several cases map to one output state? here and above. Either that, or at least loosely group the inputs; KILLING seems like it should be next to KILLED


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62937 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62937/consoleFull)** for PR 14275 at commit [`82359ba`](https://github.com/apache/spark/commit/82359bac39f4526c35b084a20755d69a4574670a).
     * This patch **fails build dependency tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Serializable `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62684 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62684/consoleFull)** for PR 14275 at commit [`53d01ec`](https://github.com/apache/spark/commit/53d01ecf8e4ac0d26afd1dd0de495df7ae96dd55).
     * This patch **fails build dependency tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71749653
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -105,16 +105,25 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
       def addDockerInfo(
           container: ContainerInfo.Builder,
           image: String,
    +      containerizer: String,
           volumes: Option[List[Volume]] = None,
    -      network: Option[ContainerInfo.DockerInfo.Network] = None,
           portmaps: Option[List[ContainerInfo.DockerInfo.PortMapping]] = None): Unit = {
    --- End diff --
    
    Yea, this is effectively dead.  I'll leave it to a future PR to remove.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    I tested it against our test suite it works fine. Have you tested it with an image: spark.mesos.executor.docker.image? Do you have a sample image?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71869084
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -131,9 +140,18 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
         val portmaps = conf
           .getOption("spark.mesos.executor.docker.portmaps")
           .map(parsePortMappingsSpec)
    +
    +    val containerizer = conf.get("spark.mesos.containerizer", "docker")
    +    if (!List("docker", "mesos").contains(containerizer)) {
    --- End diff --
    
    This might become obsolete then, not sure. But consider using `require` in any event


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71452287
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -47,7 +47,7 @@ import org.apache.spark.util.Utils
      *
      * @param loadDefaults whether to also load values from Java system properties
      */
    -class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
    +class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Serializable {
    --- End diff --
    
    Added `Serializable` so that `SparkConf` can be persisted to ZK


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71761953
  
    --- Diff: core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosCoarseGrainedSchedulerBackendSuite.scala ---
    @@ -106,10 +106,10 @@ class MesosCoarseGrainedSchedulerBackendSuite extends SparkFunSuite
         val offers = List((executorMemory * 2, executorCores + 1))
         offerResources(offers)
     
    -    val taskInfos = verifyTaskLaunched("o1")
    -    assert(taskInfos.size() == 1)
    +    val taskInfos = verifyTaskLaunched(driver, "o1")
    +    assert(taskInfos.length == 1)
     
    -    val cpus = backend.getResource(taskInfos.iterator().next().getResourcesList, "cpus")
    +    val cpus = backend.getResource(taskInfos(0).getResourcesList, "cpus")
    --- End diff --
    
    fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71870499
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    schedulerProperties: Map[String, String],
    --- End diff --
    
    I'm probably missing your point but would that affect `val`? it might be `private` too but this change just removes `val`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62840 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62840/consoleFull)** for PR 14275 at commit [`44c50d4`](https://github.com/apache/spark/commit/44c50d4cf5784fd3a91accc3087a142ee4b06e92).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71674856
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerUtils.scala ---
    @@ -79,7 +80,7 @@ private[mesos] trait MesosSchedulerUtils extends Logging {
           credBuilder.setPrincipal(principal)
         }
         conf.getOption("spark.mesos.secret").foreach { secret =>
    -      credBuilder.setSecret(ByteString.copyFromUtf8(secret))
    +      credBuilder.setSecret(secret)
    --- End diff --
    
    That is for updating dependencies. I was surprised that only that change was needed :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    good eye.  fixed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    @srowen Could you please merge?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62694 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62694/consoleFull)** for PR 14275 at commit [`085e228`](https://github.com/apache/spark/commit/085e228f54cb006759b4646c48db3a0dfae2678b).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71687752
  
    --- Diff: core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala ---
    @@ -31,17 +31,24 @@ import org.scalatest.mock.MockitoSugar
     import org.apache.spark.{LocalSparkContext, SparkConf, SparkFunSuite}
     import org.apache.spark.deploy.Command
     import org.apache.spark.deploy.mesos.MesosDriverDescription
    -
    +import org.apache.spark.scheduler.cluster.mesos.Utils
     
     class MesosClusterSchedulerSuite extends SparkFunSuite with LocalSparkContext with MockitoSugar {
     
       private val command = new Command("mainClass", Seq("arg"), Map(), Seq(), Seq(), Seq())
    --- End diff --
    
    new is redundant


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    @srowen Could you pls check and merge?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71868130
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    schedulerProperties: Map[String, String],
    --- End diff --
    
    Why can't this be `val` BTW?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71869025
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -105,16 +105,25 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
       def addDockerInfo(
           container: ContainerInfo.Builder,
           image: String,
    +      containerizer: String,
           volumes: Option[List[Volume]] = None,
    -      network: Option[ContainerInfo.DockerInfo.Network] = None,
           portmaps: Option[List[ContainerInfo.DockerInfo.PortMapping]] = None): Unit = {
    -
    -    val docker = ContainerInfo.DockerInfo.newBuilder().setImage(image)
    -
    -    network.foreach(docker.setNetwork)
    -    portmaps.foreach(_.foreach(docker.addPortMappings))
    -    container.setType(ContainerInfo.Type.DOCKER)
    -    container.setDocker(docker.build())
    +    val dockerContainerizer = containerizer == "docker"
    +    if (dockerContainerizer) {
    --- End diff --
    
    `containerizer match { ...` is another way to write this, which might at least let you easily throw a no-match exception if it's an unsupported value. No big deal


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71687133
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala ---
    @@ -478,6 +495,33 @@ private[spark] class MesosClusterScheduler(
         }
       }
     
    +  private def createTaskInfo(desc: MesosDriverDescription, offer: ResourceOffer): TaskInfo = {
    +    val taskId = TaskID.newBuilder().setValue(desc.submissionId).build()
    +
    +    val (remainingResources, cpuResourcesToUse) =
    +      partitionResources(offer.resources, "cpus", desc.cores)
    +    val (finalResources, memResourcesToUse) =
    +      partitionResources(remainingResources.asJava, "mem", desc.mem)
    +    offer.resources = finalResources.asJava
    +
    +    val appName = desc.sc.get("spark.app.name")
    +    val taskInfo = TaskInfo.newBuilder()
    +      .setTaskId(taskId)
    +      .setName(s"Driver for ${appName}")
    --- End diff --
    
    I find it redundant to use {} anyway thats my view.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r72141303
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -105,16 +105,27 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
       def addDockerInfo(
           container: ContainerInfo.Builder,
           image: String,
    +      containerizer: String,
           volumes: Option[List[Volume]] = None,
    -      network: Option[ContainerInfo.DockerInfo.Network] = None,
           portmaps: Option[List[ContainerInfo.DockerInfo.PortMapping]] = None): Unit = {
     
    -    val docker = ContainerInfo.DockerInfo.newBuilder().setImage(image)
    +    containerizer match {
    --- End diff --
    
    done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71915660
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -105,16 +105,25 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
       def addDockerInfo(
           container: ContainerInfo.Builder,
           image: String,
    +      containerizer: String,
           volumes: Option[List[Volume]] = None,
    -      network: Option[ContainerInfo.DockerInfo.Network] = None,
           portmaps: Option[List[ContainerInfo.DockerInfo.PortMapping]] = None): Unit = {
    -
    -    val docker = ContainerInfo.DockerInfo.newBuilder().setImage(image)
    -
    -    network.foreach(docker.setNetwork)
    -    portmaps.foreach(_.foreach(docker.addPortMappings))
    -    container.setType(ContainerInfo.Type.DOCKER)
    -    container.setDocker(docker.build())
    +    val dockerContainerizer = containerizer == "docker"
    +    if (dockerContainerizer) {
    --- End diff --
    
    good idea. fixed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62841 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62841/consoleFull)** for PR 14275 at commit [`4a0af2e`](https://github.com/apache/spark/commit/4a0af2e6f6224ec85df8dc690cac72a799c39e29).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71687654
  
    --- Diff: core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala ---
    @@ -31,17 +31,24 @@ import org.scalatest.mock.MockitoSugar
     import org.apache.spark.{LocalSparkContext, SparkConf, SparkFunSuite}
     import org.apache.spark.deploy.Command
     import org.apache.spark.deploy.mesos.MesosDriverDescription
    -
    +import org.apache.spark.scheduler.cluster.mesos.Utils
    --- End diff --
    
    unused import...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62735 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62735/consoleFull)** for PR 14275 at commit [`be145c7`](https://github.com/apache/spark/commit/be145c788806507992402f0ed3b2a9c631374d51).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71688940
  
    --- Diff: core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosCoarseGrainedSchedulerBackendSuite.scala ---
    @@ -120,10 +120,10 @@ class MesosCoarseGrainedSchedulerBackendSuite extends SparkFunSuite
         val offerCores = 10
         offerResources(List((executorMemory * 2, offerCores)))
     
    -    val taskInfos = verifyTaskLaunched("o1")
    -    assert(taskInfos.size() == 1)
    +    val taskInfos = verifyTaskLaunched(driver, "o1")
    +    assert(taskInfos.length == 1)
     
    -    val cpus = backend.getResource(taskInfos.iterator().next().getResourcesList, "cpus")
    +    val cpus = backend.getResource(taskInfos(0).getResourcesList, "cpus")
    --- End diff --
    
    taskInfos.head


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    @skonto rebased and ready for re-review


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71679591
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -370,6 +370,12 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
         settings.entrySet().asScala.map(x => (x.getKey, x.getValue)).toArray
       }
     
    +  def getAll(prefix: String): Array[(String, String)] = {
    --- End diff --
    
    Document the method please.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62684 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62684/consoleFull)** for PR 14275 at commit [`53d01ec`](https://github.com/apache/spark/commit/53d01ecf8e4ac0d26afd1dd0de495df7ae96dd55).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62937 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62937/consoleFull)** for PR 14275 at commit [`82359ba`](https://github.com/apache/spark/commit/82359bac39f4526c35b084a20755d69a4574670a).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71870262
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    schedulerProperties: Map[String, String],
    --- End diff --
    
    probably because its not used outside the class from what i recall.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    OK, looks good to me too though I don't really know about Mesos. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    LGTM.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71745913
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -370,6 +370,12 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
         settings.entrySet().asScala.map(x => (x.getKey, x.getValue)).toArray
       }
     
    +  def getAll(prefix: String): Array[(String, String)] = {
    --- End diff --
    
    done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71687466
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -131,9 +140,18 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
         val portmaps = conf
           .getOption("spark.mesos.executor.docker.portmaps")
           .map(parsePortMappingsSpec)
    +
    +    val containerizer = conf.get("spark.mesos.docker.containerizer", "docker")
    +    if (!List("docker", "mesos").contains(containerizer)) {
    +      throw new IllegalArgumentException(
    +        """spark.mesos.docker.containerizer must be one of {"docker", "mesos"}.""" +
    +          s"  You provided ${containerizer}")
    --- End diff --
    
    {}


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71915307
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    schedulerProperties: Map[String, String],
    --- End diff --
    
    Removing `val` means `schedulerProperties` is not saved as a field of the class.  It's just used during construction.  I could also make this `private val` if that's more standard in this project.  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71912447
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -47,7 +47,7 @@ import org.apache.spark.util.Utils
      *
      * @param loadDefaults whether to also load values from Java system properties
      */
    -class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
    +class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Serializable {
    --- End diff --
    
    fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62695 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62695/consoleFull)** for PR 14275 at commit [`5bb1a35`](https://github.com/apache/spark/commit/5bb1a35262e207f9c3f85f856b78e4db21792bed).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71985992
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    schedulerProperties: Map[String, String],
    --- End diff --
    
    Oh right, this is what causes it to become a field. Yeah I suspect lots and lots of things in the code should not even be `val`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71747044
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala ---
    @@ -353,43 +353,62 @@ private[spark] class MesosClusterScheduler(
         }
       }
     
    -  private def buildDriverCommand(desc: MesosDriverDescription): CommandInfo = {
    -    val appJar = CommandInfo.URI.newBuilder()
    -      .setValue(desc.jarUrl.stripPrefix("file:").stripPrefix("local:")).build()
    -    val builder = CommandInfo.newBuilder().addUris(appJar)
    -    val entries = conf.getOption("spark.executor.extraLibraryPath")
    -      .map(path => Seq(path) ++ desc.command.libraryPathEntries)
    -      .getOrElse(desc.command.libraryPathEntries)
    -
    -    val prefixEnv = if (!entries.isEmpty) {
    -      Utils.libraryPathEnvPrefix(entries)
    -    } else {
    -      ""
    +  private def getDriverExecutorURI(desc: MesosDriverDescription): Option[String] = {
    +    desc.sc.getOption("spark.executor.uri")
    +        .orElse(desc.command.environment.get("SPARK_EXECUTOR_URI"))
    +  }
    +
    +  private def getDriverEnvironment(desc: MesosDriverDescription): Environment = {
    +    val env = {
    +      val executorOpts = desc.sc.getAll.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
    +      val executorEnv = Map("SPARK_EXECUTOR_OPTS" -> executorOpts)
    +      val driverEnv = desc.sc.getAll("spark.mesos.driverEnv")
    +
    +      driverEnv ++ executorEnv ++ desc.command.environment
         }
    +
         val envBuilder = Environment.newBuilder()
    -    desc.command.environment.foreach { case (k, v) =>
    -      envBuilder.addVariables(Variable.newBuilder().setName(k).setValue(v).build())
    +    env.foreach { case (k, v) =>
    +      envBuilder.addVariables(Variable.newBuilder().setName(k).setValue(v))
         }
    -    // Pass all spark properties to executor.
    -    val executorOpts = desc.schedulerProperties.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
    -    envBuilder.addVariables(
    -      Variable.newBuilder().setName("SPARK_EXECUTOR_OPTS").setValue(executorOpts))
    -    val dockerDefined = desc.schedulerProperties.contains("spark.mesos.executor.docker.image")
    -    val executorUri = desc.schedulerProperties.get("spark.executor.uri")
    -      .orElse(desc.command.environment.get("SPARK_EXECUTOR_URI"))
    +    envBuilder.build()
    +  }
    +
    +  private def getDriverUris(desc: MesosDriverDescription): List[CommandInfo.URI] = {
    +    val confUris = List(conf.getOption("spark.mesos.uris"),
    +      desc.sc.getOption("spark.mesos.uris"),
    +      desc.sc.getOption("spark.submit.pyFiles")).flatMap(
    +      _.map(_.split(",").map(_.trim))
    +    ).flatten
    +
    +    val jarUrl = desc.jarUrl.stripPrefix("file:").stripPrefix("local:")
    +
    +    ((jarUrl :: confUris) ++ getDriverExecutorURI(desc).toList).map(uri =>
    +      CommandInfo.URI.newBuilder().setValue(uri.trim()).build())
    +  }
    +
    +  private def getDriverCommandValue(desc: MesosDriverDescription): String = {
    +    val dockerDefined = desc.sc.contains("spark.mesos.executor.docker.image")
    +    val executorUri = getDriverExecutorURI(desc)
         // Gets the path to run spark-submit, and the path to the Mesos sandbox.
         val (executable, sandboxPath) = if (dockerDefined) {
           // Application jar is automatically downloaded in the mounted sandbox by Mesos,
           // and the path to the mounted volume is stored in $MESOS_SANDBOX env variable.
           ("./bin/spark-submit", "$MESOS_SANDBOX")
         } else if (executorUri.isDefined) {
    -      builder.addUris(CommandInfo.URI.newBuilder().setValue(executorUri.get).build())
           val folderBasename = executorUri.get.split('/').last.split('.').head
    +
    +      val entries = conf.getOption("spark.executor.extraLibraryPath")
    +        .map(path => Seq(path) ++ desc.command.libraryPathEntries)
    +        .getOrElse(desc.command.libraryPathEntries)
    +
    +      val prefixEnv = if (!entries.isEmpty) Utils.libraryPathEnvPrefix(entries) else ""
    --- End diff --
    
    This is part of the driverEnv PR.  This part will go away once that gets merged into master.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71868805
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -47,7 +47,7 @@ import org.apache.spark.util.Utils
      *
      * @param loadDefaults whether to also load values from Java system properties
      */
    -class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
    +class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Serializable {
    --- End diff --
    
    Interesting, this wasn't already serializable eh. Normally that might be a significant choice but this just contains a concurrent map, so seems OK. (And this field `avroNamespace` which should have been in the `object`)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71809553
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -105,16 +105,25 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
       def addDockerInfo(
           container: ContainerInfo.Builder,
           image: String,
    +      containerizer: String,
           volumes: Option[List[Volume]] = None,
    -      network: Option[ContainerInfo.DockerInfo.Network] = None,
           portmaps: Option[List[ContainerInfo.DockerInfo.PortMapping]] = None): Unit = {
    --- End diff --
    
    ok just a quick check i will do and will be fine from my side.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71688894
  
    --- Diff: core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosCoarseGrainedSchedulerBackendSuite.scala ---
    @@ -106,10 +106,10 @@ class MesosCoarseGrainedSchedulerBackendSuite extends SparkFunSuite
         val offers = List((executorMemory * 2, executorCores + 1))
         offerResources(offers)
     
    -    val taskInfos = verifyTaskLaunched("o1")
    -    assert(taskInfos.size() == 1)
    +    val taskInfos = verifyTaskLaunched(driver, "o1")
    +    assert(taskInfos.length == 1)
     
    -    val cpus = backend.getResource(taskInfos.iterator().next().getResourcesList, "cpus")
    +    val cpus = backend.getResource(taskInfos(0).getResourcesList, "cpus")
    --- End diff --
    
    taskInfos.head


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62685 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62685/consoleFull)** for PR 14275 at commit [`c02b7c7`](https://github.com/apache/spark/commit/c02b7c7ec14227700576124c00827100f35eee2a).
     * This patch **fails build dependency tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62695 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62695/consoleFull)** for PR 14275 at commit [`5bb1a35`](https://github.com/apache/spark/commit/5bb1a35262e207f9c3f85f856b78e4db21792bed).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Serializable `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62569 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62569/consoleFull)** for PR 14275 at commit [`e0fe773`](https://github.com/apache/spark/commit/e0fe77363f6b050393389c532859ccc00529ce91).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71746880
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/ui/DriverPage.scala ---
    @@ -50,7 +50,7 @@ private[ui] class DriverPage(parent: MesosClusterUI) extends WebUIPage("driver")
         val driverDescription = Iterable.apply(driverState.description)
         val submissionState = Iterable.apply(driverState.submissionState)
         val command = Iterable.apply(driverState.description.command)
    -    val schedulerProperties = Iterable.apply(driverState.description.schedulerProperties)
    +    val schedulerProperties = Iterable.apply(driverState.description.sc.getAll.toMap)
    --- End diff --
    
    `conf` is still namespaced under `driverState`, so I think it's still just as clear that these are driver properties.  In fact, maybe even moreso, since "scheduler" is ambiguous, as both the dispatcher and the driver are schedulers.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71912703
  
    --- Diff: core/src/main/scala/org/apache/spark/TaskState.scala ---
    @@ -49,5 +49,6 @@ private[spark] object TaskState extends Enumeration {
         case MesosTaskState.TASK_KILLED => KILLED
         case MesosTaskState.TASK_LOST => LOST
         case MesosTaskState.TASK_ERROR => LOST
    +    case MesosTaskState.TASK_KILLING => RUNNING
    --- End diff --
    
    good idea. fixed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71745795
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -47,7 +47,7 @@ import org.apache.spark.util.Utils
      *
      * @param loadDefaults whether to also load values from Java system properties
      */
    -class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
    +class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Serializable {
    --- End diff --
    
    Yea: https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterPersistenceEngine.scala#L111


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71978483
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    schedulerProperties: Map[String, String],
    --- End diff --
    
    That was my point too above. No public getters generated if no val is used and this can be justified if the field is not used outside of the class.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71786978
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -105,16 +105,25 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
       def addDockerInfo(
           container: ContainerInfo.Builder,
           image: String,
    +      containerizer: String,
           volumes: Option[List[Volume]] = None,
    -      network: Option[ContainerInfo.DockerInfo.Network] = None,
           portmaps: Option[List[ContainerInfo.DockerInfo.PortMapping]] = None): Unit = {
    --- End diff --
    
    You can use ours: https://github.com/mesosphere/universe/blob/version-3.x/repo/packages/S/spark/14/resource.json#L5
    
    The Spark dist is located at /opt/spark/dist
    
    I've also already tested this manually.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62735 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62735/consoleFull)** for PR 14275 at commit [`be145c7`](https://github.com/apache/spark/commit/be145c788806507992402f0ed3b2a9c631374d51).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71915701
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -131,9 +140,18 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
         val portmaps = conf
           .getOption("spark.mesos.executor.docker.portmaps")
           .map(parsePortMappingsSpec)
    +
    +    val containerizer = conf.get("spark.mesos.containerizer", "docker")
    +    if (!List("docker", "mesos").contains(containerizer)) {
    --- End diff --
    
    removed in favor of the `match` above


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71684671
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    _schedulerProperties: Map[String, String],
         val submissionId: String,
         val submissionDate: Date,
         val retryState: Option[MesosClusterRetryState] = None)
       extends Serializable {
     
    +  val sc = new SparkConf(false)
    +  _schedulerProperties.foreach {case (k, v) => sc.set(k, v)}
    +
       def copy(
           name: String = name,
           jarUrl: String = jarUrl,
           mem: Int = mem,
           cores: Double = cores,
           supervise: Boolean = supervise,
           command: Command = command,
    -      schedulerProperties: Map[String, String] = schedulerProperties,
    +      schedulerProperties: SparkConf = sc,
    --- End diff --
    
    Can you point me there?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71786430
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -105,16 +105,25 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
       def addDockerInfo(
           container: ContainerInfo.Builder,
           image: String,
    +      containerizer: String,
           volumes: Option[List[Volume]] = None,
    -      network: Option[ContainerInfo.DockerInfo.Network] = None,
           portmaps: Option[List[ContainerInfo.DockerInfo.PortMapping]] = None): Unit = {
    --- End diff --
    
    Ok cool we clarified that, there is a lot of confusion out there. How about the image is there one i can use to test it quickly? I really want to do that before reporting its good.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71868873
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -370,6 +370,13 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
         settings.entrySet().asScala.map(x => (x.getKey, x.getValue)).toArray
       }
     
    +  /** Get all parameters that start with `prefix` */
    +  def getAll(prefix: String): Array[(String, String)] = {
    --- End diff --
    
    No big deal, but getAllWithPrefix or something? rather than overload with a single String arg


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71869149
  
    --- Diff: dev/deps/spark-deps-hadoop-2.2 ---
    @@ -116,7 +116,7 @@ libfb303-0.9.2.jar
     libthrift-0.9.2.jar
     log4j-1.2.17.jar
     lz4-1.3.0.jar
    -mesos-0.21.1-shaded-protobuf.jar
    +mesos-0.28.2-shaded-protobuf.jar
    --- End diff --
    
    Are there any other known implications to bumping through 7 minor releases of Mesos? that seems worth vetting


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    cc @skonto 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71671939
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -47,7 +47,7 @@ import org.apache.spark.util.Utils
      *
      * @param loadDefaults whether to also load values from Java system properties
      */
    -class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
    +class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Serializable {
    --- End diff --
    
    Is this user somewhere by a MesosClusterPersistenceEngineFactory instance or somewhere else?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71746197
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    _schedulerProperties: Map[String, String],
    --- End diff --
    
    I removed the underscore.  I was thinking python for some reason.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62840 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62840/consoleFull)** for PR 14275 at commit [`44c50d4`](https://github.com/apache/spark/commit/44c50d4cf5784fd3a91accc3087a142ee4b06e92).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62841 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62841/consoleFull)** for PR 14275 at commit [`4a0af2e`](https://github.com/apache/spark/commit/4a0af2e6f6224ec85df8dc690cac72a799c39e29).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71686283
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala ---
    @@ -434,19 +451,19 @@ private[spark] class MesosClusterScheduler(
           options ++= Seq("--class", desc.command.mainClass)
         }
     
    -    desc.schedulerProperties.get("spark.executor.memory").map { v =>
    +    desc.sc.getOption("spark.executor.memory").map { v =>
           options ++= Seq("--executor-memory", v)
         }
    -    desc.schedulerProperties.get("spark.cores.max").map { v =>
    +    desc.sc.getOption("spark.cores.max").map { v =>
           options ++= Seq("--total-executor-cores", v)
         }
    -    desc.schedulerProperties.get("spark.submit.pyFiles").map { pyFiles =>
    +    desc.sc.getOption("spark.submit.pyFiles").map { pyFiles =>
    --- End diff --
    
    same


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62939 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62939/consoleFull)** for PR 14275 at commit [`ec6707e`](https://github.com/apache/spark/commit/ec6707e35784113896063f5b5aec042e2e54ecb1).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71686268
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala ---
    @@ -434,19 +451,19 @@ private[spark] class MesosClusterScheduler(
           options ++= Seq("--class", desc.command.mainClass)
         }
     
    -    desc.schedulerProperties.get("spark.executor.memory").map { v =>
    +    desc.sc.getOption("spark.executor.memory").map { v =>
           options ++= Seq("--executor-memory", v)
         }
    -    desc.schedulerProperties.get("spark.cores.max").map { v =>
    +    desc.sc.getOption("spark.cores.max").map { v =>
    --- End diff --
    
    same


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71688958
  
    --- Diff: core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosCoarseGrainedSchedulerBackendSuite.scala ---
    @@ -134,10 +134,10 @@ class MesosCoarseGrainedSchedulerBackendSuite extends SparkFunSuite
         val executorMemory = backend.executorMemory(sc)
         offerResources(List((executorMemory, maxCores + 1)))
     
    -    val taskInfos = verifyTaskLaunched("o1")
    -    assert(taskInfos.size() == 1)
    +    val taskInfos = verifyTaskLaunched(driver, "o1")
    +    assert(taskInfos.length == 1)
     
    -    val cpus = backend.getResource(taskInfos.iterator().next().getResourcesList, "cpus")
    +    val cpus = backend.getResource(taskInfos(0).getResourcesList, "cpus")
    --- End diff --
    
    taskInfos.head


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71686260
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala ---
    @@ -434,19 +451,19 @@ private[spark] class MesosClusterScheduler(
           options ++= Seq("--class", desc.command.mainClass)
         }
     
    -    desc.schedulerProperties.get("spark.executor.memory").map { v =>
    +    desc.sc.getOption("spark.executor.memory").map { v =>
    --- End diff --
    
    foreach


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Looks like one more last comment, and needs a rebase after https://github.com/apache/spark/pull/13051


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71680547
  
    --- Diff: core/src/main/scala/org/apache/spark/TaskState.scala ---
    @@ -49,5 +49,6 @@ private[spark] object TaskState extends Enumeration {
         case MesosTaskState.TASK_KILLED => KILLED
         case MesosTaskState.TASK_LOST => LOST
         case MesosTaskState.TASK_ERROR => LOST
    +    case MesosTaskState.TASK_KILLING => RUNNING
    --- End diff --
    
    :+1: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    @srowen could you merge please?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71675287
  
    --- Diff: dev/make-distribution.sh ---
    @@ -156,7 +156,7 @@ BUILD_COMMAND=("$MVN" -T 1C clean package -DskipTests $@)
     echo -e "\nBuilding with..."
     echo -e "\$ ${BUILD_COMMAND[@]}\n"
     
    -"${BUILD_COMMAND[@]}"
    +# "${BUILD_COMMAND[@]}"
    --- End diff --
    
    why remove that?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71916241
  
    --- Diff: dev/deps/spark-deps-hadoop-2.2 ---
    @@ -116,7 +116,7 @@ libfb303-0.9.2.jar
     libthrift-0.9.2.jar
     log4j-1.2.17.jar
     lz4-1.3.0.jar
    -mesos-0.21.1-shaded-protobuf.jar
    +mesos-0.28.2-shaded-protobuf.jar
    --- End diff --
    
    It's much less of an issue than in other projects, because the library itself talking to the Mesos master.  Libmesos is.  The risk here is incompatibility with existing libmesos.  That layer is actually extremely stable.  I haven't seen any incompatibilities in any of our frameworks.  But once Mesos releases 1.0 (it's in RC now), this should be less of an issue.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62686 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62686/consoleFull)** for PR 14275 at commit [`ebaef05`](https://github.com/apache/spark/commit/ebaef05d6c466b2a9c73cbabc8c041229bf819f0).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62569 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62569/consoleFull)** for PR 14275 at commit [`e0fe773`](https://github.com/apache/spark/commit/e0fe77363f6b050393389c532859ccc00529ce91).
     * This patch **fails build dependency tests**.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging with Serializable `


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62694 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62694/consoleFull)** for PR 14275 at commit [`085e228`](https://github.com/apache/spark/commit/085e228f54cb006759b4646c48db3a0dfae2678b).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r72003350
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -105,16 +105,27 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
       def addDockerInfo(
           container: ContainerInfo.Builder,
           image: String,
    +      containerizer: String,
           volumes: Option[List[Volume]] = None,
    -      network: Option[ContainerInfo.DockerInfo.Network] = None,
           portmaps: Option[List[ContainerInfo.DockerInfo.PortMapping]] = None): Unit = {
     
    -    val docker = ContainerInfo.DockerInfo.newBuilder().setImage(image)
    +    containerizer match {
    --- End diff --
    
    Can we have a sensible message/exception when we pass in a unknown containerizer? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71747570
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala ---
    @@ -434,19 +451,19 @@ private[spark] class MesosClusterScheduler(
           options ++= Seq("--class", desc.command.mainClass)
         }
     
    -    desc.schedulerProperties.get("spark.executor.memory").map { v =>
    +    desc.sc.getOption("spark.executor.memory").map { v =>
    --- End diff --
    
    fixed


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71685603
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/ui/DriverPage.scala ---
    @@ -50,7 +50,7 @@ private[ui] class DriverPage(parent: MesosClusterUI) extends WebUIPage("driver")
         val driverDescription = Iterable.apply(driverState.description)
         val submissionState = Iterable.apply(driverState.submissionState)
         val command = Iterable.apply(driverState.description.command)
    -    val schedulerProperties = Iterable.apply(driverState.description.schedulerProperties)
    +    val schedulerProperties = Iterable.apply(driverState.description.sc.getAll.toMap)
    --- End diff --
    
    One thing that concerns me besides this being a bit verbose is that it does not provide whose properties these are. 
    It would be ok i guess if sc properties are propagated without any modification to all involved components, is this the case?



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71750078
  
    --- Diff: dev/make-distribution.sh ---
    @@ -156,7 +156,7 @@ BUILD_COMMAND=("$MVN" -T 1C clean package -DskipTests $@)
     echo -e "\nBuilding with..."
     echo -e "\$ ${BUILD_COMMAND[@]}\n"
     
    -"${BUILD_COMMAND[@]}"
    +# "${BUILD_COMMAND[@]}"
    --- End diff --
    
    whoops.  I do this during development so I can get a distribution AND use incremental builds.  removed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71912466
  
    --- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
    @@ -370,6 +370,13 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
         settings.entrySet().asScala.map(x => (x.getKey, x.getValue)).toArray
       }
     
    +  /** Get all parameters that start with `prefix` */
    +  def getAll(prefix: String): Array[(String, String)] = {
    --- End diff --
    
    fixed



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71674445
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -131,9 +140,18 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
         val portmaps = conf
           .getOption("spark.mesos.executor.docker.portmaps")
           .map(parsePortMappingsSpec)
    +
    +    val containerizer = conf.get("spark.mesos.docker.containerizer", "docker")
    --- End diff --
    
    I think this should be: spark.mesos.containerizer. Otherwise its confusing.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62686 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62686/consoleFull)** for PR 14275 at commit [`ebaef05`](https://github.com/apache/spark/commit/ebaef05d6c466b2a9c73cbabc8c041229bf819f0).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71686150
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterScheduler.scala ---
    @@ -353,43 +353,62 @@ private[spark] class MesosClusterScheduler(
         }
       }
     
    -  private def buildDriverCommand(desc: MesosDriverDescription): CommandInfo = {
    -    val appJar = CommandInfo.URI.newBuilder()
    -      .setValue(desc.jarUrl.stripPrefix("file:").stripPrefix("local:")).build()
    -    val builder = CommandInfo.newBuilder().addUris(appJar)
    -    val entries = conf.getOption("spark.executor.extraLibraryPath")
    -      .map(path => Seq(path) ++ desc.command.libraryPathEntries)
    -      .getOrElse(desc.command.libraryPathEntries)
    -
    -    val prefixEnv = if (!entries.isEmpty) {
    -      Utils.libraryPathEnvPrefix(entries)
    -    } else {
    -      ""
    +  private def getDriverExecutorURI(desc: MesosDriverDescription): Option[String] = {
    +    desc.sc.getOption("spark.executor.uri")
    +        .orElse(desc.command.environment.get("SPARK_EXECUTOR_URI"))
    +  }
    +
    +  private def getDriverEnvironment(desc: MesosDriverDescription): Environment = {
    +    val env = {
    +      val executorOpts = desc.sc.getAll.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
    +      val executorEnv = Map("SPARK_EXECUTOR_OPTS" -> executorOpts)
    +      val driverEnv = desc.sc.getAll("spark.mesos.driverEnv")
    +
    +      driverEnv ++ executorEnv ++ desc.command.environment
         }
    +
         val envBuilder = Environment.newBuilder()
    -    desc.command.environment.foreach { case (k, v) =>
    -      envBuilder.addVariables(Variable.newBuilder().setName(k).setValue(v).build())
    +    env.foreach { case (k, v) =>
    +      envBuilder.addVariables(Variable.newBuilder().setName(k).setValue(v))
         }
    -    // Pass all spark properties to executor.
    -    val executorOpts = desc.schedulerProperties.map { case (k, v) => s"-D$k=$v" }.mkString(" ")
    -    envBuilder.addVariables(
    -      Variable.newBuilder().setName("SPARK_EXECUTOR_OPTS").setValue(executorOpts))
    -    val dockerDefined = desc.schedulerProperties.contains("spark.mesos.executor.docker.image")
    -    val executorUri = desc.schedulerProperties.get("spark.executor.uri")
    -      .orElse(desc.command.environment.get("SPARK_EXECUTOR_URI"))
    +    envBuilder.build()
    +  }
    +
    +  private def getDriverUris(desc: MesosDriverDescription): List[CommandInfo.URI] = {
    +    val confUris = List(conf.getOption("spark.mesos.uris"),
    +      desc.sc.getOption("spark.mesos.uris"),
    +      desc.sc.getOption("spark.submit.pyFiles")).flatMap(
    +      _.map(_.split(",").map(_.trim))
    +    ).flatten
    +
    +    val jarUrl = desc.jarUrl.stripPrefix("file:").stripPrefix("local:")
    +
    +    ((jarUrl :: confUris) ++ getDriverExecutorURI(desc).toList).map(uri =>
    +      CommandInfo.URI.newBuilder().setValue(uri.trim()).build())
    +  }
    +
    +  private def getDriverCommandValue(desc: MesosDriverDescription): String = {
    +    val dockerDefined = desc.sc.contains("spark.mesos.executor.docker.image")
    +    val executorUri = getDriverExecutorURI(desc)
         // Gets the path to run spark-submit, and the path to the Mesos sandbox.
         val (executable, sandboxPath) = if (dockerDefined) {
           // Application jar is automatically downloaded in the mounted sandbox by Mesos,
           // and the path to the mounted volume is stored in $MESOS_SANDBOX env variable.
           ("./bin/spark-submit", "$MESOS_SANDBOX")
         } else if (executorUri.isDefined) {
    -      builder.addUris(CommandInfo.URI.newBuilder().setValue(executorUri.get).build())
           val folderBasename = executorUri.get.split('/').last.split('.').head
    +
    +      val entries = conf.getOption("spark.executor.extraLibraryPath")
    +        .map(path => Seq(path) ++ desc.command.libraryPathEntries)
    +        .getOrElse(desc.command.libraryPathEntries)
    +
    +      val prefixEnv = if (!entries.isEmpty) Utils.libraryPathEnvPrefix(entries) else ""
    --- End diff --
    
    nonEmpty


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71763139
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerUtils.scala ---
    @@ -79,7 +80,7 @@ private[mesos] trait MesosSchedulerUtils extends Logging {
           credBuilder.setPrincipal(principal)
         }
         conf.getOption("spark.mesos.secret").foreach { secret =>
    -      credBuilder.setSecret(ByteString.copyFromUtf8(secret))
    +      credBuilder.setSecret(secret)
    --- End diff --
    
    Yea, this and the new TASK_STATE.  The protobufs are relatively stable.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged to master


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test FAILed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    @srowen ready for re-review


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62685 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62685/consoleFull)** for PR 14275 at commit [`c02b7c7`](https://github.com/apache/spark/commit/c02b7c7ec14227700576124c00827100f35eee2a).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71690510
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/cluster/mesos/MesosSchedulerBackendUtil.scala ---
    @@ -105,16 +105,25 @@ private[mesos] object MesosSchedulerBackendUtil extends Logging {
       def addDockerInfo(
           container: ContainerInfo.Builder,
           image: String,
    +      containerizer: String,
           volumes: Option[List[Volume]] = None,
    -      network: Option[ContainerInfo.DockerInfo.Network] = None,
           portmaps: Option[List[ContainerInfo.DockerInfo.PortMapping]] = None): Unit = {
    --- End diff --
    
    Bridge mode is not supported why we need that?
    I read in the docs  spark.mesos.executor.docker.portmaps is used for that. I think docs need update.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71746242
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    _schedulerProperties: Map[String, String],
         val submissionId: String,
         val submissionDate: Date,
         val retryState: Option[MesosClusterRetryState] = None)
       extends Serializable {
     
    +  val sc = new SparkConf(false)
    +  _schedulerProperties.foreach {case (k, v) => sc.set(k, v)}
    +
       def copy(
           name: String = name,
           jarUrl: String = jarUrl,
           mem: Int = mem,
           cores: Double = cores,
           supervise: Boolean = supervise,
           command: Command = command,
    -      schedulerProperties: Map[String, String] = schedulerProperties,
    +      schedulerProperties: SparkConf = sc,
    --- End diff --
    
    Ah that's right.  I'll rename to `conf`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Rebased and upgraded to Mesos 1.0.0 rather than 0.28.2, since
    
    a) it's out now!, and
    b) we need support for unified containerizer caching
    
    FYI @skonto 
    
    Ready to merge @srowen 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71684506
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    _schedulerProperties: Map[String, String],
    --- End diff --
    
    Update tag parameter.
    @param schedulerProperties Extra properties to pass the Mesos scheduler



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    **[Test build #62939 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62939/consoleFull)** for PR 14275 at commit [`ec6707e`](https://github.com/apache/spark/commit/ec6707e35784113896063f5b5aec042e2e54ecb1).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark issue #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275
  
    Test "supports spark.mesos.driverEnv.*" fails.
    https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62686/testReport/junit/org.apache.spark.scheduler.cluster.mesos/MesosClusterSchedulerSuite/_/
    
    The reason is that the map there contains ".TEST_ENV", the dot is the issue.
    s/spark.mesos.driverEnv/spark.mesos.driverEnv for prefix at line 365, MesosClusterScheduler.scala


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request #14275: [SPARK-16637] Unified containerizer

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

    https://github.com/apache/spark/pull/14275#discussion_r71452324
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/mesos/MesosDriverDescription.scala ---
    @@ -40,24 +41,28 @@ private[spark] class MesosDriverDescription(
         val cores: Double,
         val supervise: Boolean,
         val command: Command,
    -    val schedulerProperties: Map[String, String],
    +    _schedulerProperties: Map[String, String],
         val submissionId: String,
         val submissionDate: Date,
         val retryState: Option[MesosClusterRetryState] = None)
       extends Serializable {
     
    +  val sc = new SparkConf(false)
    +  _schedulerProperties.foreach {case (k, v) => sc.set(k, v)}
    +
       def copy(
           name: String = name,
           jarUrl: String = jarUrl,
           mem: Int = mem,
           cores: Double = cores,
           supervise: Boolean = supervise,
           command: Command = command,
    -      schedulerProperties: Map[String, String] = schedulerProperties,
    +      schedulerProperties: SparkConf = sc,
    --- End diff --
    
    This is more semantically correct, and helps with some consistency issues in the docker methods


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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