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-3286] - Cannot view ApplicationMaster UI when Yarn’s url scheme is ... #2206

Closed
wants to merge 2 commits into from

Conversation

benoyantony
Copy link

...https

@vanzin
Copy link
Contributor

vanzin commented Aug 29, 2014

LGTM (although I haven't tested it).

@benoyantony benoyantony changed the title SPARK-3286 - Cannot view ApplicationMaster UI when Yarn’s url scheme is ... [SPARK-3286] - Cannot view ApplicationMaster UI when Yarn’s url scheme is ... Aug 29, 2014
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@tgravescs
Copy link
Contributor

Jenkins, test this please

@SparkQA
Copy link

SparkQA commented Aug 29, 2014

QA tests have started for PR 2206 at commit 254248a.

  • This patch merges cleanly.

@@ -174,7 +174,7 @@ private[spark] class ApplicationMaster(args: ApplicationMasterArguments,
if (sc == null) {
finish(FinalApplicationStatus.FAILED, "Timed out waiting for SparkContext.")
} else {
registerAM(sc.ui.appUIHostPort, YarnSparkHadoopUtil.getUIHistoryAddress(sc, sparkConf))
registerAM(sc.ui.appUIAddress, YarnSparkHadoopUtil.getUIHistoryAddress(sc, sparkConf))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I thought when I originally added this support that yarn didn't like having the scheme on it. Perhaps they fixed in in hadoop 2.x when they added support for https. I need to investigate that.

Copy link
Author

Choose a reason for hiding this comment

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

Please see YARN-1553.

Copy link
Contributor

Choose a reason for hiding this comment

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

But that patch is pretty recent. If adding the scheme breaks Yarn 2.3 and older, then Spark should detect which version is running before setting the address.

Copy link
Author

Choose a reason for hiding this comment

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

This check was originally introduced in YARN-1203. So from Hadoop-2.2 onwards , it uses the scheme passed by AppMaster.

Copy link
Contributor

Choose a reason for hiding this comment

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

So what @tgravescs said applies; this code needs to work with older Hadoop versions.

Copy link
Author

Choose a reason for hiding this comment

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

So for older versions , we need to pass the url as before and for newer versions, we pass the url with the scheme. What's the best way to check the Hadoop/Yarn version? Could you please point me to code snippet ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me investigate a bit more and I'll get back to you.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes so unfortunately I went and verified that hadoop 0.23 can't have the scheme. It automatically adds the scheme itself (just supports http://).

This is already calling down into stable/alpha versions of YarnRMClientImpl.register. We could potentially pass whole uri, including scheme and then have the alpha version strip off the scheme.

Also you modified the call in runDriver to registerAM which covers client mode, I assume the same change needs to happen in runExecutorLauncher for yarn client mode.

@SparkQA
Copy link

SparkQA commented Aug 29, 2014

QA tests have finished for PR 2206 at commit 254248a.

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

@tgravescs
Copy link
Contributor

It looks like you added SPARK-3287 into this PR, can you please separate that out?

@benoyantony
Copy link
Author

I have created a new Pull request : #2276. I have added the code to remove the url scheme for the alpha version.

I am not sure whether I need to fix runExecutorLauncher

@benoyantony benoyantony closed this Sep 4, 2014
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.

5 participants