Skip to content

Conversation

@ckadner
Copy link
Member

@ckadner ckadner commented Oct 27, 2015

SPARK-11338: HistoryPage not multi-tenancy enabled ...

  • HistoryPage.scala ...prepending all page links with the web proxy (uiRoot) path
  • HistoryServerSuite.scala ...adding a test case to verify all site-relative links are prefixed when the environment variable APPLICATION_WEB_PROXY_BASE (or System property spark.ui.proxyBase) is set

@BryanCutler
Copy link
Member

I took a look, LGTM

@holdenk
Copy link
Contributor

holdenk commented Oct 28, 2015

I've taken a first look, and it looks good pending tests. Could maybe @andrewor14 or @vanzin or @tsudukim take a look (since it seems like you are some of the more recent contributors to this part of the history server) / whitelist or trigger jenkins tests

@holdenk
Copy link
Contributor

holdenk commented Oct 28, 2015

@ckadner one question is would it make sense to simply always prepend inside of makepagelink instead of doing this change everywhere? Looking at the other pages though it seems that most them do it this way so maybe there is a good reason.

@vanzin
Copy link
Contributor

vanzin commented Oct 29, 2015

+1 to @holdenk's comment. If you're changing all invocations of makePageLink it might be easier to just change makePageLink itself.

@andrewor14
Copy link
Contributor

ok to test

@SparkQA
Copy link

SparkQA commented Oct 29, 2015

Test build #44589 has finished for PR 9291 at commit 8bcb3dc.

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

@ckadner
Copy link
Member Author

ckadner commented Oct 29, 2015

@holdenk @vanzin, thank you for your comments! I just pushed an update to my fix.

@SparkQA
Copy link

SparkQA commented Oct 29, 2015

Test build #44618 has finished for PR 9291 at commit d054bd7.

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

@ckadner
Copy link
Member Author

ckadner commented Oct 31, 2015

@vanzin, @andrewor14, if you are okay with this fix, do you think it could still make it into 1.6?

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this line should be indented now.

@vanzin
Copy link
Contributor

vanzin commented Nov 1, 2015

Couple of nits, otherwise LGTM.

@ckadner
Copy link
Member Author

ckadner commented Nov 1, 2015

@vanzin -- I pushed the nit pickity fixes 🔍 -- Thanks :-)

@SparkQA
Copy link

SparkQA commented Nov 1, 2015

Test build #44754 has finished for PR 9291 at commit 01d2f35.

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

@vanzin
Copy link
Contributor

vanzin commented Nov 1, 2015

Merging to master.

@asfgit asfgit closed this in dc7e399 Nov 1, 2015
@ckadner ckadner deleted the SPARK-11338 branch November 2, 2015 03:39
@ckadner
Copy link
Member Author

ckadner commented Nov 2, 2015

@vanzin -- Thank you!

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.

6 participants