Skip to content

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

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

Closed
zuston wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions core/src/main/resources/org/apache/spark/ui/static/historypage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ $(document).ready(function() {
if (app["attempts"].length > 1) {
hasMultipleAttempts = true;
}
var num = app["attempts"].length;
var defaultNum = 1;
for (var j in app["attempts"]) {
var attempt = app["attempts"][j];
attempt["startTime"] = formatTimeMillis(attempt["startTimeEpoch"]);
Expand All @@ -140,7 +140,8 @@ $(document).ready(function() {
(attempt.hasOwnProperty("attemptId") ? attempt["attemptId"] + "/" : "") + "logs";
Copy link
Contributor

@attilapiros attilapiros Jan 17, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@zuston zuston Jan 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@attilapiros attilapiros Jan 20, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member

@dongjoon-hyun dongjoon-hyun Jan 22, 2020

Choose a reason for hiding this comment

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

+1 for @attilapiros 's comments.

Copy link
Member Author

@zuston zuston Jan 23, 2020

Choose a reason for hiding this comment

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

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.
reliao_img_1579763662512

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.
reliao_img_1579764031165

attempt["durationMillisec"] = attempt["duration"];
attempt["duration"] = formatDuration(attempt["duration"]);
var app_clone = {"id" : id, "name" : name, "version": version, "num" : num, "attempts" : [attempt]};
var attempId = attempt.hasOwnProperty("attemptId") ? attempt["attemptId"] : defaultNum;
var app_clone = {"id" : id, "name" : name, "version": version, "num" : attempId, "attempts" : [attempt]};
array.push(app_clone);
}
}
Expand Down