[YARN][DOC] Remove non-Yarn specific configurations from running-on-yarn.md #15869

weiqingy · 2016-11-13T04:57:27Z

What changes were proposed in this pull request?

Remove spark.driver.memory, spark.executor.memory, spark.driver.cores, and spark.executor.cores from running-on-yarn.md as they are not Yarn-specific, and they are also defined inconfiguration.md.

How was this patch tested?

Build passed & Manually check.

srowen

CC @vanzin

srowen · 2016-11-14T12:00:50Z

docs/configuration.md

    Amount of memory to use per executor process (e.g. <code>2g</code>, <code>8g</code>).
  </td>
 </tr>
+<tr>


I think this is actually YARN-specific right now? At least it is according to SparkSubmit.scala

Thanks for reviewing this PR, @srowen

Actually I am confused by this. From the information below, it seems not YARN-specific:

https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/SparkContextSuite.scala#L403

In configuration.md:

<tr> <td><code>spark.dynamicAllocation.initialExecutors</code></td> <td><code>spark.dynamicAllocation.minExecutors</code></td> <td> Initial number of executors to run if dynamic allocation is enabled. <br /><br /> If `--num-executors` (or `spark.executor.instances`) is set and larger than this value, it will be used as the initial number of executors. </td> </tr>

Currentlyspark.executor.instances is defined in config/package.scala. If it is for YARN-specific, it should be defined in yarn/config.scala instead of config/package.scala.

srowen · 2016-11-14T12:01:57Z

docs/running-on-yarn.md

    Use lower-case suffixes, e.g. <code>k</code>, <code>m</code>, <code>g</code>, <code>t</code>, and <code>p</code>, for kibi-, mebi-, gibi-, tebi-, and pebibytes, respectively.
  </td>
 </tr>
-<tr>


Yes, I see that these are duplicated in the YARN docs and main config docs. So are things like spark.driver.cores, but, the info on what it means in YARN is slightly more specific to YARN in the YARN docs. The spark.driver.memory doc isn't different, but, for consistency, maybe does not hurt to leave a note about these key, generic props in the YARN docs.

Yes, in Yarn mode, the properties spark.driver.cores, spark.driver.memory, and spark.executor.memory are often used. But if searching them in Spark code (i.e. the information below), we will find they are not Yarn-specific.

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#L448

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#L477

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#L469

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/SparkSubmit.scala#L469

https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/deploy/rest/StandaloneRestServer.scala#L135

https://github.com/apache/spark/blob/master/mesos/src/main/scala/org/apache/spark/deploy/rest/mesos/MesosRestServer.scala#L92

...

srowen · 2016-11-14T12:03:22Z

docs/running-on-yarn.md

  </td>
 </tr>
+<tr>
+  <td><code>spark.yarn.report.interval</code></td>


I am not sure whether these are meant to be exposed to users -- not secret exactly, but not sure if they're to be advertised. Maybe it's fine to document. I'm neutral.

If it wasn't documented, it was on purpose. If users find it a useful config we can expose it.

Thanks for this information.

srowen · 2016-11-14T12:03:26Z

docs/running-on-yarn.md

+  <td><code>spark.yarn.report.interval</code></td>
+  <td>1s</td>
+  <td>
+  Interval between reports of the current app status in Yarn cluster mode.


Yarn -> YARN

srowen · 2016-11-14T12:03:55Z

docs/running-on-yarn.md

+</tr>
+<tr>
+  <td><code>spark.yarn.services</code></td>
+  <td>Nil</td>


As far the user is concerned, I think the best way to express the default is "(none)". Nil is an implementation detail

weiqingy · 2016-11-15T04:19:02Z

Hi, @srowen I have replied your comments and updated the PR. Could you please review it again? Thanks.

rxin · 2016-11-15T08:20:03Z

Can you just put some of the more trivial pull requests into one, e.g. the two documentation update ones?

tgravescs · 2016-11-15T15:49:51Z

docs/running-on-yarn.md

+  </td>
+</tr>
+<tr>
+  <td><code>spark.yarn.services</code></td>


I don't think we should expose this. I think this was added with a PR to start adding integration with ATS which I don't know we officially support yet, this config is just for testing at this point. @steveloughran to verify.

Thanks for pointing out this.

weiqingy · 2016-11-15T16:43:15Z

@rxin Thanks for commenting. The reason I created two PRs is that I noticed these issues not at the same time. This one is still during discussion, and I am not sure if it will be merged, however, the other one I am sure it will be merged. I did not think too much about this. I just though if I find an issue, no matter how small it is, just submit a PR directly. But, I will pay attention next time. So do you mean I should close PR #15886 and merge it into this PR? I can do that. Thanks.

weiqingy · 2016-11-15T17:00:58Z

Hi, @tgravescs What do you think about spark.driver.memory, spark.executor.memory, and spark.executor.instances?

Move spark.executor.instances from running-on-yarn.md to configuration.md.
Remove spark.driver.memory and spark.executor.memory from running-on-yarn.md(they are also defined in configuration.md).

Actually I noticed this issue when I was working on PR #15563. So if they are documented in current way on purpose, I can just close this PR. I would like your suggestion. Thanks.

tgravescs · 2016-11-15T20:00:24Z

spark.driver.memory and spark.executor.memory is good to remove from yarn side as its duplicate since they were added for others.
spark.driver.cores and spark.executor.cores could also be removed from yarn config docs as they are in the general ones now too.
spark.executor.instances is still only used in YARN. --num-executors is also YARN only. I'm not sure the exact behavior of spark.dynamicAllocation.initialExecutors on mesos or standalone mode to know if the description for that should be updated for those.

…iguration doc, etc

weiqingy · 2016-11-16T04:01:50Z

@tgravescs Thanks for the reply. I have updated the PR.

srowen

OK, but can you now update the title and description? "Update Yarn doc" is too generic, and the description doesn't match the change now.

steveloughran · 2016-11-16T12:09:02Z

The plugin point is more generic than ATS integration; it lets you stick anything in to come up in the driver. Weakness: it's actually yarn specific; I could imagine uses in standalone too. The update interval flag? I don't remember this or what it does. Sorry

weiqingy · 2016-11-16T17:00:40Z

Hi, @srowen I have updated the title and description. Thanks.

SparkQA · 2016-11-17T10:44:46Z

Test build #3429 has finished for PR 15869 at commit dcb11ae.

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

srowen · 2016-11-17T11:13:46Z

Merged to master/2.1

…arn.md ## What changes were proposed in this pull request? Remove `spark.driver.memory`, `spark.executor.memory`, `spark.driver.cores`, and `spark.executor.cores` from `running-on-yarn.md` as they are not Yarn-specific, and they are also defined in`configuration.md`. ## How was this patch tested? Build passed & Manually check. Author: Weiqing Yang <yangweiqing001@gmail.com> Closes #15869 from weiqingy/yarnDoc. (cherry picked from commit a3cac7b) Signed-off-by: Sean Owen <sowen@cloudera.com>

…arn.md ## What changes were proposed in this pull request? Remove `spark.driver.memory`, `spark.executor.memory`, `spark.driver.cores`, and `spark.executor.cores` from `running-on-yarn.md` as they are not Yarn-specific, and they are also defined in`configuration.md`. ## How was this patch tested? Build passed & Manually check. Author: Weiqing Yang <yangweiqing001@gmail.com> Closes apache#15869 from weiqingy/yarnDoc.

[YARN][DOC] Update Yarn configuration doc

3b31852

srowen requested changes Nov 14, 2016

View reviewed changes

Update the running-on-yarn.md

6dd24eb

tgravescs reviewed Nov 15, 2016

View reviewed changes

remove 'spark.driver.cores' and 'spark.executor.cores' from yarn conf…

dcb11ae

…iguration doc, etc

srowen approved these changes Nov 16, 2016

View reviewed changes

weiqingy changed the title ~~[YARN][DOC] Update Yarn configuration doc~~ [YARN][DOC] Remove non-Yarn specific configurations from running-on-yarn.md Nov 16, 2016

asfgit closed this in a3cac7b Nov 17, 2016

[YARN][DOC] Remove non-Yarn specific configurations from running-on-yarn.md #15869

[YARN][DOC] Remove non-Yarn specific configurations from running-on-yarn.md #15869

Uh oh!

Conversation

weiqingy commented Nov 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weiqingy Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weiqingy commented Nov 15, 2016

Uh oh!

rxin commented Nov 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weiqingy commented Nov 15, 2016

Uh oh!

weiqingy commented Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgravescs commented Nov 15, 2016

Uh oh!

weiqingy commented Nov 16, 2016

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

steveloughran commented Nov 16, 2016

Uh oh!

weiqingy commented Nov 16, 2016

Uh oh!

SparkQA commented Nov 17, 2016

Uh oh!

srowen commented Nov 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

weiqingy commented Nov 13, 2016 •

edited

Loading

weiqingy Nov 15, 2016 •

edited

Loading

weiqingy commented Nov 15, 2016 •

edited

Loading