SPARK-2533 - Add locality levels on stage summary view #9487

jbonofre · 2015-11-05T05:57:09Z

No description provided.

jbonofre · 2015-11-05T06:03:12Z

@kayousterhout suggestions:

can we change the format of this to say "Process local: 2 tasks; Rack local: 4 tasks; Any: 15 tasks"? I think that's easier to read and more clear what it is.
I wonder if we should put this under the "Show additional metrics" drop down. Right now, it's one of the first things someone will see when they open the UI, and it's not clear to me that that's the right prioritization.

I'm updating there.

jbonofre · 2015-11-05T08:19:18Z

Reference to the "old" PR: #9117

SparkQA · 2015-11-05T11:46:26Z

Test build #1985 has finished for PR 9487 at commit f7cf968.

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

srowen · 2015-11-05T11:55:38Z

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

+      val locality = taskUIData.taskInfo.taskLocality.toString
+      localityCounts.put(locality, localityCounts.getOrElse(locality, 0L) + 1)
+    })
+    return localityCounts.map { _ match {


I think you can omit return and the _ match since you have just one case?

Correct Sean. Thanks ! Actually, I'm implementing second Kay's suggestion (using "Show Additional Metrics" checkbox). I will update the PR.

jbonofre · 2015-11-05T14:18:21Z

Updated PR implementing Kay's suggestions: formatting the locality level summary string, and moving under the "Show Additional Metrics" drop down.

srowen · 2015-11-05T14:30:13Z

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

+      case (localityLevel, count) => s"$localityLevel: $count task(s)"
+    }}.mkString("; ")
+  }
+


This might result in inconsistent order of output and always shows 0 counts, which may or may not be desirable. How about this which avoids some of the repetition:

val localities = stageData.taskData.values.map(_.taskInfo.taskLocality) val localityCounts = localities.groupBy(identity).mapValues(_.size) val localityNamesAndCounts = localityCounts.toSeq.map { case (locality, count) => val localityName = locality match { case TaskLocality.PROCESS_LOCAL => "Process local" case TaskLocality.NODE_LOCAL => "Node local" case TaskLocality.RACK_LOCAL => "Rack local" case TaskLocality.ANY => "Any" } (localityName, count) } localityNamesAndCounts.sorted.mkString("; ")

Fair enough, let me try. Thanks again Sean, I appreciate all your help, support and guidance !

jbonofre · 2015-11-05T20:17:48Z

PR rebased and updated according to Sean's suggestion

srowen · 2015-11-05T20:19:23Z

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

@@ -21,6 +21,7 @@ import java.net.URLEncoder
 import java.util.Date
 import javax.servlet.http.HttpServletRequest

+import scala.collection.mutable.HashMap


Looking good. Finally, I think this import is no longer needed.

True !!! Let me fix that.

jbonofre · 2015-11-05T20:57:03Z

PR rebased and updated with alphabetize import thanks to @kayousterhout suggestion (thanks ;))

kayousterhout · 2015-11-05T21:54:20Z

Can you post the updated screenshot?

kayousterhout · 2015-11-05T21:55:42Z

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

+        case TaskLocality.RACK_LOCAL => "Rack local"
+        case TaskLocality.ANY => "Any"
+      }
+      (localityName, count)


It looks like this prints as "(Process local, 3); (Rack local, 5)"? Am I understanding that correctly? If so, can you replace line 83 with s"$localityName: $count" to get a nicer looking format?

Good point, let me do it.

jbonofre · 2015-11-06T05:48:42Z

Sure, let me prepare that.

jbonofre · 2015-11-06T06:43:12Z

jbonofre · 2015-11-06T06:44:55Z

PR rebased and implement Kay's suggestion (about the locality level summary string format)

srowen · 2015-11-06T07:10:43Z

LGTM

srowen · 2015-11-08T11:22:44Z

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala

@@ -177,6 +192,10 @@ private[ui] class StagePage(parent: StagesTab) extends WebUIPage("stage") {
          <div class="additional-metrics collapsed">
            <ul>
              <li>
+                <strong>Locality Level Summary: </strong>


Hm, actually on final thought, is this the right place to show this info? it's dumped into a list of metrics, but all the others are controlled by arrows and checkboxes.

What do you think about a new table "Locality Levels Summary" displayed by a checkbox in additional metric ?

Yea, it didn't register to me that this was adding to the list of additional metrics enabled by checkboxes. If it goes here, I think it would have to be treated the same way. If a table displays well, that's reasonable to me. It might be harder to make render though; could be overkill at this stage. @kayousterhout has better sensibilities here.

Catcha. Let me try something around this for you.

andrewor14 · 2015-11-10T18:57:06Z

Hm, I find it a little weird that we put it under additional metrics. It's not really a metric and it's different from the rest of the things under that drop down. Should we just put it under Total time across all tasks? @kayousterhout @srowen

kayousterhout · 2015-11-10T19:01:17Z

@andrewor14 I think that was how @jbonofre originally did it, but the complaint with that approach was that it clutters the UI with information that may not be useful to many people. I'm torn here though because it is pretty small; I'm fine with just putting it under the total time if @srowen is OK with that.

jbonofre · 2015-11-10T19:44:01Z

Correct Kay, it's the way that I did at the beginning.
I'm proposing either:

display under the total time as I did at the beginning
add a "Locality Levels Summary" expanded div (like "Additional Metric"), collapsed by default (I'm preparing an update on the PR about this)

Thoughts ?
Thanks !

jbonofre · 2015-11-10T20:01:32Z

I'm updating the PR with the collapsed table. I will attach a screenshot.

jbonofre · 2015-11-11T05:00:34Z

PR rebased and updated with a collapsed div. Please, see the following screenshots:

Default stage view:

When user expands the "locality level summary" div:

kayousterhout · 2015-11-11T05:20:03Z

I'd prefer your original approach to this, because in either case, it takes up one line of text in the default view, but with this, it takes up one line of text and you have to click on that line to see the metrics.

@andrewor14 @srowen what about putting the summary ("Locality summary: process local: 3 tasks; dode local: 6 tasks; ..." under the "Summary metrics" heading and above the table (and always displayed)?

@jbonofre might be a good idea to hold off on changing the code until there's consensus on the right approach

jbonofre · 2015-11-11T05:25:11Z

@kayousterhout ok, let's wait feedback from the others. I will revert my commit, back on a single string always displayed.

Thanks !

srowen · 2015-11-11T14:37:25Z

Thanks, the picture helps me think about where this should go.

This seems most related to the "Tasks" section which also reports the locality of each task. Is it too random to stick this in as a line of text before or after that table?

Summary Metrics: could go here too. This is only for completed tasks though. The metrics are for active tasks too right?

jbonofre · 2015-11-11T14:40:13Z

You are right: it's more related to tasks. And yes, it's valid for both completed and active tasks.

kayousterhout · 2015-11-11T17:33:41Z

Good point re: the summary metrics being only for completed tasks. I think it's a little weird to put the summary metric under the "tasks" table though; what about the original proposal of putting it near the top, under "total time"?

jbonofre · 2015-11-11T17:37:48Z

+1 with @kayousterhout

srowen · 2015-11-11T17:46:19Z

Ha OK, full circle there. At least we've considered every possible position on the page. I'm OK with that. Maybe later there's a big redesign that re-sorts all the info here but for now it's no bad place to stick this info, logically or aesthetically.

andrewor14 · 2015-11-11T17:49:23Z

I agree, we should just put it under total time. The KV pairs at the top of the page are for summary info aggregated across tasks and that's exactly what this is. It looks clunky as a table and doesn't seem to be of equal importance to things like the event timeline.

jbonofre · 2015-11-11T18:29:18Z

Let me revert the unecessary commits.

…ional Metrics" drop down

… Sean's suggestion

… Kay's suggestion

jbonofre · 2015-11-12T12:03:37Z

PR rebased, locality levels summary on stage just under the time (as first proposal).

srowen · 2015-11-12T16:22:04Z

OK that seems pretty good.

andrewor14 · 2015-11-12T18:10:46Z

retest this please

SparkQA · 2015-11-12T20:12:47Z

Test build #45752 has finished for PR 9487 at commit bd7de49.

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

andrewor14 · 2015-11-12T23:45:30Z

LGTM merging into master 1.6, thanks!

Author: Jean-Baptiste Onofré <jbonofre@apache.org> Closes #9487 from jbonofre/SPARK-2533-2. (cherry picked from commit 74c3004) Signed-off-by: Andrew Or <andrew@databricks.com>

jbonofre · 2015-11-13T05:31:21Z

Thanks ! Let me prepare new PR ;)

Author: Jean-Baptiste Onofré <jbonofre@apache.org> Closes apache#9487 from jbonofre/SPARK-2533-2.

srowen reviewed Nov 5, 2015
View reviewed changes

jbonofre force-pushed the SPARK-2533-2 branch from f7cf968 to 4388a89 Compare November 5, 2015 14:17

srowen reviewed Nov 5, 2015
View reviewed changes

jbonofre force-pushed the SPARK-2533-2 branch from 4388a89 to 4293666 Compare November 5, 2015 20:17

srowen reviewed Nov 5, 2015
View reviewed changes

jbonofre force-pushed the SPARK-2533-2 branch from 397de2e to 1f0f023 Compare November 5, 2015 20:56

kayousterhout reviewed Nov 5, 2015
View reviewed changes

jbonofre force-pushed the SPARK-2533-2 branch from 1f0f023 to 96a93ed Compare November 6, 2015 06:44

srowen reviewed Nov 8, 2015
View reviewed changes

jbonofre force-pushed the SPARK-2533-2 branch from 96a93ed to 1f52f87 Compare November 9, 2015 20:31

jbonofre force-pushed the SPARK-2533-2 branch from 1f52f87 to 5a489fe Compare November 11, 2015 04:59

jbonofre force-pushed the SPARK-2533-2 branch from 5a489fe to b4b8c90 Compare November 12, 2015 12:00

jbonofre added 7 commits November 12, 2015 13:01

SPARK-2533 - Add locality levels on stage summary view

f02d53c

SPARK-2533 - Update locality level format and move in the "Show Addit…

e3b1478

…ional Metrics" drop down

SPARK-2533 - Update locality level summary string format according to…

9a1e27b

… Sean's suggestion

SPARK-2533 - Remove unused import

439671d

SPARK-2533 - Alphabetize import

4d41ce9

SPARK-2533 - Update locality level summary string format according to…

e9abe3f

… Kay's suggestion

SPARK-2533 - Add locality levels summary on the stage view

bd7de49

jbonofre force-pushed the SPARK-2533-2 branch from b4b8c90 to bd7de49 Compare November 12, 2015 12:02

asfgit closed this in 74c3004 Nov 12, 2015

dskrvk pushed a commit to dskrvk/spark that referenced this pull request Nov 13, 2015

[SPARK-2533] Add locality levels on stage summary view

1c4b894

Author: Jean-Baptiste Onofré <jbonofre@apache.org> Closes apache#9487 from jbonofre/SPARK-2533-2.

SPARK-2533 - Add locality levels on stage summary view #9487

SPARK-2533 - Add locality levels on stage summary view #9487

Conversation

jbonofre commented Nov 5, 2015

jbonofre commented Nov 5, 2015

jbonofre commented Nov 5, 2015

SparkQA commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbonofre commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbonofre commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbonofre commented Nov 5, 2015

kayousterhout commented Nov 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbonofre commented Nov 6, 2015

jbonofre commented Nov 6, 2015

jbonofre commented Nov 6, 2015

srowen commented Nov 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewor14 commented Nov 10, 2015

kayousterhout commented Nov 10, 2015

jbonofre commented Nov 10, 2015

jbonofre commented Nov 10, 2015

jbonofre commented Nov 11, 2015

kayousterhout commented Nov 11, 2015

jbonofre commented Nov 11, 2015

srowen commented Nov 11, 2015

jbonofre commented Nov 11, 2015

kayousterhout commented Nov 11, 2015

jbonofre commented Nov 11, 2015

srowen commented Nov 11, 2015

andrewor14 commented Nov 11, 2015

jbonofre commented Nov 11, 2015

jbonofre commented Nov 12, 2015

srowen commented Nov 12, 2015

andrewor14 commented Nov 12, 2015

SparkQA commented Nov 12, 2015

andrewor14 commented Nov 12, 2015

jbonofre commented Nov 13, 2015