Skip to content

[SPARK-17438][WebUI] Show Application.executorLimit in the application page #15001

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
zsxwing wants to merge 2 commits into from
Closed

[SPARK-17438][WebUI] Show Application.executorLimit in the application page #15001

zsxwing wants to merge 2 commits into from

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Sep 7, 2016 •
edited
Loading

What changes were proposed in this pull request?

This PR adds Application.executorLimit to the applicatino page

How was this patch tested?

Checked the UI manually.

Screenshots:

  1. Dynamic allocation is disabled

screen shot 2016-09-07 at 4 21 49 pm

1. Dynamic allocation is enabled.

screen shot 2016-09-07 at 4 25 30 pm

zsxwing
zsxwing reviewed Sep 7, 2016
View reviewed changes
core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala Outdated
Copy link
Member Author

@zsxwing zsxwing Sep 7, 2016

Choose a reason for hiding this comment

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

note: maxCores.get - app.coresGranted may be negative before we kill the executors.

@SparkQA
Copy link

SparkQA commented Sep 7, 2016

Test build #65055 has finished for PR 15001 at commit c2c84f1.

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

@zsxwing zsxwing changed the title [SPARK-17438][WebUI]Master UI should show the correct core limit when ApplicationInfo.executorLimit is set [SPARK-17438][WebUI] Show Application.executorLimit in the applicatino page Sep 7, 2016
@zsxwing
Copy link
Member Author

zsxwing commented Sep 7, 2016

cc @tdas

@SparkQA
Copy link

SparkQA commented Sep 8, 2016

Test build #65059 has finished for PR 15001 at commit 6526a02.

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

@zsxwing
Copy link
Member Author

zsxwing commented Sep 14, 2016

@JoshRosen do you have time to review this PR?

@JoshRosen
Copy link
Contributor

JoshRosen commented Sep 14, 2016

Minor typo in the PR title and description: applicatino -> application

JoshRosen
JoshRosen reviewed Sep 14, 2016
View reviewed changes
Copy link
Contributor

@JoshRosen JoshRosen left a comment

Choose a reason for hiding this comment

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

This change looks good to me overall but I have one suggestion regarding wording and a question regarding the maxCores limit shown on the Master UI.

core/src/main/scala/org/apache/spark/ui/ToolTips.scala Outdated
"Shaded red when garbage collection (GC) time is over 10% of task time"

val APPLICATION_EXECUTOR_LIMIT =
"""The number of executors this application can have at any given time. This is set only when
Copy link
Contributor

@JoshRosen JoshRosen Sep 14, 2016

Choose a reason for hiding this comment

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

How about something like

Maximum number of executors that this application will use. This limit is finite only when dynamic allocation is enabled.

Copy link
Member Author

@zsxwing zsxwing Sep 14, 2016

Choose a reason for hiding this comment

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

Updated

core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala
<span data-toggle="tooltip" title={ToolTips.APPLICATION_EXECUTOR_LIMIT}
data-placement="right">
<strong>Executor Limit: </strong>
{
Copy link
Contributor

@JoshRosen JoshRosen Sep 14, 2016

Choose a reason for hiding this comment

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

I noticed that ApplicationDescription has a maxCores field which is displayed on the Master UI's applications page. Do you think that there's any possibility for confusion between the limits displayed there (which are measured in terms of cores) and the executor limits shown here?

Copy link
Member Author

@zsxwing zsxwing Sep 14, 2016

Choose a reason for hiding this comment

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

Agreed. Actually, there is another confusing difference between executor limit and maxCores: maxCores is fixed for an application while executorLimit is mutable. I'm not sure how to clarify them just on UI.

Copy link
Contributor

@andrewor14 andrewor14 Sep 19, 2016

Choose a reason for hiding this comment

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

I think for now it's OK. Conceptually the number of executors is constrained by the lower of the two: the executor limit and the cores limit (when translated into executors).

@zsxwing zsxwing changed the title [SPARK-17438][WebUI] Show Application.executorLimit in the applicatino page [SPARK-17438][WebUI] Show Application.executorLimit in the application page Sep 14, 2016
@SparkQA
Copy link

SparkQA commented Sep 15, 2016

Test build #65408 has finished for PR 15001 at commit b41aa04.

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

@SparkQA
Copy link

SparkQA commented Sep 15, 2016

Test build #3266 has finished for PR 15001 at commit b41aa04.

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

@andrewor14
Copy link
Contributor

andrewor14 commented Sep 19, 2016

LGTM, merging into master 2.0 thanks @zsxwing!

asfgit pushed a commit that referenced this pull request Sep 19, 2016
@zsxwing
[SPARK-17438][WEBUI] Show Application.executorLimit in the applicatio…
d6191a0 
…n page

## What changes were proposed in this pull request?

This PR adds `Application.executorLimit` to the applicatino page

## How was this patch tested?

Checked the UI manually.

Screenshots:

1. Dynamic allocation is disabled

<img width="484" alt="screen shot 2016-09-07 at 4 21 49 pm" src="https://cloud.githubusercontent.com/assets/1000778/18332029/210056ea-7518-11e6-9f52-76d96046c1c0.png">

2. Dynamic allocation is enabled.

<img width="466" alt="screen shot 2016-09-07 at 4 25 30 pm" src="https://cloud.githubusercontent.com/assets/1000778/18332034/2c07700a-7518-11e6-8fce-aebe25014902.png">

Author: Shixiong Zhu <shixiong@databricks.com>

Closes #15001 from zsxwing/fix-core-info.

(cherry picked from commit 80d6655)
Signed-off-by: Andrew Or <andrew@databricks.com>
@asfgit asfgit closed this in 80d6655 Sep 19, 2016
wgtmac pushed a commit to wgtmac/spark that referenced this pull request Sep 19, 2016
@zsxwing @wgtmac
[SPARK-17438][WEBUI] Show Application.executorLimit in the applicatio…
e6a9a72 
…n page

## What changes were proposed in this pull request?

This PR adds `Application.executorLimit` to the applicatino page

## How was this patch tested?

Checked the UI manually.

Screenshots:

1. Dynamic allocation is disabled

<img width="484" alt="screen shot 2016-09-07 at 4 21 49 pm" src="https://cloud.githubusercontent.com/assets/1000778/18332029/210056ea-7518-11e6-9f52-76d96046c1c0.png">

2. Dynamic allocation is enabled.

<img width="466" alt="screen shot 2016-09-07 at 4 25 30 pm" src="https://cloud.githubusercontent.com/assets/1000778/18332034/2c07700a-7518-11e6-8fce-aebe25014902.png">

Author: Shixiong Zhu <shixiong@databricks.com>

Closes apache#15001 from zsxwing/fix-core-info.
@zsxwing zsxwing deleted the fix-core-info branch September 19, 2016 20:38
zzcclp added a commit to zzcclp/spark that referenced this pull request Sep 20, 2016
@zzcclp
[EXT][SPARK-17438][WebUI] Show Application.executorLimit in the appli…
50b585b 
…cation page apache#15001
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoshRosen JoshRosen JoshRosen left review comments

+1 more reviewer

@andrewor14 andrewor14 andrewor14 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zsxwing @SparkQA @JoshRosen @andrewor14