[SPARK-7729][UI]Executor which has been killed should also be displayed on Executor Tab #10058

lianhuiwang · 2015-12-01T05:02:42Z

@andrewor14 @squito Dead Executors should also be displayed on Executor Tab.
as following:

SparkQA · 2015-12-01T05:14:01Z

Test build #46941 has finished for PR 10058 at commit bd40d49.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * class StorageStatusListener(conf: SparkConf) extends SparkListener\n

SparkQA · 2015-12-01T05:21:55Z

Test build #46942 has finished for PR 10058 at commit 97dbd62.

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

SparkQA · 2015-12-01T07:27:03Z

Test build #46943 has finished for PR 10058 at commit 16e175d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * class StorageStatusListener(conf: SparkConf) extends SparkListener\n

CodingCat · 2015-12-01T14:03:09Z

what's wrong with the font here? seems quite different with the original one?

CodingCat · 2015-12-01T14:07:19Z

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

-    val storageStatusList = listener.storageStatusList
-    (0 until storageStatusList.size).map { statusId =>
-      ExecutorsPage.getExecInfo(listener, statusId)
+    listener.synchronized {


maybe I missed something, why we need synchronized here?

more than one thread use listener object. There are rest api, executor page in UI, and SparkListener.

I'm still confused about the reason on imposing the sequential access here.....can you give an example of the problem without this synchronized....

Because ExecutorsPage.getExecInfo() use executorToTasksActive, executorToTasksFailed of ExecutorsListener and SparkListenerBus thread also change value of executorToTasksActive,executorToTasksFailed in onPostEvent() , we need to use synchronized there. It is better that synchronized is used in ExecutorsPage.getExecInfo().

I don't think it is necessary, essentially you are imposing that even the GET operations are to be processed sequentially....

lianhuiwang · 2015-12-01T15:03:56Z

@CodingCat I have addressed your comments. Thanks.

SparkQA · 2015-12-01T15:17:19Z

Test build #46964 has finished for PR 10058 at commit 7b244ff.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * class StorageStatusListener(conf: SparkConf) extends SparkListener\n

CodingCat · 2015-12-02T05:19:17Z

core/src/main/scala/org/apache/spark/ui/exec/ExecutorsPage.scala

@@ -52,68 +53,82 @@ private[ui] class ExecutorsPage(
  private val listener = parent.listener

  def render(request: HttpServletRequest): Seq[Node] = {
-    val storageStatusList = listener.storageStatusList
+    listener.synchronized {


similar situation here

lianhuiwang · 2015-12-02T07:14:15Z

@CodingCat Can you take a look again?Thanks

SparkQA · 2015-12-02T08:02:17Z

Test build #47038 has finished for PR 10058 at commit b106cfe.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * class StorageStatusListener(conf: SparkConf) extends SparkListener\n

SparkQA · 2015-12-02T11:38:32Z

Test build #47043 has finished for PR 10058 at commit 47255fa.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * class StorageStatusListener(conf: SparkConf) extends SparkListener\n

vanzin · 2015-12-03T19:29:50Z

core/src/main/scala/org/apache/spark/storage/StorageStatusListener.scala

@@ -87,8 +94,13 @@ class StorageStatusListener extends SparkListener {
  override def onBlockManagerRemoved(blockManagerRemoved: SparkListenerBlockManagerRemoved) {
    synchronized {
      val executorId = blockManagerRemoved.blockManagerId.executorId
-      executorIdToStorageStatus.remove(executorId)
+      val removedStorageStatus = executorIdToStorageStatus.remove(executorId)


nit: looks nicer if you do executorIdToStorageStatus.remove(executorId).foreach { status =>

vanzin · 2015-12-03T19:37:09Z

The code looks good, but I'd like to see at least a basic test.

lianhuiwang · 2015-12-04T14:31:05Z

@vanzin I address your comments. Can you take a look again? Thanks.

SparkQA · 2015-12-04T15:57:25Z

Test build #47198 has finished for PR 10058 at commit 325149f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * class StorageStatusListener(conf: SparkConf) extends SparkListener\n

vanzin · 2015-12-04T17:45:52Z

retest this please

vanzin · 2015-12-04T19:41:16Z

LGTM pending tests.

SparkQA · 2016-02-02T06:58:11Z

Test build #50548 has finished for PR 10058 at commit f749a5f.

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

lianhuiwang · 2016-02-02T11:33:43Z

retest this please

SparkQA · 2016-02-02T13:34:11Z

Test build #50566 has finished for PR 10058 at commit 8f0be11.

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

SparkQA · 2016-02-02T13:43:41Z

Test build #50567 has finished for PR 10058 at commit 8f0be11.

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

lianhuiwang · 2016-02-02T15:10:10Z

@zsxwing I have updated it to resolve the conflicts. and now executor ui is the following(many active executors that can not be seen in the picture are displayed on the bottom of ui ):

zsxwing · 2016-02-17T00:03:22Z

Looks great. ping @andrewor14 to take a look

andrewor14 · 2016-02-20T00:05:19Z

core/src/main/scala/org/apache/spark/ui/exec/ExecutorsTab.scala

-  def storageStatusList: Seq[StorageStatus] = storageStatusListener.storageStatusList
+  def activeStorageStatusList: Seq[StorageStatus] = storageStatusListener.activeStorageStatusList
+
+  def deadStorageStatusList: Seq[StorageStatus] = storageStatusListener.deadStorageStatusList


is this used anywhere

andrewor14 · 2016-02-20T00:10:43Z

@lianhuiwang Looks good. Can you rebase to master?

Conflicts: project/MimaExcludes.scala

SparkQA · 2016-02-21T17:16:22Z

Test build #51636 has finished for PR 10058 at commit 96950c6.

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

lianhuiwang · 2016-02-22T13:28:13Z

@andrewor14 Now all tests have been passed.

andrewor14 · 2016-02-22T19:15:45Z

Sorry this conflicts with master again. Once you rebase I'll go ahead and merge it.

Conflicts: project/MimaExcludes.scala

lianhuiwang · 2016-02-23T05:14:03Z

@andrewor14 Thanks. I have updated with master. I think you can go ahead after all tests have been passed.

Conflicts: project/MimaExcludes.scala

SparkQA · 2016-02-23T07:36:55Z

Test build #51734 has finished for PR 10058 at commit c88afa8.

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

andrewor14 · 2016-02-23T19:09:20Z

Alright, I've merged this into master after resolving the conflicts myself. It was a tiny Mima thing so it wasn't so bad. Thanks for working on this.

lianhuiwang · 2016-02-24T01:50:52Z

@andrewor14 Thanks.

…otals Table ## What changes were proposed in this pull request? Now that dead executors are shown in the executors table (#10058) the totals table is updated to include the separate totals for alive and dead executors as well as the current total, as originally discussed in #10668 ## How was this patch tested? Manually verified by running the Standalone Web UI in the latest Safari and Firefox ESR Author: Alex Bozarth <ajbozart@us.ibm.com> Closes #11381 from ajbozarth/spark13459.

…otals Table ## What changes were proposed in this pull request? Now that dead executors are shown in the executors table (apache#10058) the totals table is updated to include the separate totals for alive and dead executors as well as the current total, as originally discussed in apache#10668 ## How was this patch tested? Manually verified by running the Standalone Web UI in the latest Safari and Firefox ESR Author: Alex Bozarth <ajbozart@us.ibm.com> Closes apache#11381 from ajbozarth/spark13459.

init commit

bd40d49

lianhuiwang changed the title ~~[SPARK-7729][UI]Executor which has been killed should also be displayed on~~ [SPARK-7729][UI]Executor which has been killed should also be displayed on Executor Tab Dec 1, 2015

lianhuiwang mentioned this pull request Dec 1, 2015

SPARK-7729:Executor which has been killed should also be displayed on… #6263

Closed

rename activeStorageStatusList

97dbd62

fix scala style

16e175d

CodingCat reviewed Dec 1, 2015
View reviewed changes

address CodingCat's comments

7b244ff

fix minor error

b106cfe

CodingCat reviewed Dec 2, 2015
View reviewed changes

remove synchronized

47255fa

vanzin reviewed Dec 3, 2015
View reviewed changes

address vanzin's comments

325149f

lianhuiwang added 2 commits February 2, 2016 14:46

update with master

35eef9a

update with master

f749a5f

update with master

8f0be11

andrewor14 reviewed Feb 20, 2016
View reviewed changes

lianhuiwang added 2 commits February 21, 2016 22:43

Merge branch 'apache2' into SPARK-7729

c632d39

Conflicts: project/MimaExcludes.scala

address andrewor's comments

96950c6

Merge branch 'apache-master' into SPARK-7729

9ef6c5b

Conflicts: project/MimaExcludes.scala

Merge branch 'apache-master' into SPARK-7729

c88afa8

Conflicts: project/MimaExcludes.scala

asfgit closed this in 9f42633 Feb 23, 2016

ajbozarth mentioned this pull request Feb 26, 2016

[SPARK-13459] [Web UI] Separate Alive and Dead Executors in Executor Totals Table #11381

Closed

[SPARK-7729][UI]Executor which has been killed should also be displayed on Executor Tab #10058

[SPARK-7729][UI]Executor which has been killed should also be displayed on Executor Tab #10058

Conversation

lianhuiwang commented Dec 1, 2015

SparkQA commented Dec 1, 2015

SparkQA commented Dec 1, 2015

SparkQA commented Dec 1, 2015

CodingCat commented Dec 1, 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

Choose a reason for hiding this comment

lianhuiwang commented Dec 1, 2015

SparkQA commented Dec 1, 2015

Choose a reason for hiding this comment

lianhuiwang commented Dec 2, 2015

SparkQA commented Dec 2, 2015

SparkQA commented Dec 2, 2015

Choose a reason for hiding this comment

vanzin commented Dec 3, 2015

lianhuiwang commented Dec 4, 2015

SparkQA commented Dec 4, 2015

vanzin commented Dec 4, 2015

vanzin commented Dec 4, 2015

SparkQA commented Feb 2, 2016

lianhuiwang commented Feb 2, 2016

SparkQA commented Feb 2, 2016

SparkQA commented Feb 2, 2016

lianhuiwang commented Feb 2, 2016

zsxwing commented Feb 17, 2016

Choose a reason for hiding this comment

andrewor14 commented Feb 20, 2016

SparkQA commented Feb 21, 2016

lianhuiwang commented Feb 22, 2016

andrewor14 commented Feb 22, 2016

lianhuiwang commented Feb 23, 2016

SparkQA commented Feb 23, 2016

andrewor14 commented Feb 23, 2016

lianhuiwang commented Feb 24, 2016