[SPARK-33906][WEBUI] Fix the bug of UI Executor page stuck due to undefined peakMemoryMetrics #30920

baohe-zhang · 2020-12-24T20:35:55Z

What changes were proposed in this pull request?

Check if the executorSummary.peakMemoryMetrics is defined before accessing it. Without checking, the UI has risked being stuck at the Executors page.

Why are the changes needed?

App live UI may stuck at Executors page without this fix.
Steps to reproduce (with master branch):
In mac OS standalone mode, open a spark-shell
$SPARK_HOME/bin/spark-shell --master spark://localhost:7077

val x = sc.makeRDD(1 to 100000, 5)
x.count()

Then open the app UI in the browser, and click the Executors page, will get stuck at this page:

Also, the return JSON from API endpoint http://localhost:4040/api/v1/applications/app-20201224134418-0003/executors miss "peakMemoryMetrics" for executor objects. I attached the full json text in https://issues.apache.org/jira/browse/SPARK-33906.

I debugged it and observed that ExecutorMetricsPoller
.getExecutorUpdates returns an empty map, which causes peakExecutorMetrics to None in https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/status/LiveEntity.scala#L345. The possible reason for returning the empty map is that the stage completion time is shorter than the heartbeat interval, so the stage entry in stageTCMP has already been removed before the reportHeartbeat is called.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manual test, rerun the steps of bug reproduce and see the bug is gone.

SparkQA · 2020-12-24T21:42:06Z

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

dongjoon-hyun · 2020-12-24T21:50:39Z

Hi, @baohe-zhang . Is this caused by #30186 ?

dongjoon-hyun · 2020-12-24T22:01:31Z

Thank you for your contribution, @baohe-zhang . Although this sounds like a serious regression in terms of UI, the code review might take a longer time because it's a holiday season.

Happy Christmas and New Year!

baohe-zhang · 2020-12-24T22:07:59Z

Thank you, happy Christmas!

SparkQA · 2020-12-24T22:14:09Z

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

SparkQA · 2020-12-24T22:41:56Z

Test build #133360 has finished for PR 30920 at commit c3ab7a1.

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

dongjoon-hyun · 2020-12-24T22:45:57Z

Retest this please.

SparkQA · 2020-12-24T23:31:12Z

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

SparkQA · 2020-12-25T00:05:07Z

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

SparkQA · 2020-12-25T01:10:21Z

Test build #133363 has finished for PR 30920 at commit c3ab7a1.

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

sarutak · 2020-12-25T01:13:48Z

Hi @baohe-zhang , could you check this PR?
That PR solves similar issue.

sarutak · 2020-12-25T01:39:29Z

We can reproduce this issue with --master local-cluster[1,1,1024] more easily.

AngersZhuuuu · 2020-12-25T03:16:37Z

Hi @baohe-zhang , could you check this PR?
That PR solves similar issue.

Oh, my bad, seem I referenced this part code when making my pr, but change my part, didn't notice these code have problem too. A good catch.

baohe-zhang · 2020-12-25T04:01:12Z

Hi @sarutak that PR fixed the same issue in stages tab, and the current PR fix issue in executors tab. Should I handle the case of peakMemoryMetrics undefined similar to that PR, like return '0.0 B / 0.0 B' instead of empty string?

sarutak · 2020-12-25T04:43:46Z

@baohe-zhang
I think it's better to do with the same way as that PR.
But I wonder it's more proper to show N/A rather than 0.0B / 0.0B to intend it's not updated yet when peakMemoryMetrics is undefined.

sarutak · 2020-12-25T05:05:17Z

core/src/main/resources/org/apache/spark/ui/static/executorspage.js

@@ -414,6 +414,9 @@ $(document).ready(function () {
                        },
                        {
                            data: function (row, type) {
+                                if (typeof row.peakMemoryMetrics == "undefined") {


It's better to use === and !== rather than == and !=.

baohe-zhang · 2020-12-25T06:32:40Z

@sarutak I updated it to show N/A when undefined.

sarutak · 2020-12-25T07:21:28Z

core/src/main/resources/org/apache/spark/ui/static/executorspage.js

+                                            formatBytes(peakMemoryMetrics.JVMOffHeapMemory, type));
+                                } else {
+                                    if (type !== 'display') {
+                                        return 0;


I think it's better to return "" rather than 0 like done here.

Could you update the similar logic in the previous PR too?

Actually in https://github.com/apache/spark/blob/master/core/src/main/resources/org/apache/spark/ui/static/stagepage.js#L501, it also returns 0.
If the column is invisible, it seems fine to return anything.

Yes, that returns 0 but I wonder it's impossible for us to tell that "the metrics is not updated yet" from "that the metrics is actually 0".

SparkQA · 2020-12-25T07:27:06Z

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

SparkQA · 2020-12-25T07:31:37Z

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

sarutak · 2020-12-25T07:38:22Z

Sorry I suggested N/A but "" seems to be preferred in other places for now so let's go with "" for now for consistency.

SparkQA · 2020-12-25T09:02:36Z

Test build #133374 has finished for PR 30920 at commit e7154fd.

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

HyukjinKwon · 2020-12-28T06:38:48Z

cc @gengliangwang as well.

gengliangwang · 2020-12-28T13:43:52Z

core/src/main/resources/org/apache/spark/ui/static/executorspage.js

+                                    if (type !== 'display') {
+                                        return 0;
+                                    } else {
+                                        return 'N/A';


How about returning '0.0 B / 0.0 B' like
https://github.com/apache/spark/blob/master/core/src/main/resources/org/apache/spark/ui/static/stagepage.js#L503

Done. Replaced 'N/A' with '0.0 B / 0.0 B'

gengliangwang

Thanks for the fix!

SparkQA · 2020-12-28T19:27:59Z

Test build #133455 has finished for PR 30920 at commit 2f7b9be.

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

sarutak

I have a concern mentioned here but it's better to focus to resolve the stuck issue for now.

dongjoon-hyun

+1, LGTM. Thank you all. I agree with you. Let's fix this stuck issue first

…efined peakMemoryMetrics ### What changes were proposed in this pull request? Check if the executorSummary.peakMemoryMetrics is defined before accessing it. Without checking, the UI has risked being stuck at the Executors page. ### Why are the changes needed? App live UI may stuck at Executors page without this fix. Steps to reproduce (with master branch): In mac OS standalone mode, open a spark-shell $SPARK_HOME/bin/spark-shell --master spark://localhost:7077 val x = sc.makeRDD(1 to 100000, 5) x.count() Then open the app UI in the browser, and click the Executors page, will get stuck at this page: ![image](https://user-images.githubusercontent.com/26694233/103105677-ca1a7380-45f4-11eb-9245-c69f4a4e816b.png) Also, the return JSON from API endpoint http://localhost:4040/api/v1/applications/app-20201224134418-0003/executors miss "peakMemoryMetrics" for executor objects. I attached the full json text in https://issues.apache.org/jira/browse/SPARK-33906. I debugged it and observed that ExecutorMetricsPoller .getExecutorUpdates returns an empty map, which causes peakExecutorMetrics to None in https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/status/LiveEntity.scala#L345. The possible reason for returning the empty map is that the stage completion time is shorter than the heartbeat interval, so the stage entry in stageTCMP has already been removed before the reportHeartbeat is called. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual test, rerun the steps of bug reproduce and see the bug is gone. Closes #30920 from baohe-zhang/SPARK-33906. Authored-by: Baohe Zhang <baohe.zhang@verizonmedia.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 45df6db) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

dongjoon-hyun · 2020-12-31T21:37:09Z

Since this is caused by SPARK-23432 (#30186), this is backported to branch-3.1 to unblock Apache Spark RC1.

HeartSaVioR · 2021-01-01T04:30:23Z

Late +1. Thanks @baohe-zhang for the fix!

HyukjinKwon · 2021-01-04T00:51:58Z

Thanks all. I am going to start working on cutting RC1 today.

Check if the peakMemoryMetrics is undefined before accessing it

c3ab7a1

github-actions bot added CORE WEB UI labels Dec 24, 2020

sarutak reviewed Dec 25, 2020

View reviewed changes

Update display text to N/A when peakMemoryMetrics is undefined

e7154fd

sarutak reviewed Dec 25, 2020

View reviewed changes

gengliangwang reviewed Dec 28, 2020

View reviewed changes

Update display text to 0.0 B / 0.0 B

2f7b9be

gengliangwang approved these changes Dec 28, 2020

View reviewed changes

sarutak approved these changes Dec 30, 2020

View reviewed changes

dongjoon-hyun approved these changes Dec 31, 2020

View reviewed changes

dongjoon-hyun closed this in 45df6db Dec 31, 2020

[SPARK-33906][WEBUI] Fix the bug of UI Executor page stuck due to undefined peakMemoryMetrics #30920

[SPARK-33906][WEBUI] Fix the bug of UI Executor page stuck due to undefined peakMemoryMetrics #30920

Conversation

baohe-zhang commented Dec 24, 2020 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Dec 24, 2020

dongjoon-hyun commented Dec 24, 2020 • edited Loading

dongjoon-hyun commented Dec 24, 2020

baohe-zhang commented Dec 24, 2020

SparkQA commented Dec 24, 2020

SparkQA commented Dec 24, 2020

dongjoon-hyun commented Dec 24, 2020

SparkQA commented Dec 24, 2020

SparkQA commented Dec 25, 2020

SparkQA commented Dec 25, 2020

sarutak commented Dec 25, 2020

sarutak commented Dec 25, 2020

AngersZhuuuu commented Dec 25, 2020

baohe-zhang commented Dec 25, 2020

sarutak commented Dec 25, 2020

sarutak Dec 25, 2020

Choose a reason for hiding this comment

baohe-zhang Dec 25, 2020

Choose a reason for hiding this comment

baohe-zhang commented Dec 25, 2020

sarutak Dec 25, 2020

Choose a reason for hiding this comment

gengliangwang Dec 28, 2020

Choose a reason for hiding this comment

sarutak Dec 30, 2020

Choose a reason for hiding this comment

SparkQA commented Dec 25, 2020

SparkQA commented Dec 25, 2020

sarutak commented Dec 25, 2020 • edited Loading

SparkQA commented Dec 25, 2020

HyukjinKwon commented Dec 28, 2020

gengliangwang Dec 28, 2020

Choose a reason for hiding this comment

baohe-zhang Dec 28, 2020

Choose a reason for hiding this comment

gengliangwang left a comment

Choose a reason for hiding this comment

SparkQA commented Dec 28, 2020

sarutak left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Dec 31, 2020

HeartSaVioR commented Jan 1, 2021

HyukjinKwon commented Jan 4, 2021

baohe-zhang commented Dec 24, 2020 •

edited

Loading

dongjoon-hyun commented Dec 24, 2020 •

edited

Loading

sarutak commented Dec 25, 2020 •

edited

Loading