[SPARK-30654][WebUI] Bootstrap4 WebUI upgrade #27370

clarkead · 2020-01-27T22:42:16Z

What changes were proposed in this pull request?

Spark's Web UI is using an older version of Bootstrap (v. 2.3.2) for the portal pages. Bootstrap 2.x was moved to EOL in Aug 2013 and Bootstrap 3.x was moved to EOL in July 2019 (https://github.com/twbs/release). Older versions of Bootstrap are also getting flagged in security scans for various CVEs:

https://snyk.io/vuln/SNYK-JS-BOOTSTRAP-72889
https://snyk.io/vuln/SNYK-JS-BOOTSTRAP-173700
https://snyk.io/vuln/npm:bootstrap:20180529
https://snyk.io/vuln/npm:bootstrap:20160627

I haven't validated each CVE, but it would be nice to resolve any potential issues and get on a supported release.

The bad news is that there have been quite a few changes between Bootstrap 2 and Bootstrap 4. I've tried updating the library, refactoring/tweaking the CSS and JS to maintain a similar appearance and functionality, and testing the UI for functionality and appearance. This is a fairly large change so I'm sure additional testing and fixes will be needed.

How was this patch tested?

This has been manually tested, but there is a ton of functionality and there are many pages and detail pages so it is very possible bugs introduced from the upgrade were missed. Additional testing and feedback is welcomed. If it appears a whole page was missed let me know and I'll take a pass at addressing that page/section.

srowen · 2020-01-28T14:53:09Z

Probably a good idea, yes, but hard to evaluate indeed. Does this update jquery too?
The LICENSE file or licenses/ might need to change just to reflect the new file names.
As far as you know this doesn't change functionality, or doesn't intend to, right?

srowen · 2020-01-28T14:53:18Z

Jenkins test this please

SparkQA · 2020-01-28T14:59:56Z

Test build #117485 has finished for PR 27370 at commit 5720577.

This patch fails RAT tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public final class ColumnDictionary implements Dictionary

…ster to this issue branch.

clarkead · 2020-01-29T00:33:25Z

Thank you for taking a look. The intention with this patch is not to change any functionality. The goal is to just migrate Spark's WebUI onto a supported Bootstrap release. Due to Bootstrap changing some of their default CSS the font sizes, paddings, and margins did change. As a result, I made some overrides in the CSS and some minor changes to the HTML markup to try and maintain the existing feel, but there are still minor differences. It should be noted that the main navbar does collapse to a toggle button now for mobile devices. Previously when viewed on a mobile device or small browser window the top navbar would become partially cut off.

jQuery was not modified as it was already on version 3.4.1, but dataTables.bootstrap.min.js was updated to the latest dataTables.bootstrap4.min.js and jquery.dataTables.1.10.18.min.js was updated to jquery.dataTables.min.js (version 1.10.20). This was mostly needed for compatibility with the latest Bootstrap 4 release.

I noticed this afternoon my initial PR had an issue with my upstream merge so I've added a new commit to resolve it. I'll look into the reported license issue.

srowen · 2020-01-29T15:48:01Z

Yeah, I do think we need to make a change like this at some point. Thanks for doing the legwork. My only hesitation about merging it soon is that we're about to be in 3.0 code freeze, and it sounds like this may need a little testing. On the one hand, we'll be doing some of that in the 3.0 QA period, so I'm not against it (if you can resolve merge conflicts and feel as confident as you can this works). We can also push back to consider post 3.0.

…eck excluded files.

clarkead · 2020-01-29T19:43:04Z

I've updated the Bootstrap license to reflect the license change to MIT. I've also updated the dataTables license to reflect the updated date range. Finally, I had to update the dev/.rat-excludes to reflect the file name changes and additional files provided by the third party libraries.

clarkead · 2020-01-29T21:20:32Z

As far as when to merge it goes, while I'd like to see it merged as part of the 3.0 release I understand the concern regarding the risk it introduces so I'm good if you want to push it to after 3.0 is out the door. In either case I'll support merge conflicts and will try to resolve visual bug defects found.

As far as my confidence level in the patch, I've tested it for the use cases and pages that I know of, but I'm sure there are areas that I don't know about or missed. I am confident though that any issues issues found can be quickly resolved.

gengliangwang · 2020-01-30T01:19:13Z

@clarkead Thanks for the work. I am +1 with the upgrade(actually it was in my backlog).
Could you resolve the code conflict? I will help do some manual testing and code review.

gengliangwang · 2020-02-28T23:44:23Z

core/src/main/resources/org/apache/spark/ui/static/executorspage-template.html

-    <div class="container-fluid">
-      <table id="active-executors-table" class="table table-striped compact cell-border">
-        <thead>
+    <table id="summary-execs-table" class="table table-striped compact cell-border" style="width: 100%">


It seems that this file only removes the element <div class="container-fluid"> and add style="width: 100%"
Could you keep the other part of the file unchanged so that it is easier for review?

Thank you for reviewing the PR. I just submitted a PR to review the indent formatting updates to make the review easier.

gengliangwang · 2020-02-28T23:50:40Z

@clarkead I think we should keep the version in the dataTable related file names.

gengliangwang · 2020-02-28T23:53:01Z

core/src/main/resources/org/apache/spark/ui/static/executorspage.js

                    "Show Additional Metrics" +
                    "</a></div>" +
-                    "<div class='container-fluid container-fluid-div' id='toggle-metrics' hidden>" +
+                    "<div class='container-fluid-div ml-4 d-none' id='toggle-metrics'>" +


why do we need ml-4 d-none

The ml-4 is needed to add the left margin on the set of checkboxes so that it is indented similarly to the original / current formatting. Without the ml-4 class the checkboxes are flush with the "Show Additional Metrics" arrow.

With regards to d-none, jQuery 1.8 deprecated .toggle() and jQuery 1.9 removed it so I had to change the JS for the "Show Additional Metrics" expand/collapse functionality. I decided to change it to the jQuery .toggleClass() method and use the Bootstrap provided d-none (display none) class to show and hide the div instead of creating a new custom class.

I see. How about putting the margin-left into the CSS of container-fluid-div so that we don't need to write 'ml-4' every time.

gengliangwang · 2020-02-29T00:02:10Z

licenses/LICENSE-bootstrap.txt

@@ -1,13 +1,22 @@
-Copyright 2013 Twitter, Inc.
+The MIT License (MIT)


We need to update LICENSE-binary as well

I just updated the license-binary files associated with bootstrap and datatables.

We should also update here:
https://github.com/apache/spark/blob/master/LICENSE-binary#L419

gengliangwang · 2020-03-02T21:14:06Z

core/src/main/resources/org/apache/spark/ui/static/executorspage.js

                $("#showAdditionalMetrics").append(
                    "<div><a id='additionalMetrics'>" +
-                    "<span class='expand-input-rate-arrow arrow-closed' id='arrowtoggle-optional-metrics'></span>" +
+                    "<span class='expand-input-rate-arrow arrow-closed' id='arrowtoggle-optional-metrics'></span> " +


Nit: this line seems only adding one space, we can avoid such changes.

Agreed this is a little lame. I'll remove the space and use styling instead.

The space was removed. It was originally added to make the arrow spacing consistent between the executor page and the other pages since the other pages rely on whitespace for the gap between the arrow and the expand / collapse text. Fixing all other occurrences would require many more code changes so I just reverted the space for now.

…les.

gengliangwang · 2020-03-02T21:23:00Z

@clarkead I found some issues in the stage page.
This is after changes:

The cursor should be a pointer and there should be a list of checkbox after click.
Before changes:

clarkead · 2020-03-02T21:32:32Z

It appears I missed updating the .toggle() calls in the stagepage.js. It also affects the "Aggregated Metrics by Executor" toggle as well. I'll resolve that now.

gengliangwang · 2020-03-02T21:51:28Z

There are also some differences in the timeline.
Before changes:

After changes:

…(css) to include the version number as requested. Also discovered and fixed a table width formatting issue on the history page.

clarkead · 2020-03-07T00:13:14Z

@gengliangwang I've added the version number to the dataTables js and css filenames as discussed. I also noticed while testing that the table on the first page of the history server was not displaying at 100% width as it currently does in master so I fixed that via a tweak to core/src/main/scala/org/apache/spark/deploy/history/HistoryPage.scala (removed an unnecessary "row" class).

…mit.

gengliangwang · 2020-03-10T00:44:12Z

Jenkins retest this please

gengliangwang · 2020-03-10T01:45:36Z

Jenkins test this please

SparkQA · 2020-03-10T03:33:02Z

Test build #119593 has finished for PR 27370 at commit ae1e0c2.

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

gengliangwang · 2020-03-10T07:38:53Z

@clarkead this PR is very close to being merged.
Could you fix the test failure?

clarkead · 2020-03-11T17:04:02Z

@gengliangwang I'll take a look at this today and try to get a fix in.

…ed for Bootstrap4

gengliangwang · 2020-03-12T02:07:26Z

add to whitelist

gengliangwang · 2020-03-12T02:07:41Z

Jenkins test this please

SparkQA · 2020-03-12T04:32:11Z

Test build #119688 has finished for PR 27370 at commit e1f0b0e.

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

gengliangwang · 2020-03-12T05:11:34Z

retest this please.

SparkQA · 2020-03-12T07:05:01Z

Test build #119696 has finished for PR 27370 at commit e1f0b0e.

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

gengliangwang · 2020-03-12T07:46:37Z

retest this please

SparkQA · 2020-03-12T10:28:24Z

Test build #119707 has finished for PR 27370 at commit e1f0b0e.

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

gengliangwang · 2020-03-12T19:21:36Z

retest this please

SparkQA · 2020-03-12T22:20:23Z

Test build #119723 has finished for PR 27370 at commit e1f0b0e.

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

gengliangwang

LGTM

gengliangwang · 2020-03-13T07:57:50Z

I will merge this one if no one has other comments this Friday.

gengliangwang · 2020-03-13T22:24:17Z

Thanks for all the work, merging to master.

clarkead · 2020-03-16T16:23:38Z

@gengliangwang and @srowen Thank you for your time and help getting this merged in. Much appreciated.

### What changes were proposed in this pull request? Spark's Web UI is using an older version of Bootstrap (v. 2.3.2) for the portal pages. Bootstrap 2.x was moved to EOL in Aug 2013 and Bootstrap 3.x was moved to EOL in July 2019 (https://github.com/twbs/release). Older versions of Bootstrap are also getting flagged in security scans for various CVEs: https://snyk.io/vuln/SNYK-JS-BOOTSTRAP-72889 https://snyk.io/vuln/SNYK-JS-BOOTSTRAP-173700 https://snyk.io/vuln/npm:bootstrap:20180529 https://snyk.io/vuln/npm:bootstrap:20160627 I haven't validated each CVE, but it would be nice to resolve any potential issues and get on a supported release. The bad news is that there have been quite a few changes between Bootstrap 2 and Bootstrap 4. I've tried updating the library, refactoring/tweaking the CSS and JS to maintain a similar appearance and functionality, and testing the UI for functionality and appearance. This is a fairly large change so I'm sure additional testing and fixes will be needed. ### How was this patch tested? This has been manually tested, but there is a ton of functionality and there are many pages and detail pages so it is very possible bugs introduced from the upgrade were missed. Additional testing and feedback is welcomed. If it appears a whole page was missed let me know and I'll take a pass at addressing that page/section. Closes apache#27370 from clarkead/bootstrap4-core-upgrade. Authored-by: Dale Clarke <[email protected]> Signed-off-by: Gengliang Wang <[email protected]>

…o longer used ### What changes were proposed in this pull request? This PR removes `bootstrap-tooltip.js` which is no longer used. That script is replaced with `bootstrap.bundle.min.js` in SPARK-30654 ( #27370 ). ### Why are the changes needed? For cleaning up repository.. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manually checked whether tooltips are shown in the UI and no error message shown in the debug console. Closes #28515 from sarutak/remove-tooltipjs. Authored-by: Kousuke Saruta <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

emmanuelrflores · 2021-06-30T21:17:46Z

Has this been put into a release? Looking at commit by @gengliangwang, it looks like it should be in 3.1.2, but when I look at the release notes for 3.1.2 on the main site or the jira site, I can't find the reference.

I'm not familiar with the release process or how the notes are detailed, so my apologies if I missed something. Thank you

srowen · 2021-06-30T21:46:18Z

The commit message above shows the commit it produces, and that it's in branch 3.1. The JIRA says 3.1.0
2a4fed0

clarkead added 3 commits January 24, 2020 15:57

Updated Bootstrap CSS/JS from 2.3.2 to Bootstrap 4.4.1

a59954e

Merge branch 'master' into bootstrap4-core-upgrade

ceef0a8

Merge branch 'master' into bootstrap4-core-upgrade

5720577

Cleaning up an issue from merging the latest changes from upstream ma…

c0b0a03

…ster to this issue branch.

clarkead added 2 commits January 29, 2020 10:31

Updating the Bootstrap license and the list of third-party license ch…

8a31e0e

…eck excluded files.

Merge branch 'master' into bootstrap4-core-upgrade

824449d

Refactored tooltips on the streaming page for Bootstrap 4 compatibility.

df9f061

Merge branch 'master' into bootstrap4-core-upgrade

0db496e

dongjoon-hyun added the DOCUMENTATION label Feb 5, 2020

Merge branch 'master' into bootstrap4-core-upgrade

c01ca8b

gengliangwang reviewed Feb 28, 2020

View reviewed changes

gengliangwang reviewed Feb 29, 2020

View reviewed changes

Reverted indent formatting updates to simplify review.

c85664c

gengliangwang reviewed Mar 2, 2020

View reviewed changes

Updating the licenses-binary LICENSE files for bootstrap4 and datatab…

876a2b2

…les.

gengliangwang changed the title ~~[SPARK-30654] Bootstrap4 WebUI upgrade~~ [SPARK-30654][WebUI] Bootstrap4 WebUI upgrade Mar 2, 2020

gengliangwang removed the DOCUMENTATION label Mar 2, 2020

Renamed dataTables.bootstrap.min.js(css) and jquery.dataTables.min.js…

5373822

…(css) to include the version number as requested. Also discovered and fixed a table width formatting issue on the history page.

clarkead added 2 commits March 6, 2020 16:24

Fixing the .rat-excludes after renaming the libraries in the last com…

c27e1c6

…mit.

Merge branch 'master' into bootstrap4-core-upgrade

ae1e0c2

Fixed the UIUtilsSuite tests for the progress bar as the markup chang…

e1f0b0e

…ed for Bootstrap4

gengliangwang approved these changes Mar 13, 2020

View reviewed changes

gengliangwang closed this in 2a4fed0 Mar 13, 2020

sarutak mentioned this pull request May 12, 2020

[SPARK-30654][WEBUI][FOLLOWUP] Remove bootstrap-tooltip.js which is no longer used. #28515

Closed

sarutak mentioned this pull request Apr 19, 2021

[SPARK-35117][UI] Change progress bar back to highlight ratio of tasks in progress #32214

Closed

		@@ -1,13 +1,22 @@
		Copyright 2013 Twitter, Inc.
		The MIT License (MIT)

[SPARK-30654][WebUI] Bootstrap4 WebUI upgrade #27370

[SPARK-30654][WebUI] Bootstrap4 WebUI upgrade #27370

Uh oh!

Conversation

clarkead commented Jan 27, 2020

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen commented Jan 28, 2020

Uh oh!

srowen commented Jan 28, 2020

Uh oh!

SparkQA commented Jan 28, 2020

Uh oh!

clarkead commented Jan 29, 2020

Uh oh!

srowen commented Jan 29, 2020

Uh oh!

clarkead commented Jan 29, 2020

Uh oh!

clarkead commented Jan 29, 2020

Uh oh!

gengliangwang commented Jan 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented Feb 28, 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!

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!

clarkead Mar 3, 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!

gengliangwang commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clarkead commented Mar 2, 2020

Uh oh!

gengliangwang commented Mar 2, 2020

Uh oh!

clarkead commented Mar 7, 2020

Uh oh!

gengliangwang commented Mar 10, 2020

Uh oh!

gengliangwang commented Mar 10, 2020

Uh oh!

SparkQA commented Mar 10, 2020

Uh oh!

gengliangwang commented Mar 10, 2020

Uh oh!

clarkead commented Mar 11, 2020

Uh oh!

gengliangwang commented Mar 12, 2020

Uh oh!

gengliangwang commented Mar 12, 2020

Uh oh!

SparkQA commented Mar 12, 2020

Uh oh!

gengliangwang commented Mar 12, 2020

Uh oh!

SparkQA commented Mar 12, 2020

Uh oh!

gengliangwang commented Mar 12, 2020

clarkead Mar 3, 2020 •

edited

Loading

gengliangwang commented Mar 2, 2020 •

edited

Loading

gengliangwang commented Mar 13, 2020 •

edited

Loading