[SPARK-24851][UI] Map a Stage ID to it's Associated Job ID #21809

pgandhi999 · 2018-07-18T18:34:26Z

It would be nice to have a field in Stage Page UI which would show mapping of the current stage id to the job id's to which that stage belongs to.

What changes were proposed in this pull request?

Added a field in Stage UI to display the corresponding job id for that particular stage.

How was this patch tested?

Added a field in Stage UI to display the corresponding job id for that particular stage.

tgravescs · 2018-07-18T19:56:09Z

ok to test

SparkQA · 2018-07-19T00:36:00Z

Test build #93247 has finished for PR 21809 at commit 7be0520.

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

jiangxb1987 · 2018-07-20T04:37:45Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

      }.toSeq
  }

+  def getJobIdsAssociatedWithStage(stageId: Int): Seq[Set[Int]] = {


We don't need to fetch all the stage attempts, just the first of it is enough to get all the jobIds.

Good point! Have taken care of it.

gengliangwang · 2018-07-20T16:45:03Z

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

+          {if (!stageJobIds.isEmpty) {
+            <li>
+              <strong>Associated Job Ids: </strong>
+              {stageJobIds}


make it href link?

The problem here is that the stage could also have multiple job ids, in that case, we get a bunch of them. Do you want a generic link instead that will take you to the jobs page? Let me know what you think.

My suggestion is to map each job id as a href link.

Did that and updated the screenshot. Thank you.

gengliangwang · 2018-07-20T16:52:35Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

  }

+  def getJobIdsAssociatedWithStage(stageId: Int): Seq[Set[Int]] = {
+    store.view(classOf[StageDataWrapper]).index("stageId").first(stageId).last(stageId)


can we avoid the store look up here?

I really did not get the question, perhaps you are suggesting to try an alternative way but is there any other alternative way to do this? Let me know your thoughts. Thank you.

…h the job ids for the particular stage

SparkQA · 2018-07-23T23:32:24Z

Test build #93458 has finished for PR 21809 at commit 3151a62.

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

gengliangwang · 2018-07-24T06:14:08Z

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

        return UIUtils.headerSparkPage(request, stageHeader, content, parent)
      }

+    val stageJobIds = parent.store.getJobIdsAssociatedWithStage(stageId, stageAttemptId)


In https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala#L109 there is a query for the stage data already. We can reduce the query to the store here.

E.g. we can add a function to return the whole StageDataWrapper in AppStatusStore

Makes sense, I have fixed it. Request you to have a look.

gengliangwang · 2018-07-24T06:37:01Z

@pgandhi999 could you update the title to [SPARK-24851][UI] Map a Stage ID to it's Associated Job ID

SparkQA · 2018-07-25T18:39:45Z

Test build #93561 has finished for PR 21809 at commit a50e8b1.

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

SparkQA · 2018-07-25T22:37:55Z

Test build #93563 has finished for PR 21809 at commit d57e6dc.

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

tgravescs · 2018-07-26T16:39:25Z

test this please

SparkQA · 2018-07-26T20:15:29Z

Test build #93607 has finished for PR 21809 at commit d57e6dc.

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

…Exception on querying an invalid stage id

SparkQA · 2018-07-27T02:09:48Z

Test build #93631 has finished for PR 21809 at commit 3a06b87.

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

gengliangwang · 2018-07-27T12:07:50Z

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

+        .asOption(stageDataWrapper.info)
+        .get
+    } else {
+      stageData = {


this code branch is unreachable.

It is unreachable by the IDE but during runtime, the code does work, I have confirmed that.

gengliangwang · 2018-07-27T12:18:13Z

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

+          {if (!stageJobIds.isEmpty) {
+            <li>
+              <strong>Associated Job Ids: </strong>
+              {for (jobId <- stageJobIds) yield {val detailUrl = "%s/jobs/job/?id=%s".format(


Using map is more readable.

gengliangwang · 2018-07-27T12:19:59Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

  }

-  def stageAttempt(stageId: Int, stageAttemptId: Int, details: Boolean = false): v1.StageData = {
+  def stageAttempt(stageId: Int, stageAttemptId: Int,


Changing the return type to (StageData, jobIds) might be simpler.

Using map instead of for..yield and returning tuple instead of object

SparkQA · 2018-07-28T00:08:59Z

Test build #93689 has finished for PR 21809 at commit 52b08b2.

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

tgravescs · 2018-08-02T18:13:05Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

    val stageKey = Array(stageId, stageAttemptId)
-    val stage = store.read(classOf[StageDataWrapper], stageKey).info
-    if (details) stageWithDetails(stage) else stage
+    val stageDataWrapper: StageDataWrapper = store.read(classOf[StageDataWrapper], stageKey)


you don't really need the type here (: StageDataWrapper)

Done, thanks!

tgravescs · 2018-08-02T18:13:07Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala


-  def stageAttempt(stageId: Int, stageAttemptId: Int, details: Boolean = false): v1.StageData = {
+  def stageAttempt(stageId: Int, stageAttemptId: Int,
+    details: Boolean = false): (v1.StageData, Seq[Int]) = {


indent , you might make it look like taskSummary

Done, thanks!

tgravescs · 2018-08-02T18:25:19Z

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

-      .asOption(parent.store.stageAttempt(stageId, stageAttemptId, details = false))
-      .getOrElse {
+    var stageDataTuple: Tuple2[StageData, Seq[Int]] = null
+    try {


I think you could use options here instead of try and null checks to make this cleaner

Done, thanks!

SparkQA · 2018-08-09T19:45:39Z

Test build #94500 has finished for PR 21809 at commit 9eff537.

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

abellina

@pgandhi999 had a comment, otherwise LGTM.

abellina · 2018-09-12T16:39:44Z

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

              {Utils.bytesToString(stageData.diskBytesSpilled)}
            </li>
          }}
+          {if (!stageJobIds.isEmpty) {


This could throw an NPE if stageDataTuple is None

scala> var x:Seq[Int] = null x: Seq[Int] = null scala> x.isEmpty java.lang.NullPointerException

Since, we are returning from the code if it is None, it should never reach there as far as I can tell.

tgravescs · 2018-09-18T19:48:24Z

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

-          </div>
-        return UIUtils.headerSparkPage(request, stageHeader, content, parent)
-      }
+    var stageDataTuple: Option[Tuple2[StageData, Seq[Int]]] = try {


can't we just simplify this to be close to what is was before but return the tuple:

val (stageData, stageJodIds) = parent.store .asOption(parent.store.stageAttempt(stageId, stageAttemptId, details = false)) .getOrElse { val content = <div id="no-info"> <p>No information to display for Stage {stageId} (Attempt {stageAttemptId})</p> </div> return UIUtils.headerSparkPage(stageHeader, content, parent) }

Makes sense, simplified the code and tested it. Looks good. Thank you.

Simplifying code block in StagePage

SparkQA · 2018-09-28T19:46:05Z

Test build #96762 has finished for PR 21809 at commit 3b73840.

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

tgravescs · 2018-10-04T17:45:48Z

core/src/main/scala/org/apache/spark/status/AppStatusStore.scala

-    if (details) stageWithDetails(stage) else stage
+    val stageDataWrapper = store.read(classOf[StageDataWrapper], stageKey)
+    val stage = if (details) stageWithDetails(stageDataWrapper.info) else stageDataWrapper.info
+    val jobIds = stageDataWrapper.jobIds


instead of having separate val just put this in the return:

(stage, stageDataWrapper.jobIds.toSeq)

SparkQA · 2018-10-06T00:05:45Z

Test build #97008 has finished for PR 21809 at commit 0be099a.

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

pgandhi999 · 2018-10-08T16:02:34Z

test this please

tgravescs · 2018-10-08T16:17:48Z

add to whitelist

SparkQA · 2018-10-08T20:06:54Z

Test build #97120 has finished for PR 21809 at commit 0be099a.

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

tgravescs · 2018-10-08T20:25:56Z

test this please

SparkQA · 2018-10-09T00:40:00Z

Test build #97130 has finished for PR 21809 at commit 0be099a.

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

tgravescs · 2018-10-09T13:58:45Z

+1

tgravescs · 2018-10-09T13:59:39Z

merged to master, thanks @pgandhi999

It would be nice to have a field in Stage Page UI which would show mapping of the current stage id to the job id's to which that stage belongs to. ## What changes were proposed in this pull request? Added a field in Stage UI to display the corresponding job id for that particular stage. ## How was this patch tested? <img width="448" alt="screen shot 2018-07-25 at 1 33 07 pm" src="https://user-images.githubusercontent.com/22228190/43220447-a8e94f80-900f-11e8-8a20-a235bbd5a369.png"> Closes apache#21809 from pgandhi999/SPARK-24851. Authored-by: pgandhi <[email protected]> Signed-off-by: Thomas Graves <[email protected]>

[SPARK-24851] : Map a Stage ID to it's Associated Job ID in UI

7be0520

Added a field in Stage UI to display the corresponding job id for that particular stage.

jiangxb1987 reviewed Jul 20, 2018

View reviewed changes

gengliangwang reviewed Jul 20, 2018

View reviewed changes

[SPARK-24851] : Using both stage id as well as the attempt id to fetc…

3151a62

…h the job ids for the particular stage

gengliangwang reviewed Jul 24, 2018

View reviewed changes

pgandhi999 changed the title ~~[SPARK-24851] : Map a Stage ID to it's Associated Job ID in UI~~ [SPARK-24851][UI] Map a Stage ID to it's Associated Job ID Jul 24, 2018

[SPARK-24851] : Avoiding extra store lookup and adding href links

a50e8b1

[SPARK-24851] : Fixing Scalastyle Tests

d57e6dc

[SPARK-24851] : Adding exception handling part to avoid NoSuchElement…

3a06b87

…Exception on querying an invalid stage id

gengliangwang reviewed Jul 27, 2018

View reviewed changes

[SPARK-24851] : Addressing reviews July 27

52b08b2

Using map instead of for..yield and returning tuple instead of object

tgravescs requested changes Aug 2, 2018

View reviewed changes

[SPARK-24851] : Addressing Reviews August 9

9eff537

abellina suggested changes Sep 12, 2018

View reviewed changes

tgravescs reviewed Sep 18, 2018

View reviewed changes

[SPARK-24851] : Addressing reviews September 28, 2018

3b73840

Simplifying code block in StagePage

tgravescs reviewed Oct 4, 2018

View reviewed changes

[SPARK-24851] : Addressing Reviews October 5, 2018

0be099a

asfgit closed this in deb9588 Oct 9, 2018

[SPARK-24851][UI] Map a Stage ID to it's Associated Job ID #21809

[SPARK-24851][UI] Map a Stage ID to it's Associated Job ID #21809

Uh oh!

Conversation

pgandhi999 commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

tgravescs commented Jul 18, 2018

Uh oh!

SparkQA commented Jul 19, 2018

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!

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!

SparkQA commented Jul 23, 2018

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!

gengliangwang commented Jul 24, 2018

Uh oh!

SparkQA commented Jul 25, 2018

Uh oh!

SparkQA commented Jul 25, 2018

Uh oh!

tgravescs commented Jul 26, 2018

Uh oh!

SparkQA commented Jul 26, 2018

Uh oh!

SparkQA commented Jul 27, 2018

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 28, 2018

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 9, 2018

Uh oh!

abellina left a comment

Choose a reason for hiding this comment

Uh oh!

pgandhi999 commented Jul 18, 2018 •

edited

Loading

pgandhi999 Sep 28, 2018 •

edited

Loading

tgravescs Sep 18, 2018 •

edited

Loading