SPARK-3178 setting SPARK_WORKER_MEMORY to a value without a label (m or g) sets the worker memory limit to zero

[bbejeck]

Validate the memory is greater than zero when set from the SPARK_WORKER_MEMORY environment variable or command line without a g or m label. Added unit tests. If memory is 0 an IllegalStateException is thrown. Updated unit tests to mock environment variables by subclassing SparkConf (tip provided by Josh Rosen). Updated WorkerArguments to use SparkConf.getenv instead of System.getenv for reading the SPARK_WORKER_MEMORY environment variable.

[SparkQA]

Can one of the admins verify this patch?

[bbejeck]

Can an admin take a look and verify this PR ?

[JoshRosen]

Jenkins, this is ok to test.

[SparkQA]

QA tests have started for PR 2309 at commit 51cf915.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2309 at commit 51cf915.

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

[vanzin]

Still think the fix should be made in the Utils class. This is not the only place where this could cause trouble.

[bbejeck]

Still think the fix should be made in the Utils class. This is not the only place where this could cause trouble.

I agree there are other areas in the code that would benefit from the memory check.

I originally handled the memory issue in the WorkerArguments class because it has a precedence with respect to memory settings - default is using a percentage of system memory, then use the SPARK_WORKER_MEMORY setting (if it exists) and finally use the memory setting provided from the command line with a "-m" switch.

My concern is If it's handled in the Utils class we could have a case where the user is setting memory from the command line but is getting unexpected errors from the mislabeled environment variable. At any rate I'm probably over-optimizing and unless there are any objections I'll change the code to handle memory set with no labels in the Utils class.

Thanks

[vanzin]

My concern is If it's handled in the Utils class we could have a case where the user is setting memory from the command line but is getting unexpected errors from the mislabeled environment variable.

If that indeed happens, the code should be changed so that only the actual value being used is parsed. (The error, though, might be more benefitial than harmful: it tells the user there is something wrong somewhere in his configuration.)

But well, let's see if any committers have a different opinion.

[JoshRosen]

Hi @bbejeck,

Sorry for allowing this to sit unreviewed for so long.

I can sort of understand how it could be confusing if an invalid setting in an overridden configuration led to an error message. However, I think this is already the case if I set SPARK_WORKER_MEMORY to a completely invalid string that can't be parsed as a number.

We currently seem to have a sort of "bottom-up" way of handling configuration precedence in this file, where we compute the default configurations then keep overriding them (e.g. first set the default, then override it with the environment variable, then override it with the command-line argument). It sounds like @vanzin is proposing a "top-down" approach where we first determine which configuration should be used and then attempt to validate that configuration. I generally prefer the latter approach, but this seems like kind of a bigger change.

I agree that this issue could potentially affect other locations where we allow users to configure memory. However, it looks like spark.executor.memory and spark.driver.memory are preserved as strings and passed directly to the JVM rather than being parsed.

I guess that the code in this PR is a strict improvement over what we have now, so I'm going to merge it. If a more general version of this problem crops up, let's solve it then.

[bbejeck]

Hi @JoshRosen ,

Thanks for reviewing and closing this out.