[SPARK-20517][UI] Fix broken history UI download link

[jerryshao]

What changes were proposed in this pull request?

The download link in history server UI is concatenated with:

 <td><a href="{{uiroot}}/api/v1/applications/{{id}}/{{num}}/logs" class="btn btn-info btn-mini">Download</a></td>

Here num field represents number of attempts, this is not equal to REST APIs. In the REST API, if attempt id is not existed the URL should be api/v1/applications/<id>/logs, otherwise the URL should be api/v1/applications/<id>/<attemptId>/logs. Using <num> to represent <attemptId> will lead to the issue of "no such app".

How was this patch tested?

Manual verification.

CC @ajbozarth can you please review this change, since you add this feature before? Thanks!

[SparkQA]

Test build #76253 has finished for PR 17795 at commit 3fdba11.

• This patch fails from timeout after a configured wait of `250m`.
• This patch merges cleanly.
• This patch adds no public classes.

[jerryshao]

Jenkins, retest this please.

[SparkQA]

Test build #76259 has started for PR 17795 at commit 3fdba11.

[jerryshao]

Jenkins, retest this please.

[SparkQA]

Test build #76262 has finished for PR 17795 at commit 3fdba11.

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

[ajbozarth]

This is a clean way of doing this, but my reasoning behind how I did it was from my understanding even if there's only one attempt you could still access it with an attempt id, I actually went into the code to check that it worked that way. I'm just wondering if there's a need to add this if it works as is, but if the way I had it originally is just a loophole I'm fine changing it.

[jerryshao]

@ajbozarth the key point is that several spark applications doesn't have attempt id. It's not related to one attempt or two, for example:

{
  "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
  } ]
}

This app doesn't have attempt id, so using api/v1/applications/<application_1467878474947_0048>/1/logs to get log will be failed in here, I think this related to my recent changes, you could verify it on latest master.

[jerryshao]

This is due to my changes in #17582 , with this change, download API will verify with correct ACL, so if attemptId is not found, then withSparkUI will be failed to get correct SparkUI to do verification.

[jerryshao]

@vanzin , can you please review this PR, thanks! The download link is broken after this change #17582 . Now it will check SparkUI with given appid and attemptId. Previous way of setting attemptId in JS is not correct.

[ajbozarth]

Oh ok, I see why this is needed now that I see how #17582 changes how the api works, though I'm a bit worried this might not be the only thing that might've broken. You may want to look around for other cases like this where attempt ID is used for apps without one since prior to your change the api understood attempt id = 0 was the same as no attempt id.

Having said that, this LGTM

[jerryshao]

@ajbozarth OK, I will verify it.

[jerryshao]

@ajbozarth , I checked the UI related to attemptId, seems fine without issue. Can you please point out in which code potentially has the regression? Thanks!

[jerryshao]

I assume only event log download will be effected with #17582 .

[ajbozarth]

LGTM

[vanzin]

Merging to master / 2.2 / 2.1 / 2.0. There's a missing space in the code but I'll fix that during merge.

[vanzin]

@jerryshao the patch didn't merge to 2.0, but your other change is there, so you should probably take a look.

[jerryshao]

Thanks @vanzin , let me submit a patch for branch 2.0.

[jerryshao]

@vanzin , since branch 2.0 doesn't have this feature in UI (https://issues.apache.org/jira/browse/SPARK-11272), so I don't think it is required to fix in branch-2.0.