[SPARK-11555] spark on yarn spark-class --num-workers doesn't work

[tgravescs]

I tested the various options with both spark-submit and spark-class of specifying number of executors in both client and cluster mode where it applied.

--num-workers, --num-executors, spark.executor.instances, SPARK_EXECUTOR_INSTANCES, default nothing supplied

[tgravescs]

Note this should go into branch-1.6 and branch-1.5 also

[SparkQA]

Test build #45226 has finished for PR 9523 at commit 5c104d1.

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

[srowen]

This looks like the right fix (modulo the long line). Should the param be something like a "current num executors" or a "default"? its default value is a default value after all.

It sort of seems like --num-executors could be used to set the initial num executors under dynamic allocation if not already set, but maybe that's best left untouched.

CC @jerryshao

[tgravescs]

I fixed the scala style error. I'm fine with changing the name of the parameter but don't like default as its not always the default. It could be what the user specified.

[srowen]

'default' with respect to the method... dunno, it seems strange that the purpose of the method is to compute a number of executors to use but it takes this as a param. Really it's the value to fall back on unless otherwise specified.

I don't feel strongly; the fix is right.

[SparkQA]

Test build #45247 has finished for PR 9523 at commit e32ad30.

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

[SparkQA]

Test build #1997 has finished for PR 9523 at commit e32ad30.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * final class ShuffleSortDataFormat extends SortDataFormat<PackedRecordPointer, LongArray>\n * final class UnsafeSortDataFormat extends SortDataFormat<RecordPointerAndKeyPrefix, LongArray>\n

[SparkQA]

Test build #45254 has finished for PR 9523 at commit 22b1bad.

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

[vanzin]

LGTM. One day I'd like to see this duplication of command-line arguments vs. SparkConf entries go away...

Merging to the 3 branches.

[jerryshao]

@tgravescs , sorry for missing this parameter --num-workers. I suppose user will not directly invoke yarn client through spark-class. Also from SparkSubmitArguments, this --num-workers is not parsed.

So maybe we should deprecate this parameter, as @vanzin said current there're so many ways we could set yarn related configurations, it is hard to manage and easy to introduce bug, also confused a lot.

[jerryshao]

Also this command is quite like a legacy code:

bin/spark-class org.apache.spark.deploy.yarn.Client --jar lib/spark-examples-1.5.2.0-hadoop2.6.0.16.1506060127.jar --class org.apache.spark.examples.SparkPi --num-workers 4 --worker-memory 2g --master-memory 1g --worker-cores 1 --queue default

Is it better to move forward? Compatible with old code really makes this part of configuration messy and inconsistent with the arguments of SparkSubmit. Just my thoughts, sorry for any incomplete consideration.

[srowen]

@jerryshao --num-workers is already deprecated and should generate a warning, but it happens with --num-executors. You mean deprecate direct use of spark-class? that should also generate a warning.

[jerryshao]

Ahhh, very sorry I misunderstood this PR. Yes, it is my bad to ignore this command line argument (--num-executors) which also can set executor number. Looks like in SparkSubmit, it is already changed to spark.executor.instances, so my bad there's another way which can set through cmd line (which is inconsistent with SparkSubmit).

BTW @srowen , such way of invoking yarn client already has a warning.

[jerryshao]

Looking at the code again, looks like there's already some codes take care of this backward compatibility in yarn client. The problem is that it is not worked because ClientArguments is already initialized before this setting.

So simply we could fix this by moving this part of code into here.

    // to maintain backwards-compatibility
    if (!Utils.isDynamicAllocationEnabled(sparkConf)) {
      sparkConf.set("spark.executor.instances", args.numExecutors.toString)
    }

That is more consistent with SparkSubmit, also cmd line args can override configuration spark.executor.instances if both set.

What do you think?

[srowen]

Yeah I meant these things all already generate a warning. I tend to agree with your additional change for this JIRA. Any other opinions?