[SPARK-16194] Mesos Driver env vars #14167

mgummelt · 2016-07-13T01:09:05Z

What changes were proposed in this pull request?

Added new configuration namespace: spark.mesos.env.*

This allows a user submitting a job in cluster mode to set arbitrary environment variables on the driver.
spark.mesos.driverEnv.KEY=VAL will result in the env var "KEY" being set to "VAL"

I've also refactored the tests a bit so we can re-use code in MesosClusterScheduler.

And I've refactored the command building logic in buildDriverCommand. Command builder values were very intertwined before, and now it's easier to determine exactly how each variable is set.

How was this patch tested?

unit tests

SparkQA · 2016-07-13T01:15:28Z

Test build #62201 has finished for PR 14167 at commit e706ca6.

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

SparkQA · 2016-07-13T19:10:33Z

Test build #62270 has finished for PR 14167 at commit 37dc48a.

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

mgummelt · 2016-07-13T19:31:13Z

Error message from build: ERROR: Step ?Publish JUnit test result report? failed: No test report files were found. Configuration error?

skonto · 2016-07-14T09:59:39Z

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

Spark does not have a spark.driverEnv.[EnvironmentVariableName] while it has a spark.executorEnv.[EnvironmentVariableName] http://spark.apache.org/docs/latest/configuration.html.
From a UX experience and name consistency view i would expect something like that. The problem is that this pr only handles the mesos case so we cannot rename it to that name (unless we explicitly defined it in docs), also in client mode you do not need that so you will need to ignore it.
"spark.mesos.env." needs to be more specific like spark.mesos.driver.env but as i said spark.driverEnv seems more appropriate.

From a UX experience and name consistency view i would expect something like that.

Yea I went back and forth on this. I'm fine with spark.mesos.driverEnv Does that work?

also in client mode you do not need that so you will need to ignore it.

Do you mean there needs to be some code change to ignore it? The only handling code is in the cluster dispatcher, so it's effectively ignored in client mode.

skonto · 2016-07-14T10:09:21Z

The build fails with: [error] /home/jenkins/workspace/SparkPullRequestBuilder@4/core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/MesosClusterSchedulerSuite.scala:34: object Utils is not a member of package org.apache.spark.scheduler.cluster.mesos
[error] import org.apache.spark.scheduler.cluster.mesos.Utils._

https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/62270/consoleFull

I reproduced it locally you are missing the Utils object.

skonto · 2016-07-14T10:11:57Z

...scala/org/apache/spark/scheduler/cluster/mesos/MesosCoarseGrainedSchedulerBackendSuite.scala

Wild card imports should be avoided (unless you are importing more than 6 entities, or implicit methods) https://github.com/databricks/scala-style-guide. I think there are 5 (a bit picky here)...

Removed the wildcard.

mgummelt · 2016-07-14T19:06:22Z

@skonto comments addressed. Please re-review.

mgummelt · 2016-07-14T19:06:43Z

Note that I'll add docs once the code is 👍

SparkQA · 2016-07-14T20:53:49Z

Test build #62341 has finished for PR 14167 at commit 1bfee1d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-14T21:14:39Z

Test build #62340 has finished for PR 14167 at commit 9f7b151.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

skonto · 2016-07-14T22:08:31Z

core/src/test/scala/org/apache/spark/scheduler/cluster/mesos/Utils.scala

Pls fix indentation for method parameters (4 spaces, first parameter in a new line).

skonto · 2016-07-15T10:59:01Z

LGTM other than minor style issues. I run our tests against it so refactoring is successful i guess.

mgummelt · 2016-07-15T22:09:15Z

@skonto fixed the style issues

@andrewor14 plz merge

SparkQA · 2016-07-16T00:17:03Z

Test build #62391 has finished for PR 14167 at commit de75386.

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

mgummelt · 2016-07-20T01:22:09Z

@srowen Can we get a merge?

srowen · 2016-07-20T08:55:21Z

I don't know anything about mesos, but I presume y'all are pretty expert. Unless anyone else with Mesos knowledge chimes in, and tests pass and all that, OK to merge.

viirya · 2016-07-20T09:06:13Z

nit: From the code, it is spark.mesos.driverEnv but in PR description it is spark.mesos.env.

skonto · 2016-07-20T15:40:57Z

LGTM. I agree with @viirya for the description.

mgummelt · 2016-07-20T20:47:41Z

Updated the description and added docs.

SparkQA · 2016-07-20T22:45:12Z

Test build #62629 has finished for PR 14167 at commit b9c396b.

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

SparkQA · 2016-07-20T23:58:09Z

Test build #62635 has finished for PR 14167 at commit 1824d63.

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

skonto · 2016-07-21T09:58:48Z

@andrewor14 @srowen pls merge

srowen · 2016-07-21T17:30:24Z

Merged to master

skonto · 2016-07-21T20:42:42Z

great thnx @srowen

## 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. This is blocked on: #14167 ## 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 Author: Michael Gummelt <mgummelt@mesosphere.io> Closes #14275 from mgummelt/unified-containerizer.

## What changes were proposed in this pull request? Added new configuration namespace: spark.mesos.env.* This allows a user submitting a job in cluster mode to set arbitrary environment variables on the driver. spark.mesos.driverEnv.KEY=VAL will result in the env var "KEY" being set to "VAL" I've also refactored the tests a bit so we can re-use code in MesosClusterScheduler. And I've refactored the command building logic in `buildDriverCommand`. Command builder values were very intertwined before, and now it's easier to determine exactly how each variable is set. ## How was this patch tested? unit tests Author: Michael Gummelt <mgummelt@mesosphere.io> Closes apache#14167 from mgummelt/driver-env-vars.

mgummelt changed the title ~~[WIP] [SPARK-16194] Mesos Driver env vars~~ [SPARK-16194] Mesos Driver env vars Jul 13, 2016

skonto reviewed Jul 14, 2016
View reviewed changes

mgummelt mentioned this pull request Jul 20, 2016

[SPARK-16637] Unified containerizer #14275

Closed

mgummelt force-pushed the driver-env-vars branch from de75386 to b9c396b Compare July 20, 2016 20:45

spark.mesos.driverEnv

1824d63

mgummelt force-pushed the driver-env-vars branch from b9c396b to 1824d63 Compare July 20, 2016 21:55

asfgit closed this in 235cb25 Jul 21, 2016

mgummelt deleted the driver-env-vars branch July 21, 2016 17:42

[SPARK-16194] Mesos Driver env vars #14167

[SPARK-16194] Mesos Driver env vars #14167

Uh oh!

Conversation

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

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 13, 2016

Uh oh!

SparkQA commented Jul 13, 2016

Uh oh!

mgummelt commented Jul 13, 2016

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgummelt Jul 14, 2016

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgummelt Jul 14, 2016

Choose a reason for hiding this comment

Uh oh!

mgummelt commented Jul 14, 2016

Uh oh!

mgummelt commented Jul 14, 2016

Uh oh!

SparkQA commented Jul 14, 2016

Uh oh!

SparkQA commented Jul 14, 2016

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgummelt Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

skonto commented Jul 15, 2016

Uh oh!

mgummelt commented Jul 15, 2016

Uh oh!

SparkQA commented Jul 16, 2016

Uh oh!

mgummelt commented Jul 20, 2016

Uh oh!

srowen commented Jul 20, 2016

Uh oh!

viirya commented Jul 20, 2016

Uh oh!

skonto commented Jul 20, 2016

Uh oh!

mgummelt commented Jul 20, 2016

Uh oh!

SparkQA commented Jul 20, 2016

Uh oh!

SparkQA commented Jul 20, 2016

Uh oh!

skonto commented Jul 21, 2016

Uh oh!

srowen commented Jul 21, 2016

Uh oh!

skonto commented Jul 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mgummelt commented Jul 13, 2016 •

edited

Loading

skonto Jul 14, 2016 •

edited

Loading

skonto commented Jul 14, 2016 •

edited

Loading

skonto Jul 14, 2016 •

edited

Loading

skonto Jul 14, 2016 •

edited

Loading