[SPARK-4096][YARN]let ApplicationMaster accept executor memory argument in same format as JVM memory strings

[WangTaoTheTonic]

Here ApplicationMaster accept executor memory argument only in number format, we should let it accept JVM style memory strings as well.

[SparkQA]

Test build #22279 has started for PR 2955 at commit 3779767.

• This patch merges cleanly.

[SparkQA]

Test build #22279 has finished for PR 2955 at commit 3779767.

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

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22279/
Test FAILed.

[WangTaoTheTonic]

Jenkins, test this please.

[srowen]

It seems much more desirable to just support "3g" or "200m" in this argument, as was intended. Utils.memoryStringToMb can do the conversion.

[SparkQA]

Test build #22282 has started for PR 2955 at commit 3779767.

• This patch merges cleanly.

[WangTaoTheTonic]

@srowen I thought this way at beginning, but after more dig I found in ClientBase.scala there is an initilization of ApplicationMaster using arguments from ClientArguments which already convert args like "3g" or "200m" into numbers in MB.
So considering compatibility, maybe just modifying description is better.

[srowen]

Hm, how do you mean? the rest of the code already expects this to be an Int and a number of megabytes. Nothing else can be further parsing this (or else that's a bug). The change here is to support a different intended means of specifying that Int upstream. There is even a MemoryParam helper class for this.

[SparkQA]

Test build #22282 has finished for PR 2955 at commit 3779767.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22282/
Test PASSed.

[WangTaoTheTonic]

Guess I didn't make it clear. So I paste some code segment to help. Excuse me for this.

In ClientBase.scala we could see:
val amClass = if (isLaunchingDriver) { Class.forName("org.apache.spark.deploy.yarn.ApplicationMaster").getName } else { Class.forName("org.apache.spark.deploy.yarn.ExecutorLauncher").getName } val userArgs = args.userArgs.flatMap { arg => Seq("--arg", YarnSparkHadoopUtil.escapeForShell(arg)) }

val amArgs = Seq(amClass) ++ userClass ++ userJar ++ userArgs ++ Seq( "--executor-memory", args.executorMemory.toString, "--executor-cores", args.executorCores.toString, "--num-executors ", args.numExecutors.toString)
That is to say, we init an ApplicationMaster here using args "--executor-memory", args.executorMemory.toString,.

Here args in args.executorMemory is an ClientArguments instance in which executorMemory is a number.

So in common scene we would not directly init ApplicationMaster (even we could do), ApplicationMaster usuarlly is inited in ClientBase class.

Did I make it clear ? -______-

[srowen]

Right, so the value is already parsed 'upstream' to a value in megabytes, and it can't be parsed again or else the result will be wrong ('2000' will be treated as 2000 bytes, not 2000 megabytes).

--executor-memory is documented everywhere to accept values like '2g' though so I think it's not right to make the application master config in YARN different.

You're right the comment is wrong though, but, what about making it right? for example, if this code passes the size in megabytes, with "m" appended, and then ApplicationMasterArguments parses as a MemoryParam, it would also work?

[WangTaoTheTonic]

You mean we make ApplicationMasterArguments accept memory parameter in two kind of format, one is "2g" style and another is just number in megabytes?

[WangTaoTheTonic]

There is a better idea: we use MemoryParam to accept only "2g" style parameter in ApplicationMaster and let the memory string passed by ClientBase appended with "M".

[WangTaoTheTonic]

Code updated. How about it?

[SparkQA]

Test build #22298 has started for PR 2955 at commit ab98c70.

• This patch merges cleanly.

[srowen]

@WangTaoTheTonic heh yes that's exactly what I meant. +1!

[WangTaoTheTonic]

At beginning I misunderstood your point. Shame....

[SparkQA]

Test build #22298 has finished for PR 2955 at commit ab98c70.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/22298/
Test PASSed.

[vanzin]

Pardon if I misunderstand what's being done here, but does this actually change anything? It seems all that is changing is the ApplicationMaster command line which is completely opaque to users...

[srowen]

It should not change overall behavior. It at least makes this --executor-memory flag act like all the others, even if it is pretty internal, as the comments suggest it was meant to. A small nice thing.

[WangTaoTheTonic]

Yeah like owen said, this commit makes the config item more general.

[andrewor14]

Ok cool, I am merging this thanks.