You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by vanzin <gi...@git.apache.org> on 2018/01/24 22:14:33 UTC
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
GitHub user vanzin opened a pull request:
https://github.com/apache/spark/pull/20388
[SPARK-23020][core] Fix race in SparkAppHandle cleanup, again.
Third time is the charm?
There was still a race that was left in previous attempts. If the handle
closes the connection, the close() implementation would clean up state
that would prevent the thread from waiting on the connection thread to
finish. That could cause the race causing the test flakiness reported
in the bug.
The fix is to move the "wait for connection thread" code to a separate
close method that is used by the handle; that also simplifies the code
a bit and makes it also easier to follow.
I included an unrelated, but correct, change to a YARN test so that
it triggers when the PR is built.
Tested by inserting a sleep in the connection thread to mimic the race;
test failed reliably with the sleep, passes now. (Sleep not included in
the patch.) Also ran YARN tests to make sure.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vanzin/spark SPARK-23020
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/20388.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 #20388
----
commit fb14eaa918509f124a2a75155f8199b28de9a183
Author: Marcelo Vanzin <va...@...>
Date: 2018-01-24T21:48:52Z
[SPARK-23020][core] Fix race in SparkAppHandle cleanup, again.
Third time is the charm?
There was still a race that was left in previous attempts. If the handle
closes the connection, the close() implementation would clean up state
that would prevent the thread from waiting on the connection thread to
finish. That could cause the race causing the test flakiness reported
in the bug.
The fix is to move the "wait for connection thread" code to a separate
close method that is used by the handle; that also simplifies the code
a bit and makes it also easier to follow.
I included an unrelated, but correct, change to a YARN test so that
it triggers when the PR is built.
Tested by inserting a sleep in the connection thread to mimic the race;
test failed reliably with the sleep, passes now. (Sleep not included in
the patch.) Also ran YARN tests to make sure.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86616/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86616 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86616/testReport)** for PR 20388 at commit [`fb14eaa`](https://github.com/apache/spark/commit/fb14eaa918509f124a2a75155f8199b28de9a183).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86616 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86616/testReport)** for PR 20388 at commit [`fb14eaa`](https://github.com/apache/spark/commit/fb14eaa918509f124a2a75155f8199b28de9a183).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` class ServerConnection extends LauncherConnection `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/254/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86667 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86667/testReport)** for PR 20388 at commit [`358caff`](https://github.com/apache/spark/commit/358caffc4f0e57aa8935f523f285a5ebf6621c37).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86605/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/204/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86654 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86654/testReport)** for PR 20388 at commit [`b597705`](https://github.com/apache/spark/commit/b59770596b91cf91f56de448dbf386fd5fa93b8a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/20388
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20388#discussion_r163737892
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java ---
@@ -29,15 +30,15 @@
private final LauncherServer server;
- private LauncherConnection connection;
+ private LauncherServer.ServerConnection connection;
private List<Listener> listeners;
- private State state;
+ private AtomicReference<State> state;
--- End diff --
What's the rationale behind this? Previously we just make sure all access and modification to state is synchronized, are you changing it to `AtomicReference` for performance reasons?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20388#discussion_r164029896
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java ---
@@ -29,15 +30,15 @@
private final LauncherServer server;
- private LauncherConnection connection;
+ private LauncherServer.ServerConnection connection;
private List<Listener> listeners;
- private State state;
+ private AtomicReference<State> state;
--- End diff --
makes sense
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20388
seems unrelated. retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86667 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86667/testReport)** for PR 20388 at commit [`358caff`](https://github.com/apache/spark/commit/358caffc4f0e57aa8935f523f285a5ebf6621c37).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86654 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86654/testReport)** for PR 20388 at commit [`b597705`](https://github.com/apache/spark/commit/b59770596b91cf91f56de448dbf386fd5fa93b8a).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20388#discussion_r163743052
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java ---
@@ -99,8 +100,6 @@ boolean isDisposed() {
*/
synchronized void dispose() {
if (!isDisposed()) {
- // Unregister first to make sure that the connection with the app has been really
- // terminated.
server.unregister(this);
if (!getState().isFinal()) {
--- End diff --
do we need this `if`? `setState` would do nothing if state is final.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20388#discussion_r163939044
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java ---
@@ -99,8 +100,6 @@ boolean isDisposed() {
*/
synchronized void dispose() {
if (!isDisposed()) {
- // Unregister first to make sure that the connection with the app has been really
- // terminated.
server.unregister(this);
if (!getState().isFinal()) {
--- End diff --
It's not necessary, but it makes it clear that the code only wants to override the state in that case without having to read the code for `setState`.
(Or I could call `setState(LOST, false)` explicitly, I guess.)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86667/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86656/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20388
@cloud-fan @sameeragarwal since you looked at this before.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86656 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86656/testReport)** for PR 20388 at commit [`358caff`](https://github.com/apache/spark/commit/358caffc4f0e57aa8935f523f285a5ebf6621c37).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86654/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86656 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86656/testReport)** for PR 20388 at commit [`358caff`](https://github.com/apache/spark/commit/358caffc4f0e57aa8935f523f285a5ebf6621c37).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20388#discussion_r163938633
--- Diff: launcher/src/main/java/org/apache/spark/launcher/AbstractAppHandle.java ---
@@ -29,15 +30,15 @@
private final LauncherServer server;
- private LauncherConnection connection;
+ private LauncherServer.ServerConnection connection;
private List<Listener> listeners;
- private State state;
+ private AtomicReference<State> state;
--- End diff --
With the new code, synchronization would cause a deadlock.
- handle calls `closeAndWait()` inside synchronized block which joins connection thread
- connection thread would call `setState()` on the handle and cause a deadlock
Changing the state should be as thread-safe as before with the new code.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20388#discussion_r163939193
--- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
@@ -376,9 +353,23 @@ public void close() throws IOException {
clients.remove(this);
}
- synchronized (this) {
- super.close();
- notifyAll();
+ super.close();
+ }
+
+ /**
+ * Close the connection and wait for any buffered data to be processed before returning.
+ * This ensures any changes reported by the child application take effect.
+ */
+ public void closeAndWait() throws IOException {
+ close();
+
+ Thread connThread = this.connectionThread;
+ if (Thread.currentThread() != connThread) {
--- End diff --
More of a safeguard. I don't think it would happen in this version of the code, but better be safe.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/20388
thanks, merging to master/2.3!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/214/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20388#discussion_r163743675
--- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
@@ -376,9 +353,23 @@ public void close() throws IOException {
clients.remove(this);
}
- synchronized (this) {
- super.close();
- notifyAll();
+ super.close();
+ }
+
+ /**
+ * Close the connection and wait for any buffered data to be processed before returning.
+ * This ensures any changes reported by the child application take effect.
+ */
+ public void closeAndWait() throws IOException {
+ close();
+
+ Thread connThread = this.connectionThread;
+ if (Thread.currentThread() != connThread) {
--- End diff --
Is this a safeguard or it can really happen? i.e. the connection thread calls `closeAndWait`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/20388
Same flaky test. retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20388#discussion_r163742642
--- Diff: launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java ---
@@ -104,19 +104,12 @@ void monitorChild() {
ec = 1;
}
- State currState = getState();
- State newState = null;
if (ec != 0) {
+ State currState = getState();
// Override state with failure if the current state is not final, or is success.
if (!currState.isFinal() || currState == State.FINISHED) {
- newState = State.FAILED;
+ setState(State.FAILED, true);
}
- } else if (!currState.isFinal()) {
- newState = State.LOST;
--- End diff --
why is this not needed anymore?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86605 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86605/testReport)** for PR 20388 at commit [`fb14eaa`](https://github.com/apache/spark/commit/fb14eaa918509f124a2a75155f8199b28de9a183).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` class ServerConnection extends LauncherConnection `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/243/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/20388
**[Test build #86605 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86605/testReport)** for PR 20388 at commit [`fb14eaa`](https://github.com/apache/spark/commit/fb14eaa918509f124a2a75155f8199b28de9a183).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution/245/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20388: [SPARK-23020][core] Fix race in SparkAppHandle cl...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20388#discussion_r163938721
--- Diff: launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java ---
@@ -104,19 +104,12 @@ void monitorChild() {
ec = 1;
}
- State currState = getState();
- State newState = null;
if (ec != 0) {
+ State currState = getState();
// Override state with failure if the current state is not final, or is success.
if (!currState.isFinal() || currState == State.FINISHED) {
- newState = State.FAILED;
+ setState(State.FAILED, true);
}
- } else if (!currState.isFinal()) {
- newState = State.LOST;
--- End diff --
This is done by `dispose()` already.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20388: [SPARK-23020][core] Fix race in SparkAppHandle cleanup, ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20388
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org