[SPARK-17671] Changed implementation of HistoryServer.getApplicationInfoList for lazy evaluation #15303

wgtmac · 2016-09-29T20:56:53Z

What changes were proposed in this pull request?

Changed implementation of HistoryServer.getApplicationInfoList() to lazily evaluation the time-consuming transformation. In this way, ApplicationListResource can return an iterator without evaluating the whole input iterator.

How was this patch tested?

No API added. Just manual unit tests.

…zy evaluation

AmplabJenkins · 2016-09-29T20:57:15Z

Can one of the admins verify this patch?

wgtmac · 2016-09-29T21:03:18Z

@srowen @ajbozarth I created this PR without adding any new API. Just rewrote the way getApplicationList constructing the iterator. Can you guys take a look? Thanks!

srowen · 2016-09-30T12:00:37Z

See my comments on the previous PR. I think this is much more simply handled by returning an Iterator over apps from the beginning, calling application.values.iterator instead of application.values and returning that up the stack.

srowen · 2016-09-30T11:58:26Z

core/src/main/scala/org/apache/spark/deploy/history/HistoryServer.scala


  def getApplicationInfoList: Iterator[ApplicationInfo] = {
-    getApplicationList().iterator.map(ApplicationsListResource.appHistoryInfoToPublicAppInfo)
+    new Iterator[ApplicationInfo] {


You don't need to do this at all. Just call .iterator.map(...) but that's what it already did.

srowen · 2016-09-30T11:59:59Z

core/src/main/scala/org/apache/spark/status/api/v1/ApplicationListResource.scala

-          attempt.startTime.getTime <= maxDate.timestamp
+    val numApps = Option(limit).getOrElse(Integer.MAX_VALUE).asInstanceOf[Int]
+
+    new Iterator[ApplicationInfo] {


Likewise, this is entirely fine as-is, filtering on an iterator

wgtmac · 2016-09-30T20:06:19Z

@srowen Thank you so much for spending time on this. Now I think it is OK to leave it unchanged. I'm inclined to close this PR unless you think the minor code change makes sense.

ajbozarth · 2016-09-30T20:09:47Z

@wgtmac I think this small change is a better implementation of my previous pr and as long as @srowen agrees this LGTM. (I also learned a lot about how Iterators and Views work in scala while reviewing this)

wgtmac · 2016-09-30T20:15:40Z

@ajbozarth Yup. This is truly a good learning process to me. Really appreciate the patient help from you and @srowen

srowen

Right now this change doesn't really do anything, just moves the conditional logic about the limit around. Hm, it seemed like this was pretty fine and solvable by operating on an iterator over the applications? is it hard and/or not worth it really?

srowen · 2016-09-30T21:36:44Z

core/src/main/scala/org/apache/spark/status/api/v1/ApplicationListResource.scala

      }
    }
+
+    val numApps = Option(limit).getOrElse(Integer.MAX_VALUE).asInstanceOf[Int]


Oh I see, this is weird because limit is a java.lang.Integer. I suggest Option(limit).map(_.toInt).getOrElse(Int.MaxValue) is a tiny bit better.

Yup the iterator works fine if I limit the number of applications. I just updated this line in the PR for archive use.

Can't we just change limit to be an Int to start, it seems that would make this easier.

Yeah, JAX-RS will be OK with that, but I assume that when the parameter is missing it becomes 0 or something and that won't work nicely here.

srowen · 2016-09-30T21:37:48Z

core/src/main/scala/org/apache/spark/status/api/v1/ApplicationListResource.scala

      // keep the app if *any* attempts fall in the right time window
      val dateOk = app.attempts.exists { attempt =>
        attempt.startTime.getTime >= minDate.timestamp &&
          attempt.startTime.getTime <= maxDate.timestamp


This may be off topic but if your original concern was performance, then I'd note that there's no real need to evaluate dateOk by looking at all attempts again, if statusOk is false. Not sure it matters.

I agree. Compared to the transformation from ApplicationHistoryInfo to ApplicationInfo, this one is negligible.

srowen · 2016-09-30T22:36:55Z

@vanzin I know you raised this issue. @wgtmac what if I pursue my idea for resolving this in a separate PR, as kind of discussed in #15248 ? this change doesn't do anything really.

wgtmac · 2016-09-30T23:36:41Z

@srowen No problem.

srowen · 2016-10-02T16:13:59Z

OK, I'd suggest closing this unless anyone feels strongly about implementing this change too, but I think it doesn't have any functional impact.

ajbozarth · 2016-10-02T16:20:39Z

It's cleaner, but I have no strong opinions

srowen · 2016-10-02T16:33:53Z

OK I ported a version of all the discussed changes to the new PR. It should be cleaner still, and possibly a tiny bit faster, which seems reasonable if the point of this PR is to optimize this path

Closes apache#15303 Closes apache#15078 Closes apache#15080 Closes apache#15135 Closes apache#14565 Closes apache#12355 Closes apache#15404

Closes apache#15303 Closes apache#15078 Closes apache#15080 Closes apache#15135 Closes apache#14565 Closes apache#12355 Closes apache#15404 Author: Sean Owen <sowen@cloudera.com> Closes apache#15451 from srowen/CloseStalePRs.

changed implementation of HistoryServer.getApplicationInfoList for la…

fbf1a65

…zy evaluation

wgtmac changed the title ~~changed implementation of HistoryServer.getApplicationInfoList for lazy evaluation~~ [SPARK-17671] Changed implementation of HistoryServer.getApplicationInfoList for lazy evaluation Sep 29, 2016

srowen requested changes Sep 30, 2016

View reviewed changes

directly operator on iterator

dc8146e

srowen reviewed Sep 30, 2016

View reviewed changes

moved toInt logic in map

955154b

srowen added a commit to srowen/spark that referenced this pull request Oct 2, 2016

Port additional change from apache#15303

e385be8

srowen added a commit to srowen/spark that referenced this pull request Oct 12, 2016

Closing stale PRs.

4d40636

Closes apache#15303 Closes apache#15078 Closes apache#15080 Closes apache#15135 Closes apache#14565 Closes apache#12355 Closes apache#15404

srowen mentioned this pull request Oct 12, 2016

[BUILD] Closing stale PRs #15451

Closed

asfgit closed this in eb69335 Oct 12, 2016

[SPARK-17671] Changed implementation of HistoryServer.getApplicationInfoList for lazy evaluation #15303

[SPARK-17671] Changed implementation of HistoryServer.getApplicationInfoList for lazy evaluation #15303

Uh oh!

Conversation

wgtmac commented Sep 29, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

AmplabJenkins commented Sep 29, 2016

Uh oh!

wgtmac commented Sep 29, 2016

Uh oh!

srowen commented Sep 30, 2016

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Sep 30, 2016

Uh oh!

ajbozarth commented Sep 30, 2016

Uh oh!

wgtmac commented Sep 30, 2016

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

srowen Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

wgtmac Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

ajbozarth Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

srowen Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

srowen Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

wgtmac Sep 30, 2016

Choose a reason for hiding this comment

Uh oh!

srowen commented Sep 30, 2016

Uh oh!

wgtmac commented Sep 30, 2016

Uh oh!

srowen commented Oct 2, 2016

Uh oh!

ajbozarth commented Oct 2, 2016

Uh oh!

srowen commented Oct 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

srowen Sep 30, 2016 •

edited

Loading