[SPARK-17672] Spark 2.0 history server web Ui takes too long for a single application by wgtmac · Pull Request #15247 · apache/spark

wgtmac · 2016-09-26T23:44:07Z

What changes were proposed in this pull request?

Added a new API getApplicationInfo(appId: String) in class ApplicationHistoryProvider and class SparkUI to get app info. In this change, FsHistoryProvider can directly fetch one app info in O(1) time complexity compared to O(n) before the change which used an Iterator.find() interface.

Both ApplicationCache and OneApplicationResource classes adopt this new api.

How was this patch tested?

manual tests

…ngle application

ajbozarth

Overall I like this addition, just a couple comments

ajbozarth · 2016-09-27T21:48:11Z

 }
+
+ def getApplicationInfo(appId: String): Option[ApplicationInfo] = {
+ throw new UnsupportedOperationException()


I think this would be better to return the expected result (the info if the id matches or a None if it doesn't) rather than throw an UnsupportedException, just for consistency

ajbozarth · 2016-09-27T21:53:58Z

 case Some(LoadedAppUI(ui, updateState)) =>
- val completed = ui.getApplicationInfoList.exists(_.attempts.last.completed)
+ val completed = ui.getApplicationInfo(appId)
+ .map(_.attempts.last.completed).getOrElse(false)


Though I get what you're trying to do here, I'm not sure this is actually faster in this case since the list is only length one. It may be cleaner to leave this as is, what are other reviewers opinions?

Makes sense. I'll roll it back.

So while reviewing #15248 I think this original change actually might be better, mostly because it removes dependance both the getApplicationInfoList method that you're working with in that pr, even if it "looks" less clean.

ajbozarth · 2016-09-27T22:10:49Z

If this passes tests, LGTM

vanzin · 2016-09-28T00:22:12Z

ok to test

SparkQA · 2016-09-28T00:29:28Z

Test build #66004 has finished for PR 15247 at commit 9feb812.

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

SparkQA · 2016-09-28T04:18:52Z

Test build #66013 has finished for PR 15247 at commit 7e437a1.

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

vanzin · 2016-09-28T18:12:15Z

 }
+
+ def getApplicationInfo(appId: String): Option[ApplicationInfo] = {
+ if (appId == this.appId) {


I'd rather this one use the iterator.find approach to avoid duplication. It's a one item list anyway.

Good point. I've made this change.

vanzin · 2016-09-28T18:12:34Z

One minor comment otherwise looks ok.

SparkQA · 2016-09-28T21:25:37Z

Test build #66050 has finished for PR 15247 at commit 67ea2de.

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

wgtmac · 2016-09-28T21:39:05Z

Anyone knows why the last test failed in the sql module? My change has nothing to do with it.

ajbozarth · 2016-09-28T21:45:15Z

Jenkins, retest this please

SparkQA · 2016-09-28T23:36:44Z

Test build #66059 has finished for PR 15247 at commit 67ea2de.

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

wgtmac · 2016-09-29T02:28:41Z

Another test failed in the mllib: org.apache.spark.mllib.classification.NaiveBayesSuite.Naive Bayes Multinomial

@ajbozarth Have you seen this kind of unrelated failure before? Do I have the permission to trigger the test? Thanks!

vanzin · 2016-09-29T16:48:29Z

retest this please

SparkQA · 2016-09-29T18:59:32Z

Test build #66110 has finished for PR 15247 at commit 67ea2de.

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

andrewor14 · 2016-09-29T19:44:26Z

+ /**
+ * Returns an ApplicationHistoryInfo for the appId.
+ *
+ * @return ApplicationHistoryInfo of one appId if exists.


This is kind of verbose. I would just say @return the [[ApplicationHistoryInfo]] for the appId if it exists

I'll fix this myself when I merge

andrewor14 · 2016-09-29T19:45:39Z

 getApplicationList().iterator.map(ApplicationsListResource.appHistoryInfoToPublicAppInfo)
 }

+ def getApplicationInfo(appId: String): Option[ApplicationInfo] = {


I believe this is a public API, but it's a reasonable one

andrewor14 · 2016-09-29T19:46:01Z

 private[spark] trait UIRoot {
 def getSparkUI(appKey: String): Option[SparkUI]
 def getApplicationInfoList: Iterator[ApplicationInfo]
+ def getApplicationInfo(appId: String): Option[ApplicationInfo]


andrewor14 · 2016-09-29T19:46:23Z

LGTM merging into master 2.0, thanks.

…ngle application Added a new API getApplicationInfo(appId: String) in class ApplicationHistoryProvider and class SparkUI to get app info. In this change, FsHistoryProvider can directly fetch one app info in O(1) time complexity compared to O(n) before the change which used an Iterator.find() interface. Both ApplicationCache and OneApplicationResource classes adopt this new api. manual tests Author: Gang Wu <wgtmac@uber.com> Closes#15247 from wgtmac/SPARK-17671. (cherry picked from commit cb87b3c) Signed-off-by: Andrew Or <andrewor14@gmail.com>

[SPARK-17672] Spark 2.0 history server web Ui takes too long for a si…
db92f16
…ngle application

ajbozarth reviewed Sep 27, 2016
View reviewed changes

changed getApplicationInfo to return expected value of SparkUI
9feb812

fixed issue in scalastyle test
7e437a1

vanzin reviewed Sep 28, 2016
View reviewed changes

use iterator.find approach in getApplicationInfo
67ea2de

wgtmac closed this Sep 28, 2016

wgtmac reopened this Sep 28, 2016

ajbozarth mentioned this pull request Sep 29, 2016
[SPARK-17671] Spark 2.0 history server summary page is slow even set spark.history.ui.maxApplications #15248
Closed

andrewor14 reviewed Sep 29, 2016
View reviewed changes

asfgit closed this in cb87b3cSep 29, 2016

Uh oh!

Conversation

wgtmac commented Sep 26, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

ajbozarth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajbozarth commented Sep 27, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanzin commented Sep 28, 2016

Uh oh!

SparkQA commented Sep 28, 2016

Uh oh!

SparkQA commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin commented Sep 28, 2016

Uh oh!

SparkQA commented Sep 28, 2016

Uh oh!

wgtmac commented Sep 28, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajbozarth commented Sep 28, 2016

Uh oh!

SparkQA commented Sep 28, 2016

Uh oh!

wgtmac commented Sep 29, 2016

Uh oh!

vanzin commented Sep 29, 2016

Uh oh!

SparkQA commented Sep 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Sep 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ajbozarth commented Sep 27, 2016•
edited
Loading

wgtmac commented Sep 28, 2016•
edited
Loading