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.

Sign up for GitHub

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

Jump to bottom

[SPARK-18204][WEBUI] Remove SparkUI.appUIAddress #15603

Closed
wants to merge 4 commits into from
Closed

[SPARK-18204][WEBUI] Remove SparkUI.appUIAddress #15603

wants to merge 4 commits into from

Conversation

jaceklaskowski
Copy link
Contributor

@jaceklaskowski jaceklaskowski commented Oct 23, 2016 •
edited

What changes were proposed in this pull request?

Removing appUIAddress attribute since it is no longer in use.

How was this patch tested?

Local build

@SparkQA
Copy link

SparkQA commented Oct 23, 2016

Test build #67420 has finished for PR 15603 at commit 6fcd247.

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

@jaceklaskowski
Copy link
Contributor Author

The build has just proved that my thinking was correct. I think I'll propose few other (more agressive) changes to clean the code up a little bit more. I'd appreciate any comments (or acceptance).

srowen
srowen requested changes Oct 24, 2016
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Some of this, I wouldn't change randomly for its own sake. Removing the bit of dead code is reasonable if you take it a step further.

core/src/main/scala/org/apache/spark/ui/SparkUI.scala
}

/**
* Return the application UI host:port. This does not include the scheme (http://).
*/
private[spark] def appUIHostPort = publicHostName + ":" + boundPort

private[spark] def appUIAddress = s"http://$appUIHostPort"
private[spark] def appUIAddress = webUrl
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 these two methods can just be eliminated, because appUIAddress does seem to return exactly the same thing, and appUIHostPort is only referenced from tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Sean for confirming my thoughts! That's exactly the feedback I expected to get from you (or other Spark committers).

core/src/main/scala/org/apache/spark/ui/SparkUI.scala
@@ -135,7 +133,7 @@ private[spark] class SparkUI private (
private[spark] abstract class SparkUITab(parent: SparkUI, prefix: String)
extends WebUITab(parent, prefix) {

def appName: String = parent.getAppName
def appName: String = parent.appName
Copy link
Member

Choose a reason for hiding this comment

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

This can just be removed then, because this becomes a no-op. And then getAppName in the superclass can be removed. It's a pointless getter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! That's how I imagined the conversation would go. Thanks Sean!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might have missed why we could remove it. Mind refreshing my memory @srowen (sorry getting too old :))

Copy link
Member

Choose a reason for hiding this comment

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

Actually, leave this one. It does serve a purpose.

@srowen srowen mentioned this pull request Oct 24, 2016
Closed
srowen
srowen requested changes Oct 28, 2016
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Otherwise looks good yes.

core/src/main/scala/org/apache/spark/ui/SparkUI.scala
@@ -135,7 +133,7 @@ private[spark] class SparkUI private (
private[spark] abstract class SparkUITab(parent: SparkUI, prefix: String)
extends WebUITab(parent, prefix) {

def appName: String = parent.getAppName
def appName: String = parent.appName
Copy link
Member

Choose a reason for hiding this comment

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

Actually, leave this one. It does serve a purpose.

core/src/test/scala/org/apache/spark/ui/UISuite.scala
@@ -179,16 +179,15 @@ class UISuite extends SparkFunSuite {
test("verify appUIAddress contains the scheme") {
Copy link
Member

Choose a reason for hiding this comment

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

You might also change occurrences of "appUIAddress" in places like this test name. Have a look with a search and replace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Sean! How do you manage to find so much time to help people like me?!?! You're certainly not a robot -- I do know it since I met you few times and can confirm ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about renaming spark.driver.appUIAddress to spark.driver.webUrl? Not necessarily important, but merely for the sake of consistency...

@SparkQA
Copy link

SparkQA commented Oct 28, 2016

Test build #67705 has finished for PR 15603 at commit 8976018.

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

@jaceklaskowski jaceklaskowski changed the title [WEBUI][MINOR] Return types in methods + cleanup [WEBUI][MINOR] Remove unused appUIAddress in SparkUI + code cleanup Oct 29, 2016
@SparkQA
Copy link

SparkQA commented Oct 29, 2016

Test build #67754 has finished for PR 15603 at commit 7d65d49.

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

srowen
srowen reviewed Oct 29, 2016
View reviewed changes
core/src/test/scala/org/apache/spark/ui/UISuite.scala
val boundPort = ui.boundPort
assert(splitUIAddress(2).toInt == boundPort)
assert(splitUIAddress(3).toInt == boundPort)
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 actually should not change. The test fails and the behavior should be identical anyway.

@SparkQA
Copy link

SparkQA commented Oct 29, 2016

Test build #67757 has finished for PR 15603 at commit 29620cd.

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

@jaceklaskowski
Copy link
Contributor Author

The test failure doesn't seem to be due to my change.

@SparkQA
Copy link

SparkQA commented Oct 30, 2016

Test build #3379 has finished for PR 15603 at commit 29620cd.

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

@jaceklaskowski
Copy link
Contributor Author

Anything else you'd change/remove/add, Mr @srowen?

@srowen
Copy link
Member

srowen commented Oct 31, 2016

Since this turned into a bit more of a change than where it started, and because it removes a method (internal, defunct as it may be), and since this isn't a hurry, would you open a JIRA just to track it for the record?

@jaceklaskowski jaceklaskowski changed the title [WEBUI][MINOR] Remove unused appUIAddress in SparkUI + code cleanup [SPARK-18204][WEBUI] Remove SparkUI.appUIAddress Nov 1, 2016
@asfgit asfgit closed this in 70a5db7 Nov 2, 2016
@jaceklaskowski jaceklaskowski deleted the sparkui-fixes branch November 3, 2016 18:25
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
@jaceklaskowski @uzadude
[SPARK-18204][WEBUI] Remove SparkUI.appUIAddress
6acf195 
## What changes were proposed in this pull request?

Removing `appUIAddress` attribute since it is no longer in use.
## How was this patch tested?

Local build

Author: Jacek Laskowski <jacek@japila.pl>

Closes apache#15603 from jaceklaskowski/sparkui-fixes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srowen srowen srowen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jaceklaskowski @SparkQA @srowen