-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-32446][CORE] Add percentile distribution REST API & UI of peak memory metrics for all executors #34695
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.
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
Conversation
…y metrics for all executors
|
@sarutak @Patil Hi, I am trying to build this pr's UI table, but I found when I write such It won't change the html content. I want to know how can I enable this? Hope for your suggestion. |
|
Also ping @pgandhi999 |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #145563 has finished for PR 34695 at commit
|
|
I shall definitely review the PR, thank you. |
|
Could you please post screenshots of the UI that was tested with your code changes in the PR description? Thank you. |
core/src/main/resources/org/apache/spark/ui/static/executorspage.js
Outdated
Show resolved
Hide resolved
| return "Mapped Pool Memory"; | ||
|
|
||
| case "ProcessTreeJVMVMemory": | ||
| return "Process Tree JVM Memory"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be VMemory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there are ProcessTreeJVMVMemory and ProcessTreePythonVMemory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am talking about the column heading, it should be Process Tree JVM VMemory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am talking about the column heading, it should be
Process Tree JVM VMemory.
Hmmm, sorry for my mistake...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am talking about the column heading, it should be
Process Tree JVM VMemory.
updated
Done |
|
Kubernetes integration test starting |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Kubernetes integration test status failure |
|
Test build #145985 has finished for PR 34695 at commit
|
|
Test build #145987 has finished for PR 34695 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #145992 has finished for PR 34695 at commit
|
|
@gengliangwang Have updated, I think current code can match your requirement. |
|
Gentle ping @gengliangwang |
|
ping @gengliangwang |
|
@AngersZhuuuu I will check this one in the weekend. |
|
@AngersZhuuuu shall we hide this by default if no additional metrics is selected? |
|
cc @rednaxelafx @jasonli-db as well |
Updated |
| } | ||
|
|
||
| function getColumnNameForExecutorMetricSummary(columnKey) { | ||
| switch(columnKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we use a string map instead of switch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we use a string map instead of switch?
Hmm, same method from stagingspage.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we use a string map instead of switch?
Updated
| sumCol.visible(!sumCol.visible()); | ||
| } | ||
| var para = thisBox.attr('exec-sum-idx'); | ||
| if(para != '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use !==
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
@AngersZhuuuu this LGTM overall. |
|
@gengliangwang No reply for a long time....How about merge first then got reply form users? |
|
Gentle ping |
|
gentle ping @rednaxelafx @jasonli-db |
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |




What changes were proposed in this pull request?
This pr continue the work of #29247 since origin author didn't reply for a long time.
Will add as co-author.
For the whole process of application, user may want to know each executor's peak memory usage to see the Resource utilization. The distribution of all executor's peak memory metrics usage can help users know whether or not there is a skew/bottleneck among executor resource utilization in a given stage.
We define
activeOnlyandquantilesquery parameter in the REST API for all executors peak memory metrics distribution:0.0,0.25,0.5,0.75,1.0only effect whenwithSummaries=true, it define the quantiles we use when calculating metrics distributions.When withSummaries=true, both task metrics in percentile distribution and executor metrics in percentile distribution are included in the REST API output. The default value of withSummaries is false, i.e. no metrics percentile distribution will be included in the REST API output.
Why are the changes needed?
Always user care about executor peak usage distribution, this pr help users understand executor peak memory metrics distributions.
Does this PR introduce any user-facing change?
User can use below restful API to get all executor's peak memory metrics distribution:
How was this patch tested?