[SPARK-30540][WEBUI] HistoryServer application link is incorrect when one application having multiple attempts

[zuston]

What changes were proposed in this pull request?

Add returning params of attempId in js script file.

Why are the changes needed?

When having multiple attempts, the applicaiton link is incorrect. No matter which attempt is clicked, the link still points to the largest attemptId.This patch fix it.

Does this PR introduce any user-facing change?

Yes, before screenshot as follows:

How was this patch tested?

WebUI involved, may not require testing

[dongjoon-hyun]

Hi, @zuston . Thank you for making a PR.
Could you attach the screenshot of before and after into the PR description?

[zuston]

Hi, @zuston . Thank you for making a PR.
Could you attach the screenshot of before and after into the PR description?

Of course, as follows.

[dongjoon-hyun]

I meant the following section in the PR description.

Does this PR introduce any user-facing change?

[attilapiros]

As I see as you introduced the attemptId instead of num now in the historypage-template.html there is no more usage of the num property (view data property). If it is not used can you please delete it.

[zuston]

Ok, i will remove it. Thanks~

[attilapiros]

Here the existence of the attemptId is checked but a few line below you are using it without checking its existence. When it is missing then the new link won't be valid too.

[zuston]

The attemptId is not checked with line of <td><a href="{{uiroot}}/history/{{id}}/{{attemptId}}/jobs/">{{attemptId}}</a></td> in historypage-template.html.

Need to ignore this link when attempId is empty or null? Do you have some ideas?

[attilapiros]

The {{uiroot}}/history/{{id}}/{{attemptId}}/jobs/">{{attemptId}}</a></td> is protected by the check {{#hasMultipleAttempts}} ... {{/hasMultipleAttempts}}.

I think if there is only one attempt then the attemptId might be missing and the value 1 was used for that before your change not null and empty (because of the line var num = app["attempts"].length;).

So please test with that case too.
And I would like to ask you to attach some data to the PR: during testing you can put a breakpoint (ie by placing debugger; into the code) before the Mustache renders the page (it is about line 161) please copy-paste the data content to a code block of the PR comment.

Could you please do that for both having multiple attempts and having only one?

[dongjoon-hyun]

+1 for @attilapiros 's comments.

[zuston]

The {{uiroot}}/history/{{id}}/{{attemptId}}/jobs/">{{attemptId}}</a></td> is protected by the check {{#hasMultipleAttempts}} ... {{/hasMultipleAttempts}}.

I think if there is only one attempt then the attemptId might be missing and the value 1 was used for that before your change not null and empty (because of the line var num = app["attempts"].length;).

So please test with that case too.
And I would like to ask you to attach some data to the PR: during testing you can put a breakpoint (ie by placing debugger; into the code) before the Mustache renders the page (it is about line 161) please copy-paste the data content to a code block of the PR comment.

Could you please do that for both having multiple attempts and having only one?

I think this change is ok. The debugger data as follows:

This pic is when hasMultipleAttempts is true and showCompletedColumns is false.

Next pic is when hasMultipleAttempts is false and showCompletedColumns is true. There is only one attempt then the attemptId also exist. But to prevent attemptID missing, I will set attemptID to 1 before it.

[attilapiros]

Thanks for the PR. Please remove num if it is not needed any more and check the case when attemptId is not available.

[dongjoon-hyun]

ok to test

[SparkQA]

Test build #117215 has finished for PR 27257 at commit 6b58c3d.

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

[SparkQA]

Test build #117285 has finished for PR 27257 at commit 4ea7042.

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

[attilapiros]

Your change must be tested with the missing attemptID and see whether that default value is fine.
We can use the git annotate to find the PR which introduced the checking of the property existence.

Here is the PR: #17795

As I see

{
  "id" : "application_1467878474947_0048",
  "name" : "Spark shell",
  "attempts" : [ {
    "startTime" : "2016-12-30T03:22:47.300GMT",
    "endTime" : "2016-12-30T03:33:42.460GMT",
    "lastUpdated" : "2017-04-27T14:02:35.282GMT",
    "duration" : 655160,
    "sparkUser" : "spark",
    "completed" : true,
    "startTimeEpoch" : 1483068167300,
    "endTimeEpoch" : 1483068822460,
    "lastUpdatedEpoch" : 1493301755282
  } ]
}

was a spark shell.
Please run a test where spark shell was started (another idea to check the eventlogs used as a resource file for testing).

[github-actions]

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!