Skip to content

Conversation

@chie8842
Copy link
Contributor

spark history server log needs to be fixed to show https url when ssl is enabled

def webUrl: String = s"http://$publicHostName:$boundPort"
def webUrl: String = {
var protocol = "http"
if(conf.get("spark.ssl.enabled") == "true") {
Copy link
Member

Choose a reason for hiding this comment

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

Its cleaner to use a getOrElse("false") here instead of an == true
Also scalastyle will fail without a space after the if

/** Return the url of web interface. Only valid after bind(). */
def webUrl: String = s"http://$publicHostName:$boundPort"
def webUrl: String = {
var protocol = "http"
Copy link
Member

Choose a reason for hiding this comment

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

val protocol = if (conf.getBoolean("spark.ssl.enabled", false)) "https" else "http"

There is actually spark.ssl.ui.enabled too, to specifically control the UI. I suppose really you need to check whether sslOptions.enabled is true.

@chie8842 chie8842 closed this Oct 25, 2016
@chie8842
Copy link
Contributor Author

Thank you for your comments. I’ll fix my program

@sarutak
Copy link
Member

sarutak commented Oct 25, 2016

Hey @hayashidac , you did't need to close this PR. You should have just pushed your change...

@chie8842 chie8842 reopened this Oct 25, 2016
@chie8842
Copy link
Contributor Author

I applied comments. Please check again.

def webUrl: String = s"http://$publicHostName:$boundPort"
def webUrl: String = {
val protocol = if (sslOptions.enabled) "https" else "http"
s"$protocol://$publicHostName:$boundPort"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: don't indent this line further

@chie8842
Copy link
Contributor Author

sorry, I fixed it.

@sarutak
Copy link
Member

sarutak commented Oct 25, 2016

Jenkins, test this please.

@sarutak
Copy link
Member

sarutak commented Oct 25, 2016

I noticed that all of spark.ui.ssl in SSLOptionsSuite.scala should be spark.ssl.ui. It's very minor so you can fix it too within this PR if you would like to.

@srowen
Copy link
Member

srowen commented Oct 25, 2016

Looking good, thank you. I also wouldn't mind fixing the "spark.ui.ssl" -> "spark.ssl.ui" issue here because it's a little bit related.

@chie8842
Copy link
Contributor Author

I fixed SSLOptionsSuite.scala too.

@SparkQA
Copy link

SparkQA commented Oct 25, 2016

Test build #67505 has finished for PR 15611 at commit 6f26e30.

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

@sarutak
Copy link
Member

sarutak commented Oct 25, 2016

ok to test.

@SparkQA
Copy link

SparkQA commented Oct 25, 2016

Test build #67513 has finished for PR 15611 at commit af73518.

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

@sarutak
Copy link
Member

sarutak commented Oct 25, 2016

LGTM. Merging into master and branch-2.0. Thanks @hayashidac .

asfgit pushed a commit that referenced this pull request Oct 25, 2016
… to show https url when ssl is enabled

spark history server log needs to be fixed to show https url when ssl is enabled

Author: chie8842 <[email protected]>

Closes #15611 from hayashidac/SPARK-16988.

(cherry picked from commit c329a56)
Signed-off-by: Kousuke Saruta <[email protected]>
@asfgit asfgit closed this in c329a56 Oct 25, 2016
robert3005 pushed a commit to palantir/spark that referenced this pull request Nov 1, 2016
… to show https url when ssl is enabled

spark history server log needs to be fixed to show https url when ssl is enabled

Author: chie8842 <[email protected]>

Closes apache#15611 from hayashidac/SPARK-16988.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
… to show https url when ssl is enabled

spark history server log needs to be fixed to show https url when ssl is enabled

Author: chie8842 <[email protected]>

Closes apache#15611 from hayashidac/SPARK-16988.
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