You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by andrewor14 <gi...@git.apache.org> on 2015/03/03 10:03:56 UTC
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
GitHub user andrewor14 opened a pull request:
https://github.com/apache/spark/pull/4871
[SPARK-6133] Make sc.stop() idempotent
Before we would get the following error if we called `sc.stop()` twice. This is because the listener bus would try to post the end event again even after it has already stopped. This happens occasionally when flaky tests fail, usually as a result of other sources of error.
```
15/03/02 18:07:25.682 pool-1-thread-1 ERROR LiveListenerBus: SparkListenerBus has already stopped! Dropping event SparkListenerApplicationEnd(1425348445682)
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andrewor14/spark sc-stop
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/4871.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 #4871
----
commit 915db1620caee57126b436e9089c4715b5f1b766
Author: Andrew Or <an...@databricks.com>
Date: 2015-03-03T09:02:05Z
Move code into code
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-77060115
Ok thanks everyone for your comments. I'm merging this into master and 1.2. I will back port this into 1.3 later after the release.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-76921197
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28228/
Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-76921184
[Test build #28228 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28228/consoleFull) for PR 4871 at commit [`915db16`](https://github.com/apache/spark/commit/915db1620caee57126b436e9089c4715b5f1b766).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-77034073
> This line should have prevented 2 threads from going into the inner {...} and thefore changing the order of the lines wouldn't help.
I think the original issue here was that two sequential calls to `stop()` could cause spurious errors / warnings to occur, e.g.
```
sc.stop()
// ---- other stuff -----
sc.stop()
```
This can happen when `sc.stop()` is called in `finally` blocks; there are many cases that can lead to `stop()` being called twice on the same SparkContext, so it makes sense to have it be idempotent.
@srowen I'm not quite sure why `stopped` is set after posting the application end event or stopping the UI. Before I introduced the `stopped` variable, the old code would set `dagScheduler = null` and check that field's value to determine whether the SparkContext had stopped. That old code had the same property, so maybe there's a good reason why things were ordered this way (or maybe not; this could just be some vestigial artifact of how the code evolved over 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-77037115
@o-mdr This is not a race condition, so there are no concurrency issues here. The issue is that Spark calls `sc.stop()` internally sometimes when something bad happens, and then the application may decide to call `sc.stop()` again after that because it's what we recommend. This results in a benign error being logged that does not convey the root cause of the problem.
Also, note that any test we add here is not particularly meaningful because it would have passed even before this patch. For this reason I decided not to add one.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-77049609
I see, since we don't synchronize around the uses of `stopped`. E.g. if `postApplicationEnd` reads `stopped` in another thread (somehow) then this would matter.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/4871
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-76910010
[Test build #28228 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28228/consoleFull) for PR 4871 at commit [`915db16`](https://github.com/apache/spark/commit/915db1620caee57126b436e9089c4715b5f1b766).
* This patch merges cleanly.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-77057357
[Test build #28244 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28244/consoleFull) for PR 4871 at commit [`a14afc5`](https://github.com/apache/spark/commit/a14afc5f34128bf0f1abdf98bd886f9f2f68f7b0).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-77047210
It makes _some_ tiny difference; my observation was that the `stopped` flag's state can be observed outside this `synchronized` method. So its interleaving with other operations could matter. I have no idea if it does, but yeah, seems more logical to not put some operations before the flag state change. +1
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-77057365
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28244/
Test PASSed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-76910588
Just looking at the function it seems like this is the right thing to do. Is it right-er to move these two lines after `stopped = true`? This is read by non-`synchronized` methods so it's still possible the first two events happen before the `SparkContext` reports that it is stopped.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by o-mdr <gi...@git.apache.org>.
Github user o-mdr commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-76942656
def stop() {
SparkContext.SPARK_CONTEXT_CONSTRUCTOR_LOCK.synchronized {
This line should have prevented 2 threads from going into the inner `{...}` and thefore changing the order of the lines wouldn't help.
@srowen if this is read by non-syncronised methods, only one of them will be able to go inside this method and all the others will wait, correct? Thus it could be the problem with the others methods, that run some non-synchronised logic *before* calling into this syncronised method.
@andrewor14 Having a test would help.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-77040826
[Test build #28244 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/28244/consoleFull) for PR 4871 at commit [`a14afc5`](https://github.com/apache/spark/commit/a14afc5f34128bf0f1abdf98bd886f9f2f68f7b0).
* This patch merges cleanly.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-77040982
Ah, @srowen @o-mdr I misunderstood what you guys were saying. I think what @o-mdr meant was that moving the two lines after `stopped = true` wouldn't change anything because we in a synchronized block anyway. Yes, that is true, but I also agree with @srowen that it's a slightly more readable reorganization with no change in behavior. This is a super minor point, but I made the change anyway.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-6133] Make sc.stop() idempotent
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/4871#issuecomment-76948970
@o-mdr I'm referring to read of the `stopped` member elsewhere in this class. Yes of course `synchronized` excludes two threads from executing this block simultaneously. This is not true of reads of this field elsewhere in the class.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org