Skip to content
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

[SPARK-37493][CORE] show gc time and duration time of driver in executors page #34749

Closed
wants to merge 4 commits into from
Closed

[SPARK-37493][CORE] show gc time and duration time of driver in executors page #34749

wants to merge 4 commits into from

Conversation

summaryzb
Copy link
Contributor

@summaryzb summaryzb commented Nov 30, 2021 •
edited

What changes were proposed in this pull request?

show driver's gc time & duration time(equivalent to application time) of driver in both driver side and history side UI

Why are the changes needed?

help user to config driver's resource more appropriately

Does this PR introduce any user-facing change?

yes,user will see driver's gc time & duration time in executors page .
when spark.eventLog.logStageExecutorMetrics is enabled driver's gc time can be logged.
before this change,user always get zero

image
image
image

How was this patch tested?

unit tests

@github-actions github-actions bot added the CORE label Nov 30, 2021
@LuciferYang
Copy link
Contributor

should be [SPARK-37493][CORE] show driver's gc time and duration time in executors page

@summaryzb summaryzb changed the title [Spark 37493][CORE] show driver's gc time and duration time in executors page [SPARK-37493][CORE] show driver's gc time and duration time in executors page Nov 30, 2021
dongjoon-hyun
dongjoon-hyun reviewed Nov 30, 2021
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you for making a PR, @summaryzb . Since this is UI patch, could you add the screenshots (BEFORE and AFTER) into your PR description?

@summaryzb
Copy link
Contributor Author

Thank you for making a PR, @summaryzb . Since this is UI patch, could you add the screenshots (BEFORE and AFTER) into your PR description?

done as suggested

@dongjoon-hyun
Copy link
Member

Thank you for updates, @summaryzb .

@LuciferYang
Copy link
Contributor

Maybe we need add new UT or change existing UT tor test incremental code

@summaryzb summaryzb force-pushed the SPARK-37493 branch 2 times, most recently from 1a35643 to 99b61dc Compare December 3, 2021 08:34
@summaryzb
Copy link
Contributor Author

@srowen @LuciferYang PTAL

@srowen
Copy link
Member

srowen commented Dec 3, 2021

Looks OK to me

@srowen
Copy link
Member

srowen commented Dec 3, 2021

Jenkins test this please

@SparkQA
Copy link

SparkQA commented Dec 3, 2021

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2021

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

@SparkQA
Copy link

SparkQA commented Dec 3, 2021

Test build #145909 has finished for PR 34749 at commit 99b61dc.

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

@summaryzb
Copy link
Contributor Author

Thank you so much @srowen @LuciferYang @dongjoon-hyun

mridulm
mridulm reviewed Dec 6, 2021
View reviewed changes
Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

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

Took a pass through the PR, mostly looks good; had a couple of comments.

core/src/main/scala/org/apache/spark/metrics/ExecutorMetricType.scala Show resolved Hide resolved
core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala Outdated Show resolved Hide resolved
@summaryzb summaryzb changed the title [SPARK-37493][CORE] show driver's gc time and duration time in executors page [SPARK-37493][CORE] show gc time and duration time of driver in executors page Dec 7, 2021
mridulm
mridulm reviewed Dec 7, 2021
View reviewed changes
Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

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

Looks good to me.

+CC @thejdeep, @shardulm94

srowen
srowen reviewed Dec 11, 2021
View reviewed changes
core/src/main/scala/org/apache/spark/metrics/ExecutorMetricType.scala Outdated
@@ -137,7 +138,9 @@ case object GarbageCollectionMetrics extends ExecutorMetricType with Logging {

override private[spark] def getMetricValues(memoryManager: MemoryManager): Array[Long] = {
val gcMetrics = new Array[Long](names.length) // minorCount, minorTime, majorCount, majorTime
Copy link
Member

Choose a reason for hiding this comment

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

One last thing - should we update or remove this comment? it's missing the 5th name.
No big deal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove the comment, since it's easy to get the meaning of names, kind of trouble to update, how about current

@summaryzb summaryzb force-pushed the SPARK-37493 branch 2 times, most recently from cb76126 to c719048 Compare December 13, 2021 08:13
@summaryzb
Copy link
Contributor Author

gentle ping @srowen

@srowen srowen closed this in c2df895 Dec 13, 2021
@srowen
Copy link
Member

srowen commented Dec 13, 2021

Merged to master

@summaryzb summaryzb deleted the SPARK-37493 branch December 14, 2021 02:10
@LuciferYang
Copy link
Contributor

Congrats on your first contribution, @summaryzb zhoubin ! :)

@Ngone51
Copy link
Member

Ngone51 commented Dec 15, 2021

Hi @summaryzb Just wonder why the Task Time is 1.9 min while the Total Tasks of the driver is 0?

@summaryzb
Copy link
Contributor Author

Hi @summaryzb Just wonder why the Task Time is 1.9 min while the Total Tasks of the driver is 0?

This pr aims at showing gc time and duration time(equivalent to the meanning of Task Time) of driver.
I just can't find the equivalent meaning of Total Tasks of driver

@Ngone51
Copy link
Member

Ngone51 commented Dec 20, 2021

Which duration? @summaryzb

@summaryzb
Copy link
Contributor Author

Which duration? @summaryzb

replace task time of driver with application attempt duration time to match the semantic

@Ngone51
Copy link
Member

Ngone51 commented Dec 20, 2021

Ok, got it now.

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

@srowen srowen srowen left review comments

@LuciferYang LuciferYang LuciferYang left review comments

@mridulm mridulm mridulm left review comments

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

Assignees
No one assigned
Labels
CORE
Projects
None yet
Milestone
No milestone
7 participants
@summaryzb @LuciferYang @dongjoon-hyun @srowen @SparkQA @Ngone51 @mridulm