[SPARK-8743] [Streaming]: Deregister Codahale metrics for streaming when StreamingContext is closed #7362

nssalian · 2015-07-13T01:41:44Z

The issue link: https://issues.apache.org/jira/browse/SPARK-8743
Deregister Codahale metrics for streaming when StreamingContext is closed

Design:
Adding the method calls in the appropriate start() and stop () methods for the StreamingContext

Actions in the PullRequest:

Added the registerSource method call to the start method for the Streaming Context.
Added the removeSource method to the stop method.
Added comments for both 1 and 2 and comment to show initialization of the StreamingSource
Added a test case to check for both registration and de-registration of metrics

Previous closed PR for reference: #7250

…est to check the registration and de-registration

nssalian · 2015-07-13T01:43:12Z

@tdas and @srowen, the new PR for the JIRA.

Thank you.

srowen · 2015-07-13T09:45:39Z

streaming/src/main/scala/org/apache/spark/streaming/StreamingContext.scala

@@ -192,11 +192,8 @@ class StreamingContext private[streaming] (
      None
    }

-  /** Register streaming source to metrics system */
+  /* Initializing a streamingSource to register metrics */


Not a big deal, but I don't see why this was changed

It previously held the block of code that did the registration.
Here it simply initializes the streamingSource since the registration is moved into start()

nssalian · 2015-07-13T13:48:22Z

Added the changes and ran ./dev/scalastyle and ~test-only *StreamingContextSuite.
Both passed.

SparkQA · 2015-07-13T19:32:05Z

Test build #1060 has finished for PR 7362 at commit 0e8007a.

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

nssalian · 2015-07-13T20:58:41Z

@tdas does this PR need more improvement?

tdas · 2015-07-13T21:41:26Z

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

+    val streamingSource = StreamingContextSuite.getStreamingSource(ssc)
+    assert(sources.contains(streamingSource))
+    assert(ssc.getState() === StreamingContextState.ACTIVE)
+    Thread.sleep(100)


Any particular reason why you added this sleep?

Removed it. Added it during my runs.

Thanks.

tdas · 2015-07-13T21:42:13Z

LGTM, minus a nit on the Thread.sleep(). If there is no need for it please remove it. We dont want to increase unit test runtime unnecessarily.

nssalian · 2015-07-13T21:51:31Z

@tdas Removed it.
Thank you for the review.

tdas · 2015-07-13T21:56:54Z

Jenkins, this is ok to test.

andrewor14 · 2015-07-13T21:58:03Z

Jenkins, slow test please

SparkQA · 2015-07-13T22:42:43Z

Test build #37155 has finished for PR 7362 at commit 7d998a3.

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

tdas · 2015-07-13T22:45:59Z

LGTM! Merging this in master and branch 1.4. Thanks!

SparkQA · 2015-07-13T22:55:20Z

Test build #3 has finished for PR 7362 at commit 7d998a3.

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

…en StreamingContext is closed The issue link: https://issues.apache.org/jira/browse/SPARK-8743 Deregister Codahale metrics for streaming when StreamingContext is closed Design: Adding the method calls in the appropriate start() and stop () methods for the StreamingContext Actions in the PullRequest: 1) Added the registerSource method call to the start method for the Streaming Context. 2) Added the removeSource method to the stop method. 3) Added comments for both 1 and 2 and comment to show initialization of the StreamingSource 4) Added a test case to check for both registration and de-registration of metrics Previous closed PR for reference: #7250 Author: Neelesh Srinivas Salian <[email protected]> Closes #7362 from nssalian/branch-SPARK-8743 and squashes the following commits: 7d998a3 [Neelesh Srinivas Salian] Removed the Thread.sleep() call 8b26397 [Neelesh Srinivas Salian] Moved the scalatest.{} import 0e8007a [Neelesh Srinivas Salian] moved import org.apache.spark{} to correct place daedaa5 [Neelesh Srinivas Salian] Corrected Ordering of imports 8873180 [Neelesh Srinivas Salian] Removed redundancy in imports 59227a4 [Neelesh Srinivas Salian] Changed the ordering of the imports to classify scala and spark imports d8cb577 [Neelesh Srinivas Salian] Added registerSource to start() and removeSource to stop(). Wrote a test to check the registration and de-registration

…en StreamingContext is closed The issue link: https://issues.apache.org/jira/browse/SPARK-8743 Deregister Codahale metrics for streaming when StreamingContext is closed Design: Adding the method calls in the appropriate start() and stop () methods for the StreamingContext Actions in the PullRequest: 1) Added the registerSource method call to the start method for the Streaming Context. 2) Added the removeSource method to the stop method. 3) Added comments for both 1 and 2 and comment to show initialization of the StreamingSource 4) Added a test case to check for both registration and de-registration of metrics Previous closed PR for reference: #7250 Author: Neelesh Srinivas Salian <[email protected]> Closes #7362 from nssalian/branch-SPARK-8743 and squashes the following commits: 7d998a3 [Neelesh Srinivas Salian] Removed the Thread.sleep() call 8b26397 [Neelesh Srinivas Salian] Moved the scalatest.{} import 0e8007a [Neelesh Srinivas Salian] moved import org.apache.spark{} to correct place daedaa5 [Neelesh Srinivas Salian] Corrected Ordering of imports 8873180 [Neelesh Srinivas Salian] Removed redundancy in imports 59227a4 [Neelesh Srinivas Salian] Changed the ordering of the imports to classify scala and spark imports d8cb577 [Neelesh Srinivas Salian] Added registerSource to start() and removeSource to stop(). Wrote a test to check the registration and de-registration (cherry picked from commit b7bcbe2) Signed-off-by: Tathagata Das <[email protected]>

tdas · 2015-07-14T21:32:40Z

This has been merged. But for some reason the PR was not automatically closed. So could you close this PR. Thanks!

nssalian · 2015-07-14T21:38:23Z

Closing. Thank you.

Added registerSource to start() and removeSource to stop(). Wrote a t…

d8cb577

…est to check the registration and de-registration

nssalian mentioned this pull request Jul 13, 2015

[SPARK-8743] [Streaming]: Deregister Codahale metrics for streaming when StreamingContext is closed #7250

Closed

srowen reviewed Jul 13, 2015
View reviewed changes

Neelesh Srinivas Salian added 4 commits July 13, 2015 05:07

Changed the ordering of the imports to classify scala and spark imports

59227a4

Removed redundancy in imports

8873180

Corrected Ordering of imports

daedaa5

moved import org.apache.spark{} to correct place

0e8007a

Moved the scalatest.{} import

8b26397

tdas reviewed Jul 13, 2015
View reviewed changes

Removed the Thread.sleep() call

7d998a3

nssalian closed this Jul 14, 2015

[SPARK-8743] [Streaming]: Deregister Codahale metrics for streaming when StreamingContext is closed #7362

[SPARK-8743] [Streaming]: Deregister Codahale metrics for streaming when StreamingContext is closed #7362

Uh oh!

Conversation

nssalian commented Jul 13, 2015

Uh oh!

nssalian commented Jul 13, 2015

Uh oh!

srowen Jul 13, 2015

Choose a reason for hiding this comment

Uh oh!

nssalian Jul 13, 2015

Choose a reason for hiding this comment

Uh oh!

nssalian commented Jul 13, 2015

Uh oh!

SparkQA commented Jul 13, 2015

Uh oh!

nssalian commented Jul 13, 2015

Uh oh!

tdas Jul 13, 2015

Choose a reason for hiding this comment

Uh oh!

nssalian Jul 13, 2015

Choose a reason for hiding this comment

Uh oh!

tdas commented Jul 13, 2015

Uh oh!

nssalian commented Jul 13, 2015

Uh oh!

tdas commented Jul 13, 2015

Uh oh!

andrewor14 commented Jul 13, 2015

Uh oh!

SparkQA commented Jul 13, 2015

Uh oh!

tdas commented Jul 13, 2015

Uh oh!

SparkQA commented Jul 13, 2015

Uh oh!

tdas commented Jul 14, 2015

Uh oh!

nssalian commented Jul 14, 2015

Uh oh!

Uh oh!