Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-61010433
Thanks. Merged in master branch-1.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
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/2957
---
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
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/2957#discussion_r19455126
--- Diff: core/src/main/scala/org/apache/spark/FutureAction.scala ---
@@ -210,7 +210,9 @@ class ComplexFutureAction[T] extends FutureAction[T] {
}
Github user zsxwing commented on a diff in the pull request:
https://github.com/apache/spark/pull/2957#discussion_r19455903
--- Diff: core/src/main/scala/org/apache/spark/FutureAction.scala ---
@@ -210,7 +210,9 @@ class ComplexFutureAction[T] extends FutureAction[T] {
}
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/2957#discussion_r19503380
--- Diff: core/src/main/scala/org/apache/spark/FutureAction.scala ---
@@ -210,7 +210,9 @@ class ComplexFutureAction[T] extends FutureAction[T] {
}
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60857035
@rxin Done. Is it OK?
---
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
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60857510
[Test build #22403 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22403/consoleFull)
for PR 2957 at commit
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60863669
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60863666
[Test build #22403 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22403/consoleFull)
for PR 2957 at commit
GitHub user zsxwing opened a pull request:
https://github.com/apache/spark/pull/2957
[SPARK-4097] Fix the race condition of 'thread'
There is a chance that `thread` is null when calling `thread.interrupt()`.
```Scala
override def cancel(): Unit = this.synchronized {
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60561668
[Test build #22284 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22284/consoleFull)
for PR 2957 at commit
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60563934
You can also just take a local reference to the thread, and operate on it.
The local reference will of course be null in both cases or not-null in both
cases.
---
If
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60565342
You can also just take a local reference to the thread, and operate on
it. The local reference will of course be null in both cases or not-null in
both cases.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60565582
[Test build #22289 has
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22289/consoleFull)
for PR 2957 at commit
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60566474
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60568910
[Test build #22284 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22284/consoleFull)
for PR 2957 at commit
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60568924
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60572797
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60572789
[Test build #22289 has
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/22289/consoleFull)
for PR 2957 at commit
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60574364
@zsxwing You're trying to handle the case where `thread` changes between
checking `thread != null` and calling `thread.interrupt()` right? Copying
`thread` to a local
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60577583
The usage of `run` is in `AsyncRDDActions.takeAsync`. It uses
`scala.concurrent.ExecutionContext.Implicits.global` as the implicit
`ExecutionContext`. Because
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60578902
How would running another task X change the value of `t` in T1's stack? I
understand it could modify `thread`.
---
If your project is set up for it, you can reply to
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60579222
How would running another task X change the value of t in T1's stack?
No, I mean another task X will observe `t.interrupt()` because task X
happens to run in
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60579828
Ah right. But wasn't this already a potential problem and still after this
change? the caller's call to `cancel()` may be just about to happen, when the
old task
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60580481
No, I checked `ThreadPoolExecutor` here:
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60580846
In summary, the local variable solution may interrupt the thread during
running another new task. So that's a potential problem.
Although my solution here may
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60581156
But the call to interrupt the old task interrupt happens after the new task
begins on the thread. The prior interrupt state doesn't matter; the thread is
interrupted in
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60581389
But the call to interrupt the old task interrupt happens after the new
task begins on the thread.
Because I use the same lock, this should not happen.
---
If
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60581813
There are only two cases:
```Scala
t1:
if (thread != null) {
thread.interrupt()
}
t2:
thread = null
```
Or:
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60583305
Yes I get that this is not just about preventing an NPE, which is what I
thought the intent was originally. I think this does not go far enough to
prevent the problem
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60583790
You example assumes `ComplexFutureAction.run` may be called more than once.
However after reviewing other codes, I don't think `ComplexFutureAction.run`
can be called
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60584045
If `ComplexFutureAction.run` is called only once, I think my current fix is
enough.
---
If your project is set up for it, you can reply to this email and have your
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60584425
Here is the usage of `ComplexFutureAction`:
https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/rdd/AsyncRDDActions.scala#L66
I
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60584591
Yeah I think that would be enough to guarantee it, and it makes sense.
Either interrupt happens before the task nulls the reference, in which case the
task is not done no
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/2957#issuecomment-60584794
@srowen thank you for reviewing it:)
---
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
35 matches
Mail list logo
