-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
Conversation
LGTM (although I haven't tested it). |
Can one of the admins verify this patch? |
Jenkins, test this please |
QA tests have started for PR 2206 at commit
|
@@ -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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see YARN-1553.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
QA tests have finished for PR 2206 at commit
|
…cationMaster webUI is not displayed
It looks like you added SPARK-3287 into this PR, can you please separate that out? |
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 |
...https