[SPARK-23431][CORE] Expose the new executor memory metrics at the stage level #23340

edwinalu · 2018-12-18T02:51:06Z

What changes were proposed in this pull request?

Collect and show the new executor memory metrics for each stage, to provide more information on how memory is used per stage. Peak values for metrics are show for each stage. For executor summaries for each stage, the peak values per executor are also shown.

How was this patch tested?

Added new unit tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

felixcheung · 2018-12-18T07:41:22Z

ok to test

SparkQA · 2018-12-18T08:05:01Z

Test build #100275 has finished for PR 23340 at commit d77612c.

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

squito · 2018-12-18T18:40:42Z

Jenkins, retest this please

squito

Can you update the PR description to make it clear this is only updating the REST endpoints for now? As you refer to 'showing' the metrics, it sounds like they're visible in the UI.

squito · 2018-12-18T18:44:30Z

core/src/main/scala/org/apache/spark/status/AppStatusListener.scala

+      val activeStages = liveStages.values().asScala
+        .filter(_.status == v1.StageStatus.ACTIVE)
+      activeStages.foreach { stage =>
+        stage.peakExecutorMetrics.compareAndUpdatePeakValues(updates)


do you need a maybeUpdate(stage, now) after this?

Yes, thanks for catching this.

rezasafi

Thank you very much for the pr.

rezasafi · 2018-12-18T20:46:00Z

core/src/main/scala/org/apache/spark/status/AppStatusListener.scala

-
+    Option(liveStages.get((executorMetrics.stageId, executorMetrics.stageAttemptId)))
+      .foreach { stage =>
+      stage.peakExecutorMetrics.compareAndUpdatePeakValues(executorMetrics.executorMetrics)


nit: Is it possible to change the name of the parameter or the field member? executorMetrics.executorMetrics feels kind of weird

How about "event" for the parameter name -- this would be more inline with the other methods.

yeah, sounds good to me.

rezasafi · 2018-12-18T20:47:38Z

core/src/main/scala/org/apache/spark/status/LiveEntity.scala

  var metrics = createMetrics(default = 0L)

+  // peak values for executor level metrics
+  var peakExecutorMetrics = new ExecutorMetrics()


can this also be a val like the one in line 387?

Yes, this should be val.

SparkQA · 2018-12-18T23:11:54Z

Test build #100289 has finished for PR 23340 at commit d77612c.

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

SparkQA · 2018-12-19T03:05:17Z

Test build #100294 has finished for PR 23340 at commit 42599fe.

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

squito · 2018-12-19T18:23:13Z

core/src/main/scala/org/apache/spark/status/AppStatusListener.scala

+    }
    // check if there is a new peak value for any of the executor level memory metrics,
    // while reading from the log. SparkListenerStageExecutorMetrics are only processed
    // when reading logs.


I need a little refresher here -- does this comment also apply to the new code you're adding above?

and why again does this only apply for when we're reading from the event logs (maybe this comment should be updated to point to whatever is happening for a live app)

Yes, comments are still applicable, although for stage level. I'll move the comments around, and and add a pointer to onExecutorMetricsUpdate for the live app case.

SparkQA · 2018-12-20T05:16:40Z

Test build #100324 has finished for PR 23340 at commit 3dd8424.

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

SparkQA · 2019-04-23T23:11:52Z

Test build #104844 has finished for PR 23340 at commit 6d6f64c.

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

edwinalu · 2019-04-23T23:43:02Z

@squito and @rezasafi , could you please take another look? Thanks for your review and comments.

github-actions · 2020-01-03T00:08:46Z

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.

If you'd like to revive this PR, please reopen it!

itamarst · 2020-05-26T14:39:19Z

Ping—any chance someone could review this patch? These are useful metrics.

stackedsax · 2020-06-23T22:44:37Z

@squito @rezasafi any chance either of you could review this patch?

dongjoon-hyun · 2020-07-01T03:52:49Z

This is reopened according to the community request. I also removed the label, Stale.

AmplabJenkins · 2020-07-01T03:58:29Z

Can one of the admins verify this patch?

stackedsax · 2020-07-01T04:05:35Z

@edwinalu thanks for getting this started so long ago. Looks like there are some conflicts after so much time has passed. Do you have time/interest to continue working on this or would you like some help getting this sorted out?

edwinalu · 2020-07-01T14:12:46Z

@stackedsax , I don't have time to continue working on this unfortunately. If you or someone else is able to help with adding stage level metrics, that would be great. Please let me know if there's any questions about the original PR.

imback82 · 2020-07-02T04:42:21Z

@stackedsax, do you plan to work on this? Otherwise, I can start looking into this next week. Thanks!

itamarst · 2020-07-02T17:19:33Z

@imback82 I'll be taking this up if you don't (on behalf of @stackedsax), but since I am not a Spark contributor at the moment it'd probably be a lot faster and easier if you did it. So please do look into it—thank you!

imback82 · 2020-07-03T03:06:29Z

Thanks @itamarst. I started looking into this and will update this thread.

imback82 · 2020-07-07T07:27:55Z

I created a PR here: #29020. I will also ask a reviewing committer to put @edwinalu as a co-author if that's the practice when a PR is "inherited". Thanks!

… API ### What changes were proposed in this pull request? Note that this PR is forked from #23340 originally written by edwinalu. This PR proposes to expose the peak executor metrics at the stage level via the REST APIs: * `/applications/<application_id>/stages/`: peak values of executor metrics for **each stage** * `/applications/<application_id>/stages/<stage_id>/< stage_attempt_id >`: peak values of executor metrics for **each executor** for the stage, followed by peak values of executor metrics for the stage ### Why are the changes needed? The stage level peak executor metrics can help better understand your application's resource utilization. ### Does this PR introduce _any_ user-facing change? 1. For the `/applications/<application_id>/stages/` API, you will see the following new info for **each stage**: ```JSON "peakExecutorMetrics" : { "JVMHeapMemory" : 213367864, "JVMOffHeapMemory" : 189011656, "OnHeapExecutionMemory" : 0, "OffHeapExecutionMemory" : 0, "OnHeapStorageMemory" : 2133349, "OffHeapStorageMemory" : 0, "OnHeapUnifiedMemory" : 2133349, "OffHeapUnifiedMemory" : 0, "DirectPoolMemory" : 282024, "MappedPoolMemory" : 0, "ProcessTreeJVMVMemory" : 0, "ProcessTreeJVMRSSMemory" : 0, "ProcessTreePythonVMemory" : 0, "ProcessTreePythonRSSMemory" : 0, "ProcessTreeOtherVMemory" : 0, "ProcessTreeOtherRSSMemory" : 0, "MinorGCCount" : 13, "MinorGCTime" : 115, "MajorGCCount" : 4, "MajorGCTime" : 339 } ``` 2. For the `/applications/<application_id>/stages/<stage_id>/<stage_attempt_id>` API, you will see the following new info for **each executor** under `executorSummary`: ```JSON "peakMemoryMetrics" : { "JVMHeapMemory" : 0, "JVMOffHeapMemory" : 0, "OnHeapExecutionMemory" : 0, "OffHeapExecutionMemory" : 0, "OnHeapStorageMemory" : 0, "OffHeapStorageMemory" : 0, "OnHeapUnifiedMemory" : 0, "OffHeapUnifiedMemory" : 0, "DirectPoolMemory" : 0, "MappedPoolMemory" : 0, "ProcessTreeJVMVMemory" : 0, "ProcessTreeJVMRSSMemory" : 0, "ProcessTreePythonVMemory" : 0, "ProcessTreePythonRSSMemory" : 0, "ProcessTreeOtherVMemory" : 0, "ProcessTreeOtherRSSMemory" : 0, "MinorGCCount" : 0, "MinorGCTime" : 0, "MajorGCCount" : 0, "MajorGCTime" : 0 } ``` , and the following at the stage level: ```JSON "peakExecutorMetrics" : { "JVMHeapMemory" : 213367864, "JVMOffHeapMemory" : 189011656, "OnHeapExecutionMemory" : 0, "OffHeapExecutionMemory" : 0, "OnHeapStorageMemory" : 2133349, "OffHeapStorageMemory" : 0, "OnHeapUnifiedMemory" : 2133349, "OffHeapUnifiedMemory" : 0, "DirectPoolMemory" : 282024, "MappedPoolMemory" : 0, "ProcessTreeJVMVMemory" : 0, "ProcessTreeJVMRSSMemory" : 0, "ProcessTreePythonVMemory" : 0, "ProcessTreePythonRSSMemory" : 0, "ProcessTreeOtherVMemory" : 0, "ProcessTreeOtherRSSMemory" : 0, "MinorGCCount" : 13, "MinorGCTime" : 115, "MajorGCCount" : 4, "MajorGCTime" : 339 } ``` ### How was this patch tested? Added tests. Closes #29020 from imback82/metrics. Lead-authored-by: Terry Kim <[email protected]> Co-authored-by: edwinalu <[email protected]> Signed-off-by: Gengliang Wang <[email protected]>

stackedsax · 2020-08-11T18:56:50Z

@imback82 Looks like #29020 is merged into 3.1.0. Should this issue get closed now, or is it being left open for someone to do the same in 2.x versions of Spark?

imback82 · 2020-08-11T19:27:00Z

This can be closed now. We can use changes in #29020 if backporting to 2.4 is needed. (@dongjoon-hyun / @gengliangwang can confirm, but I don't think this will be backported to 2.4.)

github-actions · 2020-11-20T00:39:17Z

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.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

add stage level metrics

d77612c

squito reviewed Dec 18, 2018

View reviewed changes

rezasafi suggested changes Dec 18, 2018

View reviewed changes

code review comments

42599fe

squito reviewed Dec 19, 2018

View reviewed changes

update comments in AppStatusListener.scala

3dd8424

Merge branch 'master' into SPARK-23431

6d6f64c

dongjoon-hyun added the SPARK CORE label Jun 14, 2019

github-actions bot added the Stale label Jan 3, 2020

github-actions bot closed this Jan 4, 2020

dongjoon-hyun reopened this Jul 1, 2020

dongjoon-hyun removed the Stale label Jul 1, 2020

imback82 mentioned this pull request Jul 7, 2020

[SPARK-23431][CORE] Expose stage level peak executor metrics via REST API #29020

Closed

github-actions bot added the Stale label Nov 20, 2020

github-actions bot closed this Nov 21, 2020

[SPARK-23431][CORE] Expose the new executor memory metrics at the stage level #23340

[SPARK-23431][CORE] Expose the new executor memory metrics at the stage level #23340

Uh oh!

Conversation

edwinalu commented Dec 18, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

felixcheung commented Dec 18, 2018

Uh oh!

SparkQA commented Dec 18, 2018

Uh oh!

squito commented Dec 18, 2018

Uh oh!

squito left a comment

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!

rezasafi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edwinalu Dec 18, 2018 • 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!

SparkQA commented Dec 18, 2018

Uh oh!

SparkQA commented Dec 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 20, 2018

Uh oh!

SparkQA commented Apr 23, 2019

Uh oh!

edwinalu commented Apr 23, 2019

Uh oh!

github-actions bot commented Jan 3, 2020

Uh oh!

itamarst commented May 26, 2020

Uh oh!

stackedsax commented Jun 23, 2020

Uh oh!

dongjoon-hyun commented Jul 1, 2020

Uh oh!

AmplabJenkins commented Jul 1, 2020

Uh oh!

stackedsax commented Jul 1, 2020

Uh oh!

edwinalu commented Jul 1, 2020

Uh oh!

imback82 commented Jul 2, 2020

Uh oh!

itamarst commented Jul 2, 2020

Uh oh!

imback82 commented Jul 3, 2020

Uh oh!

imback82 commented Jul 7, 2020

Uh oh!

stackedsax commented Aug 11, 2020

Uh oh!

imback82 commented Aug 11, 2020

Uh oh!

github-actions bot commented Nov 20, 2020

Uh oh!

Reviewers

Assignees

edwinalu Dec 18, 2018 •

edited

Loading