[SPARK-6077] Remove streaming tab while stopping StreamingContext #4828

zhichao-li · 2015-02-28T06:53:23Z

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", 9999, 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", 9999, StorageLevel.MEMORY_AND_DISK_SER)
val words = lines.flatMap(.split(" "))
val wordCounts = words.map(x => (x, 1)).reduceByKey(_ + _)
wordCounts.print()
ssc.start()

AmplabJenkins · 2015-02-28T06:57:09Z

Can one of the admins verify this patch?

srowen · 2015-02-28T15:07:18Z

streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingTab.scala

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(...)

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

srowen · 2015-03-02T07:52:27Z

streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingPage.scala

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.

srowen · 2015-03-02T08:27:31Z

This looks OK to me. I'd like to give @tdas at least a day to look at it.

tdas · 2015-03-02T21:32:55Z

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?

JoshRosen · 2015-03-02T21:54:35Z

@tdas do you think we should add a Selenium test for this?

tdas · 2015-03-02T22:07:21Z

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 [email protected] wrote:

@tdas https://github.com/tdas do you think we should add a Selenium
test for this?

—
Reply to this email directly or view it on GitHub
#4828 (comment).

JoshRosen · 2015-03-02T22:28:15Z

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

tdas · 2015-03-02T23:45:45Z

Then we should go for it!

On Mon, Mar 2, 2015 at 2:28 PM, Josh Rosen [email protected] 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
#4828 (comment).

zhichao-li · 2015-03-03T05:38:30Z

@tdas Just updated the code with the solution you mentioned(attach-on-start-and-remove-on-stop) by adding the detachTab and detachPage function.

jerryshao · 2015-03-06T05:38:21Z

core/src/main/scala/org/apache/spark/ui/WebUI.scala

Change the import ordering.

Java import Scala import Third party import Spark import

tdas · 2015-03-06T21:27:52Z

core/src/main/scala/org/apache/spark/ui/WebUI.scala

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

tdas · 2015-03-06T21:40:34Z

Can you add unit tests for detaching pages and tabs. @JoshRosen will be able to help you out.

tdas · 2015-03-06T22:14:48Z

BTW, please update the title and JIRA as we are not updating existing tab any more.

JoshRosen · 2015-03-06T22:30:13Z

Spark core's UISuite has an ignored test for "attaching a new tab":

spark/core/src/test/scala/org/apache/spark/ui/UISuite.scala

Line 75 in dba0b2e

ignore("attaching a new tab") {

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 framework, which will let you use Selenium CSS selectors, etc. in your test.

tdas · 2015-03-12T06:00:57Z

ok to test

SparkQA · 2015-03-12T06:03:12Z

Test build #28500 has started for PR 4828 at commit 51e6c7f.

This patch merges cleanly.

tdas · 2015-03-12T06:11:46Z

LGTM, assuming the tests pass.

tdas · 2015-03-12T06:16:38Z

Oh wait. I just realized that there is a already a unit test in org.apace.spark.streaming.UISuite which tested whether the streaming tab works. But thats ignored too. Could you update that testsuite to use selenium and test whether the streaming tab is attached on and detached correctly. I just realized that the unit tests in the PR tests detaching for tabs, but does not test whether it works specifically for streaming (suppose the line to detach gets deleted accidentally in future, no unit test would fail). So please add the streaming's specific unit test - rename spark.streaming.UISuite to UISeleniumSuite, and then update test to use selenium. Other than that, existing changes look good.

SparkQA · 2015-03-12T07:24:17Z

Test build #28500 has finished for PR 4828 at commit 51e6c7f.

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

AmplabJenkins · 2015-03-12T07:24:22Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28500/
Test PASSed.

SparkQA · 2015-03-12T08:32:47Z

Test build #28502 has started for PR 4828 at commit 4e9e4fc.

This patch merges cleanly.

SparkQA · 2015-03-12T08:37:53Z

Test build #28503 has started for PR 4828 at commit 59180a7.

This patch merges cleanly.

zhichao-li · 2015-03-12T08:38:23Z

@tdas UISeleniumSuite has been added for streaming and the LocalStreamingContext utilities is borrowed from LocalSparkContext .

SparkQA · 2015-03-12T08:42:36Z

Test build #28504 has started for PR 4828 at commit bff699b.

This patch merges cleanly.

SparkQA · 2015-03-12T09:45:42Z

Test build #28502 has finished for PR 4828 at commit 4e9e4fc.

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

AmplabJenkins · 2015-03-12T09:45:46Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28502/
Test PASSed.

SparkQA · 2015-03-12T09:52:14Z

Test build #28503 has finished for PR 4828 at commit 59180a7.

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

AmplabJenkins · 2015-03-12T09:52:17Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28503/
Test PASSed.

SparkQA · 2015-03-12T10:00:37Z

Test build #28504 has finished for PR 4828 at commit bff699b.

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

AmplabJenkins · 2015-03-12T10:00:41Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28504/
Test PASSed.

tdas · 2015-03-13T08:16:58Z

streaming/src/test/scala/org/apache/spark/streaming/LocalStreamingContext.scala

This is already defined in streaming.TestSuiteBase. Rather than creating this new file, please use that.

SparkQA · 2015-03-13T08:28:04Z

Test build #28556 has started for PR 4828 at commit c329806.

This patch merges cleanly.

SparkQA · 2015-03-13T09:50:08Z

Test build #28556 has finished for PR 4828 at commit c329806.

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

AmplabJenkins · 2015-03-13T09:50:12Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28556/
Test PASSed.

tdas · 2015-03-13T19:18:31Z

LGTM! Will merge asap.

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", 9999, 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", 9999, StorageLevel.MEMORY_AND_DISK_SER) val words = lines.flatMap(_.split(" ")) val wordCounts = words.map(x => (x, 1)).reduceByKey(_ + _) wordCounts.print() ssc.start() Author: lisurprise <[email protected]> Closes #4828 from zhichao-li/master and squashes the following commits: c329806 [lisurprise] add test for attaching/detaching streaming tab 51e6c7f [lisurprise] move detach method into StreamingTab 31a44fa [lisurprise] add unit test for attaching and detaching new tab db25ed2 [lisurprise] clean code 8281bcb [lisurprise] clean code 193c542 [lisurprise] remove streaming tab while closing streaming context (cherry picked from commit f149b8b) Signed-off-by: Tathagata Das <[email protected]>

tdas · 2015-03-16T20:11:13Z

Merged this in master and branch-1.3

zhichao-li changed the title ~~[SPARK-6077] update listener for the existing streamingTab instead of adding an one new one~~ [SPARK-6077] update listener for the existing streamingTab instead of adding a new one Feb 28, 2015

srowen reviewed Feb 28, 2015
View reviewed changes

zhichao-li force-pushed the master branch from c3dd9cf to b7c92fe Compare March 2, 2015 03:16

srowen reviewed Mar 2, 2015
View reviewed changes

zhichao-li force-pushed the master branch from 2db756d to 7d4c71f Compare March 3, 2015 05:04

remove streaming tab while closing streaming context

193c542

zhichao-li force-pushed the master branch 2 times, most recently from 7694813 to 193c542 Compare March 3, 2015 05:19

jerryshao reviewed Mar 6, 2015
View reviewed changes

zhichao-li added 2 commits March 6, 2015 15:47

clean code

8281bcb

clean code

db25ed2

tdas reviewed Mar 6, 2015
View reviewed changes

core/src/main/scala/org/apache/spark/ui/WebUI.scala Outdated

Copy link

Contributor

tdas Mar 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

add unit test for attaching and detaching new tab

31a44fa

zhichao-li force-pushed the master branch from 4e9e4fc to 59180a7 Compare March 12, 2015 08:33

zhichao-li force-pushed the master branch from 59180a7 to bff699b Compare March 12, 2015 08:41

tdas reviewed Mar 13, 2015
View reviewed changes

add test for attaching/detaching streaming tab

c329806

zhichao-li force-pushed the master branch from bff699b to c329806 Compare March 13, 2015 08:26

asfgit closed this in f149b8b Mar 16, 2015

[SPARK-6077] Remove streaming tab while stopping StreamingContext #4828

[SPARK-6077] Remove streaming tab while stopping StreamingContext #4828

Uh oh!

Conversation

zhichao-li commented Feb 28, 2015

Uh oh!

AmplabJenkins commented Feb 28, 2015

Uh oh!

srowen Feb 28, 2015

Choose a reason for hiding this comment

Uh oh!

zhichao-li Mar 2, 2015

Choose a reason for hiding this comment

Uh oh!

srowen Mar 2, 2015

Choose a reason for hiding this comment

Uh oh!

zhichao-li Mar 2, 2015

Choose a reason for hiding this comment

Uh oh!

srowen commented Mar 2, 2015

Uh oh!

tdas commented Mar 2, 2015

Uh oh!

JoshRosen commented Mar 2, 2015

Uh oh!

tdas commented Mar 2, 2015

Uh oh!

JoshRosen commented Mar 2, 2015

Uh oh!

tdas commented Mar 2, 2015

Uh oh!

zhichao-li commented Mar 3, 2015

Uh oh!

jerryshao Mar 6, 2015

Choose a reason for hiding this comment

Uh oh!

tdas Mar 6, 2015

Choose a reason for hiding this comment

Uh oh!

tdas commented Mar 6, 2015

Uh oh!

tdas commented Mar 6, 2015

Uh oh!

JoshRosen commented Mar 6, 2015

Uh oh!

tdas commented Mar 12, 2015

Uh oh!

SparkQA commented Mar 12, 2015

Uh oh!

tdas commented Mar 12, 2015

Uh oh!

tdas commented Mar 12, 2015

Uh oh!

SparkQA commented Mar 12, 2015

Uh oh!

AmplabJenkins commented Mar 12, 2015

Uh oh!

SparkQA commented Mar 12, 2015

Uh oh!

SparkQA commented Mar 12, 2015

Uh oh!

zhichao-li commented Mar 12, 2015

Uh oh!

SparkQA commented Mar 12, 2015

Uh oh!

SparkQA commented Mar 12, 2015

Uh oh!

AmplabJenkins commented Mar 12, 2015

Uh oh!

SparkQA commented Mar 12, 2015

Uh oh!

AmplabJenkins commented Mar 12, 2015

Uh oh!

SparkQA commented Mar 12, 2015

Uh oh!

AmplabJenkins commented Mar 12, 2015

Uh oh!

tdas Mar 13, 2015

Choose a reason for hiding this comment