Skip to content

Conversation

@vundela
Copy link

@vundela vundela commented Nov 3, 2015

Use the proxyBase set by the AM, if not found then use env. This is to fix the issue if somebody accidentally set APPLICATION_WEB_PROXY_BASE to wrong proxyBase

@vanzin
Copy link
Contributor

vanzin commented Nov 3, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Nov 4, 2015

Test build #44966 has finished for PR 9448 at commit 1b272a4.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use the opportunity to clean this up a bit:

sys.props("spark.ui.proxyBase").orElse(sys.env("APPLICATION_WEB_PROXY_BASE")).getOrElse("")

@vanzin
Copy link
Contributor

vanzin commented Nov 4, 2015

LGTM. My only concern is that someone might be using yarn-client mode with a different UI proxy than the RM; this change makes that impossible, since the AM always sets spark.ui.proxyBase. I find that case rather unlikely though.

@vundela
Copy link
Author

vundela commented Nov 4, 2015

Thanks for the review @vanzin, made the changes as you suggested.

@vanzin
Copy link
Contributor

vanzin commented Nov 4, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Nov 4, 2015

Test build #44998 has finished for PR 9448 at commit b2a6838.

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

Copy link
Member

Choose a reason for hiding this comment

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

I think this ultimately makes sense, letting Spark-specific stuff take precedence, though it's worth enumerating here or in the JIRA why this is connected to Oozie / uber mode, and why this is right-er than the current behavior.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review @srowen. Updated the JIRA to reflect the customer reported issue and why it is not related to Oozie/uber mode.

@vanzin
Copy link
Contributor

vanzin commented Nov 5, 2015

Merging to master.

@asfgit asfgit closed this in c76865c Nov 5, 2015
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