Skip to content

Commit 448a8b7

Browse files
JoshRosenhvanhovell
authored andcommitted
[SC-4878] Hacky fix for log viewer's "load more" / "load previous" links
## What changes were proposed in this pull request? This patch attempts to fix the broken "load more" / "load previous" links in the Spark web UI's log viewer page. In a nutshell, the problem is that some of the links and URLs that power this page are constructed in Javascript and thus are not visible to SparkUIProxyServlet's URL-rewriting code, causing them to break. A partial fix, implemented by me in 600abffb19a2cb5feccae9d78666f958e30b7da2, is to change the URLs to use the relative path `../log/*` instead of `/log`. The reason that this work is that the log viewer Javascript is only served from the master / worker web UIs at `uiroot/logPage`, so the `../` takes us back to the UI root and the `/log` re-descends back into the endpoint for loading more logs. By using relative paths here we ensure that the browser issues the right requests from inside of the IFrame. That earlier patch of mine fixed the load more link when navigating to the log viewer page from the driver UI's executor page links, but it turns out that the load more links are still broken when navigating to that same log viewer page from the worker web UI or master web UI. It turns out that this is caused by a subtle difference in behavior due to _how_ those pages are linked to: - The driver UI generates links with a trailing slash after the `logPage` part, e.g. `logPage/?appId=%s&executorId=%s&logType=stdout`. - The master and worker UIs, however, omit the trailing slash and generate URLs like `logPage?appId=%s&executorId=%s&logType=stdout`; these URLs don't work with the `../` trick because they cause the generated URL / request to go up one level too high and miss the SparkContextID parameter, resulting in a broken URL which returns an error response and causes a Javascript error. The new fix, implemented here, is still somewhat hacky: I just updated the URL generation code to always append that trailing slash. This risks breakage in case the open-source UI changes significantly in the future, but we're already vulnerable to such breakage and the more comprehensive fix will involve either extensive upstream changes to how we generate URLs in the UI (e.g. always fetching the base URL from an attribute of a HTML element on the page and using that in the JS). (Also, before anyone suggests this, the HTML5 `<base>` tag is not a good idea here since it risks serious breakage of anchor links and third-party JS). ## How was this patch tested? Manually on Dogfood via the following steps: - From the cluster page, navigate to the driver UI: - Click executors tab, then for the first executor: - Click 'stderr', verify that log viewer works. Whether this works or not can be verified by clicking on "load more": it should either display more output or should put up blue informational text saying that no more output is available. - Go back, click 'stdout', test that page. - From the cluster page, click on Spark master UI link. - Click on application details page for the only running application. - Test stdout and stderr pages for the first executor. - From the cluster page, click on Spark worker UI link: - Test sdtout and stderr pages for the first executor. Author: Josh Rosen <joshrosendatabricks.com> (cherry picked from commit dfd994f7121517c50d5926ccd0715843b0d548b7) Signed-off-by: Herman van Hovell <hvanhovelldatabricks.com> ## Conflicts: core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala Author: Josh Rosen <[email protected]> Closes apache#122 from hvanhovell/SC-4878.
1 parent 4718bb4 commit 448a8b7

File tree

4 files changed

+17
-9
lines changed

4 files changed

+17
-9
lines changed

core/src/main/resources/org/apache/spark/ui/static/log-view.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ function loadMore() {
5757

5858
$.ajax({
5959
type: "GET",
60-
url: "/log" + baseParams + "&offset=" + offset + "&byteLength=" + moreByteLength,
60+
url: "../log" + baseParams + "&offset=" + offset + "&byteLength=" + moreByteLength,
6161
success: function (data) {
6262
var oldHeight = $(".log-content")[0].scrollHeight;
6363
var newlineIndex = data.indexOf('\n');
@@ -83,14 +83,14 @@ function loadMore() {
8383
function loadNew() {
8484
$.ajax({
8585
type: "GET",
86-
url: "/log" + baseParams + "&byteLength=0",
86+
url: "../log" + baseParams + "&byteLength=0",
8787
success: function (data) {
8888
var dataInfo = data.substring(0, data.indexOf('\n')).match(/\d+/g);
8989
var newDataLen = dataInfo[2] - totalLogLength;
9090
if (newDataLen != 0) {
9191
$.ajax({
9292
type: "GET",
93-
url: "/log" + baseParams + "&byteLength=" + newDataLen,
93+
url: "../log" + baseParams + "&byteLength=" + newDataLen,
9494
success: function (data) {
9595
var newlineIndex = data.indexOf('\n');
9696
var dataInfo = data.substring(0, newlineIndex).match(/\d+/g);

core/src/main/scala/org/apache/spark/deploy/master/ui/ApplicationPage.scala

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,11 @@ private[ui] class ApplicationPage(parent: MasterWebUI) extends WebUIPage("app")
124124
<td>{executor.memory}</td>
125125
<td>{executor.state}</td>
126126
<td>
127-
<a href={"%s/logPage?appId=%s&executorId=%s&logType=stdout"
127+
<!-- Note: It's very important that the slash after logPage is kept here.
128+
Omitting it will break the load more JS on the log viewer page. -->
129+
<a href={"%s/logPage/?appId=%s&executorId=%s&logType=stdout"
128130
.format(workerUrlRef, executor.application.id, executor.id)}>stdout</a>
129-
<a href={"%s/logPage?appId=%s&executorId=%s&logType=stderr"
131+
<a href={"%s/logPage/?appId=%s&executorId=%s&logType=stderr"
130132
.format(workerUrlRef, executor.application.id, executor.id)}>stderr</a>
131133
</td>
132134
</tr>

core/src/main/scala/org/apache/spark/deploy/worker/ExecutorRunner.scala

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,8 @@ private[deploy] class ExecutorRunner(
155155
builder.environment.put("SPARK_LAUNCH_WITH_SCALA", "0")
156156

157157
// Add webUI log urls
158+
// Note: It's very important that the slash after logPage is kept here.
159+
// Omitting it will break the load more JS on the log viewer page.
158160
val baseUrl =
159161
if (conf.getBoolean("spark.ui.reverseProxy", false)) {
160162
s"/proxy/$workerId/logPage/?appId=$appId&executorId=$execId&logType="

core/src/main/scala/org/apache/spark/deploy/worker/ui/WorkerPage.scala

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,11 @@ private[ui] class WorkerPage(parent: WorkerWebUI) extends WebUIPage("") {
117117
</ul>
118118
</td>
119119
<td>
120-
<a href={"logPage?appId=%s&executorId=%s&logType=stdout"
120+
<!-- Note: It's very important that the slash after logPage is kept here.
121+
Omitting it will break the load more JS on the log viewer page. -->
122+
<a href={"logPage/?appId=%s&executorId=%s&logType=stdout"
121123
.format(executor.appId, executor.execId)}>stdout</a>
122-
<a href={"logPage?appId=%s&executorId=%s&logType=stderr"
124+
<a href={"logPage/?appId=%s&executorId=%s&logType=stderr"
123125
.format(executor.appId, executor.execId)}>stderr</a>
124126
</td>
125127
</tr>
@@ -138,8 +140,10 @@ private[ui] class WorkerPage(parent: WorkerWebUI) extends WebUIPage("") {
138140
{Utils.megabytesToString(driver.driverDesc.mem)}
139141
</td>
140142
<td>
141-
<a href={s"logPage?driverId=${driver.driverId}&logType=stdout"}>stdout</a>
142-
<a href={s"logPage?driverId=${driver.driverId}&logType=stderr"}>stderr</a>
143+
<!-- Note: It's very important that the slash after logPage is kept here.
144+
Omitting it will break the load more JS on the log viewer page. -->
145+
<a href={s"logPage/?driverId=${driver.driverId}&logType=stdout"}>stdout</a>
146+
<a href={s"logPage/?driverId=${driver.driverId}&logType=stderr"}>stderr</a>
143147
</td>
144148
<td>
145149
{driver.finalException.getOrElse("")}

0 commit comments

Comments
 (0)