[SPARK-32886][WEBUI] fix 'undefined' link in event timeline view #29757

zhli1142015 · 2020-09-15T08:21:01Z

What changes were proposed in this pull request?

Fix ".../jobs/undefined" link from "Event Timeline" in jobs page. Job page link in "Event Timeline" view is constructed by fetching job page link defined in job list below. when job count exceeds page size of job table, only links of jobs in job table can be fetched from page. Other jobs' link would be 'undefined', and links of them in "Event Timeline" are broken, they are redirected to some wired URL like ".../jobs/undefined". This PR is fixing this wrong link issue. With this PR, job link in "Event Timeline" view would always redirect to correct job page.

Why are the changes needed?

Wrong link (".../jobs/undefined") in "Event Timeline" of jobs page. for example, the first job in below page is not in table below, as job count(116) exceeds page size(100). When clicking it's item in "Event Timeline", page is redirected to ".../jobs/undefined", which is wrong. Links in "Event Timeline" should always be correct.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manually tested.

zhli1142015 · 2020-09-15T08:26:43Z

@sarutak , @dongjoon-hyun, could you please take a look, thanks.

srowen · 2020-09-15T13:51:39Z

core/src/main/resources/org/apache/spark/ui/static/timeline-view.js

       return "#job-" + jobId;
      };

+      var getPathForJobEntry = function(baseElem) {


No big deal but why not just inline this?

Thanks @srowen . Yes, this is good idea, updated.

srowen · 2020-09-15T13:51:50Z

Jenkins test this please

sarutak · 2020-09-15T15:31:06Z

core/src/main/resources/org/apache/spark/ui/static/timeline-view.js

      $(this).click(function() {
        var jobPagePath = $(getSelectorForJobEntry(this)).find("a.name-link").attr("href");
-          window.location.href = jobPagePath
+        if (jobPagePath == undefined) {


Do we need this if branch?
If we can unify the two pieces of code for jobPagePath with the new logic, can we remove away this if branch?

Thanks @sarutak . we should remove it and use new logic, updated.

sarutak · 2020-09-15T15:36:03Z

@zhli1142015 Thanks for the fix. The same issue can be in JobPage. Could you confirm it?

sarutak · 2020-09-15T15:52:12Z

core/src/main/resources/org/apache/spark/ui/static/timeline-view.js


+      var getPathForJobEntry = function(baseElem) {
+        var jobIdText = $($(baseElem).find(".application-timeline-content")[0]).text();
+        var jobId = jobIdText.match("\\(Job (\\d+)\\)$")[1];


jobIdText and jobId are calculated here so can we reuse them to avoid additional overhead of jQuery selector and regex?

Thanks for your comments. i extracted common function getIdForJobEntry for selector and regex, and moved it out of each loop. It is only triggered by click and hover events. i think this is ok here.

SparkQA · 2020-09-15T16:53:29Z

Test build #128715 has finished for PR 29757 at commit 00f5c60.

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

zhli1142015 · 2020-09-16T00:07:32Z

@zhli1142015 Thanks for the fix. The same issue can be in JobPage. Could you confirm it?

Good catch, this also happens for job page. i fixed it in this PR also.

sarutak · 2020-09-18T01:53:44Z

core/src/main/resources/org/apache/spark/ui/static/timeline-view.js

+    var jobIdText = $($(baseElem).find(".application-timeline-content")[0]).text();
+    var jobId = jobIdText.match("\\(Job (\\d+)\\)$")[1];
+    return jobId;
+  };


nit: ; is not needed at the end of functions here and other new functions you added.

@sarutak , good catch, removed them, thanks.

sarutak · 2020-09-18T02:03:22Z

core/src/main/resources/org/apache/spark/ui/static/timeline-view.js

-          window.location.href = jobPagePath
+        var jobId = getIdForJobEntry(this);
+        // trim last '/' if exists and append '/job/' segment and id
+        var jobPagePath = window.location.pathname.replace(/\/$/, "/job/?id=" + jobId);


It might be better to use the same logic as this to build paths.

agreed, this should be a better approach, updated.

sarutak · 2020-09-18T02:04:20Z

@zhli1142015 Thanks for the update. I've left two minor comments.

dongjoon-hyun

Thank you for pinging me, @zhli1142015 . The patch looks good to me since it's revised already.

Just one question, this bug seems to exist in branch-2.4, doesn't it? If then, could you update the affected version in SPARK-32886 accordingly? Currently, the affected versions are 3.0.0 and 3.1.0.

zhli1142015 · 2020-09-18T05:37:27Z

Thank you for pinging me, @zhli1142015 . The patch looks good to me since it's revised already.

Just one question, this bug seems to exist in branch-2.4, doesn't it? If then, could you update the affected version in SPARK-32886 accordingly? Currently, the affected versions are 3.0.0 and 3.1.0.

Thanks for your comments. I just verified, this bug can be reproduced in 2.4.4. updated the JIRA.

srowen · 2020-09-20T17:09:45Z

Jenkins retest this please

SparkQA · 2020-09-20T19:53:00Z

Test build #128919 has finished for PR 29757 at commit 40d94f6.

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

### What changes were proposed in this pull request? Fix ".../jobs/undefined" link from "Event Timeline" in jobs page. Job page link in "Event Timeline" view is constructed by fetching job page link defined in job list below. when job count exceeds page size of job table, only links of jobs in job table can be fetched from page. Other jobs' link would be 'undefined', and links of them in "Event Timeline" are broken, they are redirected to some wired URL like ".../jobs/undefined". This PR is fixing this wrong link issue. With this PR, job link in "Event Timeline" view would always redirect to correct job page. ### Why are the changes needed? Wrong link (".../jobs/undefined") in "Event Timeline" of jobs page. for example, the first job in below page is not in table below, as job count(116) exceeds page size(100). When clicking it's item in "Event Timeline", page is redirected to ".../jobs/undefined", which is wrong. Links in "Event Timeline" should always be correct. ![undefinedlink](https://user-images.githubusercontent.com/10524738/93184779-83fa6d80-f6f1-11ea-8a80-1a304ca9cbb2.JPG) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Manually tested. Closes #29757 from zhli1142015/fix-link-event-timeline-view. Authored-by: Zhen Li <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit d01594e) Signed-off-by: Sean Owen <[email protected]>

srowen · 2020-09-21T14:06:20Z

Merged to master/3.0/2.4

sarutak · 2020-09-21T16:14:15Z

Hi all, I noticed it's not work for branch-2.4 because appBasePath is absent for branch-2.4.
It's introduced by SPARK-31882 but it's not backported into branch-2.4.

I've made a PR to revert this change for branch-2.4 (#29825).

### What changes were proposed in this pull request? This PR reverts SPARK-32886 (#29757) for branch-2.4. That change needs `appBasePath` in `webui.js` but it's absent for `branch-2.4`. Closes #29825 from sarutak/hotfix-for-SPARK-32886-2.4. Authored-by: Kousuke Saruta <[email protected]> Signed-off-by: Kousuke Saruta <[email protected]>

zhli1142015 · 2020-09-21T22:39:28Z

Hi all, I noticed it's not work for branch-2.4 because appBasePath is absent for branch-2.4.
It's introduced by SPARK-31882 but it's not backported into branch-2.4.

I've made a PR to revert this change for branch-2.4 (#29825).

Thanks @sarutak . I checked issue described in SPARK-31882 also exists in branch-2.4, is it ok to backport related fix to branch-2.4? Or, should I create a independent fix targeting branch-2.4 for this issue?

dongjoon-hyun · 2020-09-22T03:18:59Z

@zhli1142015 . Yes. You can make an independent PR to branch-2.4.

### What changes were proposed in this pull request? Fix ".../jobs/undefined" link from "Event Timeline" in jobs page. Job page link in "Event Timeline" view is constructed by fetching job page link defined in job list below. when job count exceeds page size of job table, only links of jobs in job table can be fetched from page. Other jobs' link would be 'undefined', and links of them in "Event Timeline" are broken, they are redirected to some wired URL like ".../jobs/undefined". This PR is fixing this wrong link issue. With this PR, job link in "Event Timeline" view would always redirect to correct job page. ### Why are the changes needed? Wrong link (".../jobs/undefined") in "Event Timeline" of jobs page. for example, the first job in below page is not in table below, as job count(116) exceeds page size(100). When clicking it's item in "Event Timeline", page is redirected to ".../jobs/undefined", which is wrong. Links in "Event Timeline" should always be correct. ![undefinedlink](https://user-images.githubusercontent.com/10524738/93184779-83fa6d80-f6f1-11ea-8a80-1a304ca9cbb2.JPG) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Manually tested. Closes apache#29757 from zhli1142015/fix-link-event-timeline-view. Authored-by: Zhen Li <[email protected]> Signed-off-by: Sean Owen <[email protected]>

zhli1142015 · 2020-09-22T07:48:03Z

@zhli1142015 . Yes. You can make an independent PR to branch-2.4.

Thanks, I create a PR to cherry-pick both SPARK-31882 and this change to branch-2.4. First, issue mentioned in SPARK-31882 also exists in branch-2.4. Second, if removing dependency of SPARK-31882, we need a different way to fix this issue, this fix looks better to me.
Please let me know if we have different principle for such problem.
Cherry pick PR: #29833

…timeline view ### What changes were proposed in this pull request? Fix [SPARK-32886](https://issues.apache.org/jira/projects/SPARK/issues/SPARK-32886) in branch-2.4. i cherry-pick [29757](#29757) and partial [28690](#28690 part is ignored as conflict), which PR `29757` has dependency on PR `28690`. This change fixes two below issues in branch-2.4. [SPARK-31882: DAG-viz is not rendered correctly with pagination.](https://issues.apache.org/jira/projects/SPARK/issues/SPARK-31882) [SPARK-32886: '.../jobs/undefined' link from "Event Timeline" in jobs page](https://issues.apache.org/jira/projects/SPARK/issues/SPARK-32886) ### Why are the changes needed? sarutak found `29757` has dependency on `28690`. If we only merge `29757` to 2.4 branch, it would cause UI break. And I verified both issues mentioned in [SPARK-32886](https://issues.apache.org/jira/projects/SPARK/issues/SPARK-32886) and [SPARK-31882](https://issues.apache.org/jira/projects/SPARK/issues/SPARK-31882) exist in branch-2.4. So i cherry pick them to branch 2.4 in same PR. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manually tested. ![dag](https://user-images.githubusercontent.com/10524738/93854440-9c770480-fc6a-11ea-9009-0ee68ef090e1.JPG) ![eventTLJPG](https://user-images.githubusercontent.com/10524738/93854447-a13bb880-fc6a-11ea-9b34-73623fa59def.JPG) Closes #29833 from zhli1142015/cherry-pick-fix-for-31882-32886. Lead-authored-by: Zhen Li <[email protected]> Co-authored-by: Kousuke Saruta <[email protected]> Signed-off-by: Sean Owen <[email protected]>

### What changes were proposed in this pull request? Fix ".../jobs/undefined" link from "Event Timeline" in jobs page. Job page link in "Event Timeline" view is constructed by fetching job page link defined in job list below. when job count exceeds page size of job table, only links of jobs in job table can be fetched from page. Other jobs' link would be 'undefined', and links of them in "Event Timeline" are broken, they are redirected to some wired URL like ".../jobs/undefined". This PR is fixing this wrong link issue. With this PR, job link in "Event Timeline" view would always redirect to correct job page. ### Why are the changes needed? Wrong link (".../jobs/undefined") in "Event Timeline" of jobs page. for example, the first job in below page is not in table below, as job count(116) exceeds page size(100). When clicking it's item in "Event Timeline", page is redirected to ".../jobs/undefined", which is wrong. Links in "Event Timeline" should always be correct. ![undefinedlink](https://user-images.githubusercontent.com/10524738/93184779-83fa6d80-f6f1-11ea-8a80-1a304ca9cbb2.JPG) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Manually tested. Closes apache#29757 from zhli1142015/fix-link-event-timeline-view. Authored-by: Zhen Li <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit d01594e) Signed-off-by: Sean Owen <[email protected]>

fix 'undefined' link in event timeline view

970c2f5

probot-autolabeler bot added CORE WEB UI labels Sep 15, 2020

minor change

00f5c60

srowen reviewed Sep 15, 2020

View reviewed changes

sarutak reviewed Sep 15, 2020

View reviewed changes

address comments

b243dc3

zhli1142015 requested review from sarutak and srowen September 16, 2020 00:12

sarutak reviewed Sep 18, 2020

View reviewed changes

minor change

40d94f6

zhli1142015 requested a review from sarutak September 18, 2020 02:35

dongjoon-hyun reviewed Sep 18, 2020

View reviewed changes

sarutak approved these changes Sep 19, 2020

View reviewed changes

srowen approved these changes Sep 21, 2020

View reviewed changes

srowen closed this in d01594e Sep 21, 2020

sarutak mentioned this pull request Sep 21, 2020

[HOTFIX][2.4] Revert SPARK-32886 #29825

Closed

zhli1142015 mentioned this pull request Sep 22, 2020

[SPARK-32886][SPARK-31882][WEBUI][2.4] fix 'undefined' link in event timeline view #29833

Closed

[SPARK-32886][WEBUI] fix 'undefined' link in event timeline view #29757

[SPARK-32886][WEBUI] fix 'undefined' link in event timeline view #29757

Uh oh!

Conversation

zhli1142015 commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

zhli1142015 commented Sep 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Sep 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhli1142015 Sep 16, 2020 • 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!

sarutak commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 15, 2020

Uh oh!

zhli1142015 commented Sep 16, 2020

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!

sarutak commented Sep 18, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

zhli1142015 commented Sep 18, 2020

Uh oh!

srowen commented Sep 20, 2020

Uh oh!

SparkQA commented Sep 20, 2020

Uh oh!

srowen commented Sep 21, 2020

Uh oh!

sarutak commented Sep 21, 2020

Uh oh!

zhli1142015 commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Sep 22, 2020

Uh oh!

zhli1142015 commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zhli1142015 commented Sep 15, 2020 •

edited

Loading

zhli1142015 Sep 16, 2020 •

edited

Loading

sarutak commented Sep 15, 2020 •

edited

Loading

zhli1142015 commented Sep 21, 2020 •

edited

Loading