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-10531] [CORE] AppId is set as AppName in status rest api #8688

Closed
wants to merge 8 commits into from
Closed

[SPARK-10531] [CORE] AppId is set as AppName in status rest api #8688

wants to merge 8 commits into from

Conversation

zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Sep 10, 2015

Verify it manually.

@andrewor14
Copy link
Contributor

add to whitelist

@andrewor14
Copy link
Contributor

@vanzin @srowen can you have a look?

@SparkQA
Copy link

SparkQA commented Sep 10, 2015

Test build #42281 has finished for PR 8688 at commit 4f81d1f.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

vanzin
vanzin reviewed Sep 10, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/SparkContext.scala
@@ -455,7 +455,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
_ui =
if (conf.getBoolean("spark.ui.enabled", true)) {
Some(SparkUI.createLiveUI(this, _conf, listenerBus, _jobProgressListener,
_env.securityManager, appName, startTime = startTime))
_env.securityManager, applicationId, appName, startTime = startTime))
Copy link
Contributor

Choose a reason for hiding this comment

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

applicationId hasn't been initialized yet here, so you shouldn't need to provide it in the constructor. Just call setAppId later as you're already doing.

@zjffdu
Copy link
Contributor Author

zjffdu commented Sep 11, 2015

Thanks for the review @vanzin
For SparkUI, yes it is not necessary to put appId in constructor. But for history server, the appId is known when creating SparkUI. Although in history server the appId of SparkUI is never used for now. But for safety, I put appId in constructor in case appId will be used in the future.

@SparkQA
Copy link

SparkQA commented Sep 11, 2015

Test build #42313 has finished for PR 8688 at commit 98dc5ea.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 11, 2015

Test build #42314 has finished for PR 8688 at commit 9593f52.

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

@SparkQA
Copy link

SparkQA commented Sep 11, 2015

Test build #42327 has finished for PR 8688 at commit d25a6f7.

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

@vanzin
Copy link
Contributor

vanzin commented Sep 11, 2015

@zjffdu there's no point in adding more code if a single line change fixes the issue. Then both live UIs and history server UIs are always created the same way. Consistency = good.

@zjffdu
Copy link
Contributor Author

zjffdu commented Sep 14, 2015

Push another commit to revert the unecessary change. And I think the appName is not set correctly in FsHistoryProvider.scala & Master.scala (Fix it in this commit). @vanzin Please help review.

@SparkQA
Copy link

SparkQA commented Sep 14, 2015

Test build #42410 has finished for PR 8688 at commit ef4abaa.

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

@vanzin
Copy link
Contributor

vanzin commented Sep 14, 2015

LGTM. At first the changes to the app name seem weird, since it was nice to have the extra information show up in the browser's title bar when looking at the history server or the master UI, but I see that it would make the information exposed in the API wrong.

retest this please

@vanzin
Copy link
Contributor

vanzin commented Sep 14, 2015

retest this please

vanzin
vanzin reviewed Sep 14, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala
@@ -146,7 +146,7 @@ private[history] class FsHistoryProvider(conf: SparkConf, clock: Clock)
val ui = {
val conf = this.conf.clone()
val appSecManager = new SecurityManager(conf)
SparkUI.createHistoryUI(conf, replayBus, appSecManager, appId,
SparkUI.createHistoryUI(conf, replayBus, appSecManager, "",
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of "" this should be appInfo.name

Copy link
Contributor

Choose a reason for hiding this comment

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

(and then the call to ui.setAppName later is not needed.)

Copy link
Contributor

Choose a reason for hiding this comment

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

(and then maybe SparkUI.setAppName may not be needed at all!)

@SparkQA
Copy link

SparkQA commented Sep 14, 2015

Test build #42434 has finished for PR 8688 at commit ef4abaa.

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

@vanzin
Copy link
Contributor

vanzin commented Sep 16, 2015

@zjffdu could you take a look at whether SparkUI.setAppName is really needed at all?

@SparkQA
Copy link

SparkQA commented Sep 17, 2015

Test build #42583 has finished for PR 8688 at commit 6b51692.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zjffdu
Copy link
Contributor Author

zjffdu commented Sep 17, 2015

Push another commit to remove SparkUI#setAppName, it is not used anywhere. I think the original purpose for setAppName is a workaround for including extra info on the html title. If it is for this purpose, we should expose another api rather than using appName. Another change in the patch is rename appInfo to appAttemptInfo. (There's another appInfo outside, two same appInfo may cause potential bugs)

@SparkQA
Copy link

SparkQA commented Sep 17, 2015

Test build #42584 has finished for PR 8688 at commit c52e35d.

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

@vanzin
Copy link
Contributor

vanzin commented Sep 17, 2015

LGTM. Merging to master.

@asfgit asfgit closed this in 36d8b27 Sep 17, 2015
@viirya viirya mentioned this pull request Sep 18, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@zjffdu @andrewor14 @SparkQA @vanzin