Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-22287][MESOS] SPARK_DAEMON_MEMORY not honored by MesosClusterD… #19515

Closed
wants to merge 1 commit into from

Conversation

pmackles
Copy link

…ispatcher

What changes were proposed in this pull request?

Allow JVM max heap size to be controlled for MesosClusterDispatcher via SPARK_DAEMON_MEMORY environment variable.

How was this patch tested?

Tested on local Mesos cluster

@ArtRand
Copy link

ArtRand commented Oct 18, 2017

Hello @pmackles, thanks for this. It would be set to the value of SPARK_DRIVER_MEMORY by default correct? What to you think about introducing a new envvar (SPARK_DISPATCHER_MEMORY) so that it can be individually configured? Only motivated by the fact that the Dispatcher is a pretty unique beast compared to the other Spark daemons.

@pmackles
Copy link
Author

pmackles commented Oct 18, 2017

Hi @ArtRand - Based on my testing and interpretation of the code, SPARK_DRIVER_MEMORY has no affect on MesosClusterDispatcher. Heap size always winds up being set to -Xmx1G (the default).

I went with SPARK_DAEMON_MEMORY because that's what is used for the Master/Worker in a standalone cluster and I think Dispatcher is pretty similar in nature to those daemons. That said, I don't have a strong opinion and SPARK_DISPATCHER_MEMORY works fine for my use case (docker container running on mesos) so I am happy to switch it in the name of getting this PR accepted.

@pmackles
Copy link
Author

@ArtRand - WDYT? I was going to switch it to SPARK_DISPATCHER_MEMORY but then I noticed that the other env vars for MesosClusterDispatcher or also prefixed with SPARK_DAEMON_* so I thought it might be better to keep the names consistent.

@ArtRand
Copy link

ArtRand commented Oct 23, 2017

@pmackles, Yeah that sounds good. The default would stay the same, correct? Thanks for this!

@pmackles
Copy link
Author

@ArtRand - yeah default would remain the same which is 1g

@ArtRand
Copy link

ArtRand commented Oct 24, 2017

LGTM

@pmackles
Copy link
Author

pmackles commented Nov 9, 2017

@felixcheung - any chance of getting this tiny change merged and included in the upcoming 2.2.1 release?

@felixcheung
Copy link
Member

Jenkins, test this please

@felixcheung
Copy link
Member

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

@felixcheung
Copy link
Member

@pmackles perhaps you could email this to dev@spark.apache.org to get some visibility to this and hopefully someone else on the mesos side can review?

@SparkQA
Copy link

SparkQA commented Nov 9, 2017

Test build #83648 has finished for PR 19515 at commit 33a8e68.

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

@vanzin
Copy link
Contributor

vanzin commented Nov 10, 2017

LGTM, merging to master / 2.2.

asfgit pushed a commit that referenced this pull request Nov 10, 2017
…ispatcher

## What changes were proposed in this pull request?

Allow JVM max heap size to be controlled for MesosClusterDispatcher via SPARK_DAEMON_MEMORY environment variable.

## How was this patch tested?

Tested on local Mesos cluster

Author: Paul Mackles <pmackles@adobe.com>

Closes #19515 from pmackles/SPARK-22287.

(cherry picked from commit f5fe63f)
Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
@asfgit asfgit closed this in f5fe63f Nov 10, 2017
MatthewRBruce pushed a commit to Shopify/spark that referenced this pull request Jul 31, 2018
…ispatcher

## What changes were proposed in this pull request?

Allow JVM max heap size to be controlled for MesosClusterDispatcher via SPARK_DAEMON_MEMORY environment variable.

## How was this patch tested?

Tested on local Mesos cluster

Author: Paul Mackles <pmackles@adobe.com>

Closes apache#19515 from pmackles/SPARK-22287.

(cherry picked from commit f5fe63f)
Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants