Skip to content
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

[SPARK-16808][Core] prepend base URI for links on main history server #15739

Closed
wants to merge 1 commit into from
Closed

Conversation

jantes
Copy link

@jantes jantes commented Nov 2, 2016

What changes were proposed in this pull request?

Make the 2.0.x Spark UI links work on systems with proxy servers by prepending the spark.ui.proxyBase or APPLICATION_WEB_PROXY_BASE to links on the first page. This is a regression from 1.6.x (although that was fixed at the caller).

Fixing it in the getAttemptURI places the entire URI generation in one place like done for other URI's in history making future regressions less likely.

How was this patch tested?

Tested manually and dev/run-tests

@andrewor14

the contribution is original work and I license the work to the project under the project's open source license

@@ -296,7 +296,7 @@ object HistoryServer extends Logging {

private[history] def getAttemptURI(appId: String, attemptId: Option[String]): String = {
val attemptSuffix = attemptId.map { id => s"/$id" }.getOrElse("")
s"${HistoryServer.UI_PATH_PREFIX}/${appId}${attemptSuffix}"
UIUtils.prependBaseUri(s"${HistoryServer.UI_PATH_PREFIX}/${appId}${attemptSuffix}")
Copy link
Contributor

@vijoshi vijoshi Nov 2, 2016

Choose a reason for hiding this comment

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

I believe the application links rendered through the historypage-template.html would not pick up the attempt URI from this API, so those would remain broken. Also, looks like this would cause jetty to now register the UI handler for this app on the /<proxy-base>/<HistoryServer.UI_PATH_PREFIX>/ context path, which may not be what we want ?

@andrewor14
Copy link
Contributor

ok to test @vanzin

@andrewor14
Copy link
Contributor

Also there's another patch trying to solve the same issue: #15742

@SparkQA
Copy link

SparkQA commented Nov 3, 2016

Test build #68079 has finished for PR 15739 at commit 164a082.

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

@jantes
Copy link
Author

jantes commented Nov 4, 2016

The other patch includes a testcase. Let's go with that one.

@jantes jantes closed this Nov 4, 2016
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