Skip to content

[MINOR][DOC] spark.executor.memoryOverhead is not cluster-mode only #30311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[MINOR][DOC] spark.executor.memoryOverhead is not cluster-mode only #30311

wants to merge 1 commit into from

Conversation

@yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Nov 10, 2020

What changes were proposed in this pull request?

Remove "in cluster mode" from the description of spark.executor.memoryOverhead

Why are the changes needed?

fix correctness issue in documentaion

Does this PR introduce any user-facing change?

yes, users may not get confused about the description spark.executor.memoryOverhead

How was this patch tested?

pass GA doc generation

@SparkQA
Copy link

SparkQA commented Nov 10, 2020

Test build #130847 has finished for PR 30311 at commit 3aa18e8.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 10, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35455/

@SparkQA
Copy link

SparkQA commented Nov 10, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35455/

@yaooqinn
Copy link
Member Author

yaooqinn commented Nov 10, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Nov 10, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35479/

srowen
srowen approved these changes Nov 10, 2020
View reviewed changes
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.

Yes I believe you're correct.

@SparkQA
Copy link

SparkQA commented Nov 10, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35479/

@SparkQA
Copy link

SparkQA commented Nov 10, 2020

Test build #130872 has finished for PR 30311 at commit 3aa18e8.

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

@yaooqinn
Copy link
Member Author

yaooqinn commented Nov 11, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Nov 11, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35505/

@SparkQA
Copy link

SparkQA commented Nov 11, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35505/

@SparkQA
Copy link

SparkQA commented Nov 11, 2020

Test build #130899 has finished for PR 30311 at commit 3aa18e8.

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

@yaooqinn
Copy link
Member Author

yaooqinn commented Nov 11, 2020

cc @cloud-fan @maropu too~

maropu
maropu reviewed Nov 11, 2020
View reviewed changes
core/src/main/scala/org/apache/spark/internal/config/package.scala
private[spark] val EXECUTOR_MEMORY_OVERHEAD = ConfigBuilder("spark.executor.memoryOverhead")
.doc("The amount of non-heap memory to be allocated per executor in cluster mode, " +
"in MiB unless otherwise specified.")
.doc("The amount of non-heap memory to be allocated per executor, in MiB unless otherwise" +
Copy link
Member

@maropu maropu Nov 11, 2020

Choose a reason for hiding this comment

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

This change means that the non-cluster mode refers to this value?

Copy link
Member Author

@yaooqinn yaooqinn Nov 12, 2020

Choose a reason for hiding this comment

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

yes, the 'client' deploy mode respects this too

@maropu maropu closed this in 4335af0 Nov 12, 2020
maropu pushed a commit that referenced this pull request Nov 12, 2020
@yaooqinn @maropu
[MINOR][DOC] spark.executor.memoryOverhead is not cluster-mode only
5ee76e6 
### What changes were proposed in this pull request?

Remove "in cluster mode" from the description of `spark.executor.memoryOverhead`

### Why are the changes needed?

fix correctness issue in documentaion

### Does this PR introduce _any_ user-facing change?

yes, users may not get confused about the description `spark.executor.memoryOverhead`

### How was this patch tested?

pass GA doc generation

Closes #30311 from yaooqinn/minordoc.

Authored-by: Kent Yao <[email protected]>
Signed-off-by: Takeshi Yamamuro <[email protected]>
(cherry picked from commit 4335af0)
Signed-off-by: Takeshi Yamamuro <[email protected]>
@maropu
Copy link
Member

maropu commented Nov 12, 2020

Thanks! Merged to master/branch-3.0. Could you open a PR to backport this into branch-2.4?

@yaooqinn
Copy link
Member Author

yaooqinn commented Nov 12, 2020

yea, thanks for merging

@yaooqinn
Copy link
Member Author

yaooqinn commented Nov 13, 2020

@maropu the description for spark.executor.memoryOverhead is fine in http://spark.apache.org/docs/2.4.7/configuration.html. Then it is not that necessary to backport this although it's wrong in the code.

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

Reviewers

@maropu maropu maropu approved these changes

@srowen srowen srowen approved these changes

@cloud-fan cloud-fan cloud-fan approved these changes

Assignees

No one assigned

Labels

CORE DOCS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yaooqinn @SparkQA @maropu @srowen @cloud-fan