Skip to content

[SPARK-31900][SPARK-SUBMIT] Client memory passed unvalidated to the JVM Xmx#28720

Closed
gerashegalov wants to merge 2 commits intoapache:masterfrom
gerashegalov:SPARK-31900
Closed

[SPARK-31900][SPARK-SUBMIT] Client memory passed unvalidated to the JVM Xmx#28720
gerashegalov wants to merge 2 commits intoapache:masterfrom
gerashegalov:SPARK-31900

Conversation

@gerashegalov
Copy link
Contributor

An example of a command that fail in the client mode but would not in the cluster mode or if the same value were used for spark.executor.memory

> $SPARK_HOME/bin/spark-shell --conf spark.driver.memory=" 4g "
Invalid maximum heap size: -Xmx 4g
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

also note that the same message is produced by the JVM makes it hard to identify which of the configs SPARK_DAEMON_MEMORY, SPARK_DRIVER_MEMORY, etc was responsible

Why are the changes needed?

easier ops/diagnostics, more uniform behavior

Does this PR introduce any user-facing change?

fix-only change

How was this patch tested?

  • unit tests
  • manual testing

@gatorsmile
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented Jun 5, 2020

Test build #123546 has finished for PR 28720 at commit 87e1d67.

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

@dongjoon-hyun
Copy link
Member

Retest this please.

@SparkQA
Copy link

SparkQA commented Jun 19, 2020

Test build #124240 has finished for PR 28720 at commit 87e1d67.

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

@github-actions
Copy link

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Sep 28, 2020
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a whole lot of extra code to catch one particular type of typo, which even produces an error that narrows it down pretty far. I'm not sure it's worthwhile.

@srowen srowen closed this Sep 28, 2020
@gerashegalov
Copy link
Contributor Author

thanks for looking at the PR @srowen. Since there are multiple culprits potentially responsible for the issue I thought a patch was warrant. And I like to see consistent behavior between similar settings. However, I also understand the tradeoff of accepting more code for future maintenance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants