-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-28004][UI] Update jquery to 3.4.1 #24843
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.
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
Conversation
|
Test build #106394 has finished for PR 24843 at commit
|
| return obj != null && typeof obj === 'object' && (propName in obj); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are all from the source jquery.mustache.js, not custom modifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also checked this. This file is identical with mustache.js v3.0.1. Only the trailing spaces are removed in a few lines.
| // Scroll now too in case we had opened the page on a hash, but wait a bit because some browsers | ||
| // will try to do *their* initial scroll after running the onReady handler. | ||
| $(window).load(function() { setTimeout(function() { maybeScrollToHash(); }, 25); }); | ||
| $(window).on('load', function() { setTimeout(function() { maybeScrollToHash(); }, 25); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually the only change I seemed to need to make to the Spark javascript.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. This is the correct update based on the upgrade-guide.
|
Ah right. The history server. There's a problem there. I'll look into it. |
|
Test build #106400 has finished for PR 24843 at commit
|
|
Test build #106441 has finished for PR 24843 at commit
|
|
Test build #106443 has finished for PR 24843 at commit
|
|
Test build #106465 has finished for PR 24843 at commit
|
|
Hm, for anyone following along, these tests pass locally. Maybe I'm doing something wrong running locally. Looks like an httpclient version mismatch that I'll have to run down: |
|
Test build #106471 has finished for PR 24843 at commit
|
|
|
||
| var historySummary = $("#history-summary"); | ||
| var searchString = historySummary["context"]["location"]["search"]; | ||
| var searchString = window.location.search; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also verified that this works in the same way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Thank you, @srowen !
This should be tested in the browser's private mode to avoid old javascript cache.
I verified this PR with the followings.
Spark shell application UIThrift JDBC/ODBC Server application UISpark History Server UIon my several Spark history logs.
All tabs look working correctly. I might miss a few tiny UI features like hover info, but the navigation and the info looks correct in general. I hope we can merge this and proceed.
Merged to master.
|
cc @gatorsmile and @gengliangwang |
## What changes were proposed in this pull request? We're using an old-ish jQuery, 1.12.4, and should probably update for Spark 3 to keep up in general, but also to keep up with CVEs. In fact, we know of at least one resolved in only 3.4.0+ (https://nvd.nist.gov/vuln/detail/CVE-2019-11358). They may not affect Spark, but, if the update isn't painful, maybe worthwhile in order to make future 3.x updates easier. jQuery 1 -> 2 doesn't sound like a breaking change, as 2.0 is supposed to maintain compatibility with 1.9+ (https://blog.jquery.com/2013/04/18/jquery-2-0-released/) 2 -> 3 has breaking changes: https://jquery.com/upgrade-guide/3.0/. It's hard to evaluate each one, but the most likely area for problems is in ajax(). However, our usage of jQuery (and plugins) is pretty simple. Update jquery to 3.4.1; update jquery blockUI and mustache to latest ## How was this patch tested? Manual testing of docs build (except R docs), worker/master UI, spark application UI. Note: this really doesn't guarantee it works, as our tests can't test javascript, and this is merely anecdotal testing, although I clicked about every link I could find. There's a risk this breaks a minor part of the UI; it does seem to work fine in the main. Closes apache#24843 from srowen/SPARK-28004. Authored-by: Sean Owen <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
| implicit val webDriver: WebDriver = new HtmlUnitDriver(true) { | ||
| getWebClient.getOptions.setThrowExceptionOnScriptError(false) | ||
| } | ||
| implicit val webDriver: WebDriver = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last clarifying comment: This change slightly improves the test to not ignore javascript errors, and as part of that, needed to test as a modern browser (IE 11 vs IE 8).
|
A late LGTM. I also tried |
|
any chance this will be back ported to 2.3? 2.3 is currently quarantined at my site because of this. |
|
No, 2.3 is EOL. You should update to 2.4.x |
We're using an old-ish jQuery, 1.12.4, and should probably update for Spark 3 to keep up in general, but also to keep up with CVEs. In fact, we know of at least one resolved in only 3.4.0+ (https://nvd.nist.gov/vuln/detail/CVE-2019-11358). They may not affect Spark, but, if the update isn't painful, maybe worthwhile in order to make future 3.x updates easier. jQuery 1 -> 2 doesn't sound like a breaking change, as 2.0 is supposed to maintain compatibility with 1.9+ (https://blog.jquery.com/2013/04/18/jquery-2-0-released/) 2 -> 3 has breaking changes: https://jquery.com/upgrade-guide/3.0/. It's hard to evaluate each one, but the most likely area for problems is in ajax(). However, our usage of jQuery (and plugins) is pretty simple. Update jquery to 3.4.1; update jquery blockUI and mustache to latest Manual testing of docs build (except R docs), worker/master UI, spark application UI. Note: this really doesn't guarantee it works, as our tests can't test javascript, and this is merely anecdotal testing, although I clicked about every link I could find. There's a risk this breaks a minor part of the UI; it does seem to work fine in the main. Closes apache#24843 from srowen/SPARK-28004. Authored-by: Sean Owen <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
What changes were proposed in this pull request?
We're using an old-ish jQuery, 1.12.4, and should probably update for Spark 3 to keep up in general, but also to keep up with CVEs. In fact, we know of at least one resolved in only 3.4.0+ (https://nvd.nist.gov/vuln/detail/CVE-2019-11358). They may not affect Spark, but, if the update isn't painful, maybe worthwhile in order to make future 3.x updates easier.
jQuery 1 -> 2 doesn't sound like a breaking change, as 2.0 is supposed to maintain compatibility with 1.9+ (https://blog.jquery.com/2013/04/18/jquery-2-0-released/)
2 -> 3 has breaking changes: https://jquery.com/upgrade-guide/3.0/. It's hard to evaluate each one, but the most likely area for problems is in ajax(). However, our usage of jQuery (and plugins) is pretty simple.
Update jquery to 3.4.1; update jquery blockUI and mustache to latest
How was this patch tested?
Manual testing of docs build (except R docs), worker/master UI, spark application UI.
Note: this really doesn't guarantee it works, as our tests can't test javascript, and this is merely anecdotal testing, although I clicked about every link I could find. There's a risk this breaks a minor part of the UI; it does seem to work fine in the main.