Skip to content

[SPARK-20042] [Web UI] Fix log page buttons for reverse proxy mode#17370

Closed
okoethibm wants to merge 2 commits intoapache:masterfrom
okoethibm:master
Closed

[SPARK-20042] [Web UI] Fix log page buttons for reverse proxy mode#17370
okoethibm wants to merge 2 commits intoapache:masterfrom
okoethibm:master

Conversation

@okoethibm
Copy link
Copy Markdown
Contributor

@okoethibm okoethibm commented Mar 21, 2017

with spark.ui.reverseProxy=true, full path URLs like /log will point to
the master web endpoint which is serving the worker UI as reverse proxy.
To access a REST endpoint in the worker in reverse proxy mode , the
leading /proxy/"target"/ part of the base URI must be retained.

Added logic to log-view.js to handle this, similar to executorspage.js

Patch was tested manually

with spark.ui.reverseProxy=true, full path URLs like /log will point to
the master web endpoint which is serving the worker UI as reverse proxy.
To access a REST endpoint in the worker in reverse proxy mode , the
leading /proxy/<target>/ part of the base URI must be retained.

Added logic to log-view.js to handle this, similar to executorspage.js
@srowen
Copy link
Copy Markdown
Member

srowen commented Mar 21, 2017

CC @ajbozarth

Copy link
Copy Markdown
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

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

I'm not a big fan of how difficult to read/understand getRESTEndPoint() is, but given it matches similar functionality elsewhere I say this LGTM.

@ajbozarth
Copy link
Copy Markdown
Member

@srowen can you ok to test this?

@ajbozarth
Copy link
Copy Markdown
Member

@srowen or @vanzin mind okaying this to test (and taking a quick look)?

@srowen
Copy link
Copy Markdown
Member

srowen commented Apr 4, 2017

Hm, is this the only link that needs rewriting in the presence of a proxy? it looks kind of brittle, like it depends on a quite specific URL structure. Can this be generalized? And/or can the logic be explained?

@okoethibm
Copy link
Copy Markdown
Contributor Author

okoethibm commented Apr 4, 2017

This is the only link generated from JavaScript that doesn't work at the moment. As noted above, the same kind of rewrite already takes place in executorspage.js (committed by @tgravescs, function createRESTEndPoint), that's where I copied the logic, with the dependency on /proxy/ inside the URL.
Of course it would be nicer to have this generalized somewhere -- this PR is intended as a low-risk localized bugfix, not a refactoring.

@srowen
Copy link
Copy Markdown
Member

srowen commented Apr 4, 2017

I see, so it's the same approach already used everywhere else and it's not easy to centralize. OK.

@okoethibm
Copy link
Copy Markdown
Contributor Author

@srowen Can you please "ok to test" then?

@SparkQA
Copy link
Copy Markdown

SparkQA commented Apr 4, 2017

Test build #3636 has finished for PR 17370 at commit 216e8fc.

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

asfgit pushed a commit that referenced this pull request Apr 5, 2017
with spark.ui.reverseProxy=true, full path URLs like /log will point to
the master web endpoint which is serving the worker UI as reverse proxy.
To access a REST endpoint in the worker in reverse proxy mode , the
leading /proxy/"target"/ part of the base URI must be retained.

Added logic to log-view.js to handle this, similar to executorspage.js

Patch was tested manually

Author: Oliver Köth <okoeth@de.ibm.com>

Closes #17370 from okoethibm/master.

(cherry picked from commit 6f09dc7)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@srowen
Copy link
Copy Markdown
Member

srowen commented Apr 5, 2017

Merged to master/2.1

@asfgit asfgit closed this in 6f09dc7 Apr 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants