Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-7217][STREAMING] Add configuration to control the default behavior of StreamingContext.stop() implicitly calling SparkContext.stop() #5929

Closed
wants to merge 2 commits into from

Conversation

tdas
Copy link
Contributor

@tdas tdas commented May 6, 2015

In environments like notebooks, the SparkContext is managed by the underlying infrastructure and it is expected that the SparkContext will not be stopped. However, StreamingContext.stop() calls SparkContext.stop() as a non-intuitive side-effect. This PR adds a configuration in SparkConf that sets the default StreamingContext stop behavior. It should be such that the existing behavior does not change for existing users.

@tdas
Copy link
Contributor Author

tdas commented May 6, 2015

@pwendell @srowen Can you take a quick look at this. Any thought on the configuration name?

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@SparkQA
Copy link

SparkQA commented May 6, 2015

Test build #31928 has started for PR 5929 at commit 685fe00.

@harishreedharan
Copy link
Contributor

LGTM

@SparkQA
Copy link

SparkQA commented May 6, 2015

Test build #31928 has finished for PR 5929 at commit 685fe00.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins
Copy link

Merged build finished. Test FAILed.

@AmplabJenkins
Copy link

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

* to be processed).
*
* @param stopSparkContext if true, stops the associated SparkContext. The underlying SparkContext
* will be stopped regardless of whether this StreamingContext has been
* started.
*/
def stop(stopSparkContext: Boolean = true): Unit = synchronized {
def stop(stopSparkContext: Boolean): Unit = synchronized {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is actually api incompatible change no? I am wondering if there is a workaround here which does not break compat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it wasnt, but it seems to be. I will implement it differently.
On May 5, 2015 6:16 PM, "Hari Shreedharan" [email protected] wrote:

In
streaming/src/main/scala/org/apache/spark/streaming/StreamingContext.scala
#5929 (comment):

* to be processed).
*
* @param stopSparkContext if true, stops the associated SparkContext. The underlying SparkContext
*                         will be stopped regardless of whether this StreamingContext has been
*                         started.
*/
  • def stop(stopSparkContext: Boolean = true): Unit = synchronized {
  • def stop(stopSparkContext: Boolean): Unit = synchronized {

Hmm, this is actually api incompatible change no? I am wondering if there
is a workaround here which does not break compat.


Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/5929/files#r29728863.

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@SparkQA
Copy link

SparkQA commented May 6, 2015

Test build #31939 has started for PR 5929 at commit 869a763.

@SparkQA
Copy link

SparkQA commented May 6, 2015

Test build #31939 has finished for PR 5929 at commit 869a763.

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

@AmplabJenkins
Copy link

Merged build finished. Test PASSed.

@AmplabJenkins
Copy link

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

@srowen
Copy link
Member

srowen commented May 6, 2015

Seems OK to me. I agree with leaving it "hidden".

@tdas
Copy link
Contributor Author

tdas commented May 6, 2015

@srowen Yep, that's the plan for now.

On Tue, May 5, 2015 at 10:37 PM, Sean Owen [email protected] wrote:

Seems OK to me. I agree with leaving it "hidden".


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

@tdas
Copy link
Contributor Author

tdas commented May 7, 2015

I am merging this. Thanks @srowen @harishreedharan for taking a look.

@asfgit asfgit closed this in 01187f5 May 7, 2015
asfgit pushed a commit that referenced this pull request May 7, 2015
…avior of StreamingContext.stop() implicitly calling SparkContext.stop()

In environments like notebooks, the SparkContext is managed by the underlying infrastructure and it is expected that the SparkContext will not be stopped. However, StreamingContext.stop() calls SparkContext.stop() as a non-intuitive side-effect. This PR adds a configuration in SparkConf that sets the default StreamingContext stop behavior. It should be such that the existing behavior does not change for existing users.

Author: Tathagata Das <[email protected]>

Closes #5929 from tdas/SPARK-7217 and squashes the following commits:

869a763 [Tathagata Das] Changed implementation.
685fe00 [Tathagata Das] Added configuration

(cherry picked from commit 01187f5)
Signed-off-by: Tathagata Das <[email protected]>
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request May 28, 2015
…avior of StreamingContext.stop() implicitly calling SparkContext.stop()

In environments like notebooks, the SparkContext is managed by the underlying infrastructure and it is expected that the SparkContext will not be stopped. However, StreamingContext.stop() calls SparkContext.stop() as a non-intuitive side-effect. This PR adds a configuration in SparkConf that sets the default StreamingContext stop behavior. It should be such that the existing behavior does not change for existing users.

Author: Tathagata Das <[email protected]>

Closes apache#5929 from tdas/SPARK-7217 and squashes the following commits:

869a763 [Tathagata Das] Changed implementation.
685fe00 [Tathagata Das] Added configuration
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
…avior of StreamingContext.stop() implicitly calling SparkContext.stop()

In environments like notebooks, the SparkContext is managed by the underlying infrastructure and it is expected that the SparkContext will not be stopped. However, StreamingContext.stop() calls SparkContext.stop() as a non-intuitive side-effect. This PR adds a configuration in SparkConf that sets the default StreamingContext stop behavior. It should be such that the existing behavior does not change for existing users.

Author: Tathagata Das <[email protected]>

Closes apache#5929 from tdas/SPARK-7217 and squashes the following commits:

869a763 [Tathagata Das] Changed implementation.
685fe00 [Tathagata Das] Added configuration
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
…avior of StreamingContext.stop() implicitly calling SparkContext.stop()

In environments like notebooks, the SparkContext is managed by the underlying infrastructure and it is expected that the SparkContext will not be stopped. However, StreamingContext.stop() calls SparkContext.stop() as a non-intuitive side-effect. This PR adds a configuration in SparkConf that sets the default StreamingContext stop behavior. It should be such that the existing behavior does not change for existing users.

Author: Tathagata Das <[email protected]>

Closes apache#5929 from tdas/SPARK-7217 and squashes the following commits:

869a763 [Tathagata Das] Changed implementation.
685fe00 [Tathagata Das] Added configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants