[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-77648366
  
Spark core's `UISuite` has an ignored test for "attaching a new tab": 
https://github.com/apache/spark/blob/dba0b2eadb441f41ded0f0b9706b720bcfa84881/core/src/test/scala/org/apache/spark/ui/UISuite.scala#L75

In order to test the tab attach / detach functionality, I think we should 
re-enable and extend this test to check that we're able to remove the tab that 
we just attached.  It looks like this test may have been disabled due to port 
contention issues (#466), but I think those issues should be resolved now.  If 
it will be easier, you might rewriting this test using the new 
[UISeleniumSuite](https://github.com/apache/spark/blob/dba0b2eadb441f41ded0f0b9706b720bcfa84881/core/src/test/scala/org/apache/spark/ui/UISeleniumSuite.scala)
 framework, which will let you use Selenium CSS selectors, etc. in your test.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[tdas]

Github user tdas commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-77646184
  
BTW, please update the title and JIRA as we are not updating existing tab 
any more. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[tdas]

Github user tdas commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-77641090
  
Can you add unit tests for detaching pages and tabs. @JoshRosen will be 
able to help you out. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[tdas]

Github user tdas commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25979865
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingTab.scala ---
@@ -38,10 +38,13 @@ private[spark] class StreamingTab(ssc: StreamingContext)
   parent.attachTab(this)
 }
 
-private object StreamingTab {
+private[spark] object StreamingTab {
   def getSparkUI(ssc: StreamingContext): SparkUI = {
 ssc.sc.ui.getOrElse {
   throw new SparkException("Parent SparkUI to attach this tab to not 
found!")
 }
   }
+  def detachStreamingTab(ssc: StreamingContext, tab: SparkUITab) {
--- End diff --

Need extra line. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[tdas]

Github user tdas commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25979785
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/StreamingContext.scala ---
@@ -553,6 +553,7 @@ class StreamingContext private[streaming] (
*/
   def stop(stopSparkContext: Boolean = true): Unit = synchronized {
 stop(stopSparkContext, false)
+this.uiTab.foreach(StreamingTab.detachStreamingTab(this, _))
--- End diff --

Why is it hear and not in the other `stop` The tab should be removed when 
the stremaing context is stopped, so both `stop` should detach the tab.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[tdas]

Github user tdas commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25979694
  
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -60,14 +62,31 @@ private[spark] abstract class WebUI(
 tab.pages.foreach(attachPage)
 tabs += tab
   }
+  
+  def detachTab(tab: WebUITab) {
+tab.pages.foreach(detachPage)
+tabs -= tab
+  }
+  
+  def detachPage(page: WebUIPage) {
+page2Handlers.remove(page).foreach(_.foreach(detachHandler))
+  }
+  
 
   /** Attach a page to this UI. */
   def attachPage(page: WebUIPage) {
--- End diff --

@JoshRosen Can you take a look at this code. This is your domain. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[tdas]

Github user tdas commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25979619
  
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -45,6 +46,7 @@ private[spark] abstract class WebUI(
 
   protected val tabs = ArrayBuffer[WebUITab]()
   protected val handlers = ArrayBuffer[ServletContextHandler]()
+  protected val page2Handlers = new HashMap[WebUIPage, 
ArrayBuffer[ServletContextHandler]]
--- End diff --

We dont use the idiom "to --> 2" in our code. `pageToHandlers` is better.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[zhichao-li]

Github user zhichao-li commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25930020
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/StreamingContext.scala ---
@@ -553,6 +553,10 @@ class StreamingContext private[streaming] (
*/
   def stop(stopSparkContext: Boolean = true): Unit = synchronized {
 stop(stopSparkContext, false)
+this.uiTab match  {
--- End diff --

Thanks @jerryshao . Done with the changes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25926833
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/StreamingContext.scala ---
@@ -553,6 +553,10 @@ class StreamingContext private[streaming] (
*/
   def stop(stopSparkContext: Boolean = true): Unit = synchronized {
 stop(stopSparkContext, false)
+this.uiTab match  {
--- End diff --

The same as above.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25926814
  
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -60,14 +61,34 @@ private[spark] abstract class WebUI(
 tab.pages.foreach(attachPage)
 tabs += tab
   }
+  
+  def detachTab(tab: WebUITab) {
+tab.pages.foreach(detachPage)
+tabs -= tab
+  }
+  
+  def detachPage(page: WebUIPage) {
+page2Handlers.remove(page) match {
--- End diff --

This can be simplified as `page2Handles.remove(page).foreach(...)`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[jerryshao]

Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25926778
  
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -17,17 +17,17 @@
 
 package org.apache.spark.ui
 
+import scala.collection.mutable.HashMap
--- End diff --

Change the import ordering.

```
Java import

Scala import 

Third party import 

Spark import 
```



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[zhichao-li]

Github user zhichao-li commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-76890176
  
@tdas  Just updated the code with the solution you 
mentioned(attach-on-start-and-remove-on-stop) by adding the detachTab and 
detachPage function. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[tdas]

Github user tdas commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-76853339
  
Then we should go for it!


On Mon, Mar 2, 2015 at 2:28 PM, Josh Rosen  wrote:

> Poorly-written Selenium tests can be flaky if they don't account for
> things like asynchrony (e.g. when testing Javascript interactions), but I
> don't think that will be a problem here. We now have a bunch of Selenium
> tests for the Spark core UI and I don't think I've ever seen them fail in
> Jenkins:
> 
https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/ui/UISeleniumSuite.scala
>
> —
> Reply to this email directly or view it on GitHub
> .
>



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-76840648
  
Poorly-written Selenium tests can be flaky if they don't account for things 
like asynchrony (e.g. when testing Javascript interactions), but I don't think 
that will be a problem here.  We now have a bunch of Selenium tests for the 
Spark core UI and I don't think I've ever seen them fail in Jenkins: 
https://github.com/apache/spark/blob/master/core/src/test/scala/org/apache/spark/ui/UISeleniumSuite.scala


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[tdas]

Github user tdas commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-76836876
  
Will that be stable? Not flaky? In the past we had simple web ui tests that
uses Scala's Source class to fetch a URL to see whether a tab has been
loaded or unloaded. Those were disabled because of flakiness. I wonder
whether selenium tests will be more stable.

On Mon, Mar 2, 2015 at 1:55 PM, Josh Rosen  wrote:

> @tdas  do you think we should add a Selenium
> test for this?
>
> —
> Reply to this email directly or view it on GitHub
> .
>



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[JoshRosen]

Github user JoshRosen commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-76834346
  
@tdas do you think we should add a Selenium test for this?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[tdas]

Github user tdas commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-76829945
  
Hey this is a decent fix, but I think this is not the right one. With this 
fix, the new ssc will be reflected in the new streaming tab, however it will be 
still visible even after the earlier ssc has been stopped. The right solution 
is that the tab should be removed when a stream ing context is stopped. Since 
only one streaming context can be active on the same spark context at the same 
time, attach-on-start-and-remove-on-stop will fix the multiple tab problem in 
the right way. 

Does that make sense?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[srowen]

Github user srowen commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-76673885
  
This looks OK to me. I'd like to give @tdas at least a day to look at it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[zhichao-li]

Github user zhichao-li commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25583379
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingTab.scala ---
@@ -29,19 +29,29 @@ import StreamingTab._
  */
 private[spark] class StreamingTab(ssc: StreamingContext)
   extends SparkUITab(getSparkUI(ssc), "streaming") with Logging {
-
   val parent = getSparkUI(ssc)
-  val listener = ssc.progressListener
-
+  var listener = ssc.progressListener
   ssc.addStreamingListener(listener)
   attachPage(new StreamingPage(this))
   parent.attachTab(this)
 }
 
-private object StreamingTab {
+private[spark] object StreamingTab {
   def getSparkUI(ssc: StreamingContext): SparkUI = {
 ssc.sc.ui.getOrElse {
   throw new SparkException("Parent SparkUI to attach this tab to not 
found!")
 }
   }
+  
+  def updateOrCreateStreamingTab(ssc: StreamingContext): 
Option[StreamingTab] = {
--- End diff --

Updated accordingly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[zhichao-li]

Github user zhichao-li commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25583362
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingPage.scala ---
@@ -20,20 +20,26 @@ package org.apache.spark.streaming.ui
 import java.util.Calendar
 import javax.servlet.http.HttpServletRequest
 
-import scala.xml.Node
-
 import org.apache.spark.Logging
-import org.apache.spark.ui._
 import org.apache.spark.ui.UIUtils._
+import org.apache.spark.ui._
 import org.apache.spark.util.Distribution
 
+import scala.xml.Node
+
 /** Page for Spark Web UI that shows statistics of a streaming job */
 private[ui] class StreamingPage(parent: StreamingTab)
   extends WebUIPage("") with Logging {
 
-  private val listener = parent.listener
-  private val startTime = Calendar.getInstance().getTime()
+  private var listener = parent.listener
+  private var startTime = Calendar.getInstance().getTime()
   private val emptyCell = "-"
+  
+  def updateListener(listener : StreamingJobProgressListener):
+  Unit ={
--- End diff --

Done.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25582261
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingTab.scala ---
@@ -29,19 +29,29 @@ import StreamingTab._
  */
 private[spark] class StreamingTab(ssc: StreamingContext)
   extends SparkUITab(getSparkUI(ssc), "streaming") with Logging {
-
   val parent = getSparkUI(ssc)
-  val listener = ssc.progressListener
-
+  var listener = ssc.progressListener
   ssc.addStreamingListener(listener)
   attachPage(new StreamingPage(this))
   parent.attachTab(this)
 }
 
-private object StreamingTab {
+private[spark] object StreamingTab {
   def getSparkUI(ssc: StreamingContext): SparkUI = {
 ssc.sc.ui.getOrElse {
   throw new SparkException("Parent SparkUI to attach this tab to not 
found!")
 }
   }
+  
+  def updateOrCreateStreamingTab(ssc: StreamingContext): 
Option[StreamingTab] = {
--- End diff --

This method does not need to return `Option`. It always returns 
`StreamingTab`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25582237
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingPage.scala ---
@@ -20,20 +20,26 @@ package org.apache.spark.streaming.ui
 import java.util.Calendar
 import javax.servlet.http.HttpServletRequest
 
-import scala.xml.Node
-
 import org.apache.spark.Logging
-import org.apache.spark.ui._
 import org.apache.spark.ui.UIUtils._
+import org.apache.spark.ui._
 import org.apache.spark.util.Distribution
 
+import scala.xml.Node
+
 /** Page for Spark Web UI that shows statistics of a streaming job */
 private[ui] class StreamingPage(parent: StreamingTab)
   extends WebUIPage("") with Logging {
 
-  private val listener = parent.listener
-  private val startTime = Calendar.getInstance().getTime()
+  private var listener = parent.listener
+  private var startTime = Calendar.getInstance().getTime()
   private val emptyCell = "-"
+  
+  def updateListener(listener : StreamingJobProgressListener):
+  Unit ={
--- End diff --

The wrapping is wrong here; `Unit` should be on the previous line and no 
space before the brace. No space before `:`. Also, `System.currentTimeMillis()` 
is much more conventional for getting the time.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[zhichao-li]

Github user zhichao-li commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25578794
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingTab.scala ---
@@ -29,19 +29,30 @@ import StreamingTab._
  */
 private[spark] class StreamingTab(ssc: StreamingContext)
   extends SparkUITab(getSparkUI(ssc), "streaming") with Logging {
-
   val parent = getSparkUI(ssc)
-  val listener = ssc.progressListener
-
+  var listener = ssc.progressListener
   ssc.addStreamingListener(listener)
   attachPage(new StreamingPage(this))
   parent.attachTab(this)
 }
 
-private object StreamingTab {
+private[spark] object StreamingTab {
   def getSparkUI(ssc: StreamingContext): SparkUI = {
 ssc.sc.ui.getOrElse {
   throw new SparkException("Parent SparkUI to attach this tab to not 
found!")
 }
   }
+  def 
updateOrCreateStreamingTab(ssc:StreamingContext):Option[StreamingTab]={
--- End diff --

@srowen , Thanks for the advice. I've just tested and refactored the code 
accordingly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[srowen]

Github user srowen commented on a diff in the pull request:

https://github.com/apache/spark/pull/4828#discussion_r25558151
  
--- Diff: 
streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingTab.scala ---
@@ -29,19 +29,30 @@ import StreamingTab._
  */
 private[spark] class StreamingTab(ssc: StreamingContext)
   extends SparkUITab(getSparkUI(ssc), "streaming") with Logging {
-
   val parent = getSparkUI(ssc)
-  val listener = ssc.progressListener
-
+  var listener = ssc.progressListener
   ssc.addStreamingListener(listener)
   attachPage(new StreamingPage(this))
   parent.attachTab(this)
 }
 
-private object StreamingTab {
+private[spark] object StreamingTab {
   def getSparkUI(ssc: StreamingContext): SparkUI = {
 ssc.sc.ui.getOrElse {
   throw new SparkException("Parent SparkUI to attach this tab to not 
found!")
 }
   }
+  def 
updateOrCreateStreamingTab(ssc:StreamingContext):Option[StreamingTab]={
--- End diff --

You have some style issues here, like spaces around operators, braces, and 
parentheses. Have a look at other code. Also we usually write e.g. `foreach { 
tab =>` not `foreach(tab = {`
I think the code here can be more concise and avoid `return` with a 
construction built around 
`getTabs.find(_.isInstanceOf[StreamingTab]).map(...).getOrElse(...)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/4828#issuecomment-76514483
  
Can one of the admins verify this patch?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-6077] update listener for the existing ...

[zhichao-li]

GitHub user zhichao-li opened a pull request:

https://github.com/apache/spark/pull/4828

[SPARK-6077] update listener for the existing streamingTab instead of 
adding an one new one

Currently we would create a new streaming tab for each streamingContext 
even if there's already one on the same sparkContext which would cause 
duplicate StreamingTab created and none of them is taking effect.
snapshot: 
https://www.dropbox.com/s/t4gd6hqyqo0nivz/bad%20multiple%20streamings.png?dl=0
How to reproduce:
1)
import org.apache.spark.SparkConf
import org.apache.spark.streaming.
{Seconds, StreamingContext}
import org.apache.spark.storage.StorageLevel
val ssc = new StreamingContext(sc, Seconds(1))
val lines = ssc.socketTextStream("localhost", , 
StorageLevel.MEMORY_AND_DISK_SER)
val words = lines.flatMap(_.split(" "))
val wordCounts = words.map(x => (x, 1)).reduceByKey(_ + _)
wordCounts.print()
ssc.start()
.
2)
ssc.stop(false)
val ssc = new StreamingContext(sc, Seconds(1))
val lines = ssc.socketTextStream("localhost", , 
StorageLevel.MEMORY_AND_DISK_SER)
val words = lines.flatMap(_.split(" "))
val wordCounts = words.map(x => (x, 1)).reduceByKey(_ + _)
wordCounts.print()
ssc.start()

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/zhichao-li/spark master

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/4828.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #4828


commit a711920518fe6c0c569d490a2c6ee33cdb1f14c0
Author: lisurprise 
Date:   2015-02-28T06:34:31Z

update listener for the existing streamingTab instead of adding one new tab




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org