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-11872] Prevent the call to SparkContext#stop() in the listener bus's thread #9852

Closed
wants to merge 2 commits into from

Conversation

tedyu
Copy link
Contributor

@tedyu tedyu commented Nov 20, 2015

This is continuation of SPARK-11761

Andrew suggested adding this protection. See tail of #9741

@tedyu tedyu changed the title Prevent the call to SparkContext#stop() in the listener bus's thread [SPARK-11872] Prevent the call to SparkContext#stop() in the listener bus's thread Nov 20, 2015
@SparkQA
Copy link

SparkQA commented Nov 20, 2015

Test build #46376 has finished for PR 9852 at commit 19c144e.

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

@tedyu
Copy link
Contributor Author

tedyu commented Nov 20, 2015

@andrewor14 @zsxwing
Kindly review

@zsxwing
Copy link
Member

zsxwing commented Nov 20, 2015

@tedyu could you add a unit test? Just to avoid someone breaking it in future.

@tedyu
Copy link
Contributor Author

tedyu commented Nov 20, 2015

Should I add test in StreamingListenerSuite.scala (stopping SparkContext instead of stopping StreamingContext) ?
Which notification should be overridden ?

Thanks

@zsxwing
Copy link
Member

zsxwing commented Nov 20, 2015

You can add the test to SparkListenerSuite in core

@SparkQA
Copy link

SparkQA commented Nov 20, 2015

Test build #46430 has finished for PR 9852 at commit 15fb7b2.

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

@zsxwing
Copy link
Member

zsxwing commented Nov 20, 2015

retest this please

@tedyu
Copy link
Contributor Author

tedyu commented Nov 20, 2015

[error] Failed: Total 378, Failed 1, Errors 0, Passed 377, Ignored 2
[error] Failed tests:
[error]     org.apache.spark.streaming.CheckpointSuite
[info] ExecutorClassLoaderSuite:
[info] - child first (4 seconds, 178 milliseconds)
[info] - parent first (49 milliseconds)
[info] - child first can fall back (47 milliseconds)
[info] - child first can fail (44 milliseconds)
[info] - failing to fetch classes from HTTP server should not leak resources (SPARK-6209) *** FAILED *** (1 second, 886 milliseconds)
[info]   java.lang.ClassNotFoundException: ReplFakeClass2
[info]   at org.apache.spark.repl.ExecutorClassLoader.findClass(ExecutorClassLoader.scala:68)

I don't think the above failures were related to my change

@tedyu
Copy link
Contributor Author

tedyu commented Nov 20, 2015

Manually ran org.apache.spark.streaming.CheckpointSuite (with this change) which passed.

@SparkQA
Copy link

SparkQA commented Nov 20, 2015

Test build #46445 has finished for PR 9852 at commit 15fb7b2.

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

@zsxwing
Copy link
Member

zsxwing commented Nov 20, 2015

LGTM

@tedyu
Copy link
Contributor Author

tedyu commented Nov 23, 2015

@zsxwing @andrewor14
Is there anything I need to do ?

asfgit pushed a commit that referenced this pull request Nov 24, 2015
… bus's thread

This is continuation of SPARK-11761

Andrew suggested adding this protection. See tail of #9741

Author: tedyu <yuzhihong@gmail.com>

Closes #9852 from tedyu/master.

(cherry picked from commit 8101254)
Signed-off-by: Shixiong Zhu <shixiong@databricks.com>
@asfgit asfgit closed this in 8101254 Nov 24, 2015
@zsxwing
Copy link
Member

zsxwing commented Nov 24, 2015

Merged to master and 1.6. Thanks!

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.

3 participants