Skip to content

Conversation

@kuwii
Copy link
Contributor

@kuwii kuwii commented Jun 27, 2022

What changes were proposed in this pull request?

Updated REST API /api/v1/applications, to use the same condition as history server page to filter completed/incomplete applications.

Why are the changes needed?

When opening summary page, history server follows this logic:

  • If there's completed/incomplete application, page will add script in response, using AJAX to call the REST API to get the filtered list.
  • If there's no such application, page will only return a message telling nothing found.

Issue is that page and REST API are using different conditions to filter applications. In HistoryPage, an application is considered as completed as long as the last attempt is completed. But in ApplicationListResource, all attempts should be completed. This brings inconsistency and will cause issue in a corner case.

In driver, event queues have capacity to protect memory. When there's too many events, some of them will be dropped and the event log file will be incomplete. For an application with multiple attempts, there's possibility that the last attempt is completed, but the previous attempts is considered as incomplete due to loss of application end event.

For this type of application, page thinks it is completed, but the API thinks it is still running. When opening summary page:

  • When checking completed applications, page will call script, but API returns nothing.
  • When checking incomplete applications, page returns nothing.

So the user won't be able to see this app in history server.

Does this PR introduce any user-facing change?

Yes, there will be a change on /api/v1/applications API and history server summary page.

When calling API, for application mentioned above, previously it is considered as running. After the change it is considered as completed. So the result will be different using same filter. But this change should be OK. Because attempts are executed sequentially and incrementally. So if an attempt with bigger ID is completed, the previous attempts can be considered as completed.

For history server summary page, previously user is not able to see the application. Now it will appear in the completed applications.

How was this patch tested?

Add a new unit test HistoryServerPageSuite, which will check whether HistoryPage behaves the same as ApplicationListResource when filtering applications. To implement the test, there's a minor change of HistoryPage, exposing a method called shouldDisplayApplications to tell whether the summary page will display applications.

The test verifies that:

  • If no completed/incomplete application found, HistoryPage should not display applications, and API should return an empty list.
  • Otherwise, HistoryPage should display applications, and API should return a non-empty list.

Currently 2 scenarios are included:

  • Application with last attempt completed but previous attempt incomplete.
  • Application with last attempt incomplete but previous attempt completed.

@github-actions github-actions bot added the CORE label Jun 27, 2022
@kuwii kuwii marked this pull request as ready for review June 28, 2022 08:31
@kuwii kuwii changed the title [SPARK-39620][Web UI][WIP] Use same condition in history server page and API to filter applications [SPARK-39620][Web UI Use same condition in history server page and API to filter applications Jun 28, 2022
@kuwii kuwii changed the title [SPARK-39620][Web UI Use same condition in history server page and API to filter applications [SPARK-39620][Web UI] Use same condition in history server page and API to filter applications Jun 28, 2022
@kuwii
Copy link
Contributor Author

kuwii commented Jun 28, 2022

Kindly ping @squito @srowen @smurakozi

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Seems OK, though I don't know this part well.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@kuwii kuwii requested a review from srowen July 7, 2022 14:11
@srowen
Copy link
Member

srowen commented Jul 7, 2022

Hm, can you try retriggering tests? I'm not clear if it's related

@kuwii
Copy link
Contributor Author

kuwii commented Jul 8, 2022

Hm, can you try retriggering tests? I'm not clear if it's related

Have rerun the test. Checks have passed.

@srowen
Copy link
Member

srowen commented Jul 8, 2022

Merged to master

@srowen srowen closed this in 3331d4c Jul 8, 2022
@kuwii kuwii deleted the kuwii/hs-fix branch November 6, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants