You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by kanzhang <gi...@git.apache.org> on 2014/04/03 00:24:59 UTC
[GitHub] spark pull request: SPARK-1118
GitHub user kanzhang opened a pull request:
https://github.com/apache/spark/pull/306
SPARK-1118
adding EXITED executor state and not relaunching normally exited executors
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kanzhang/spark SPARK-1118
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/306.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 #306
----
commit 59512dd960af1e19e6f532dace7582c96094bd7a
Author: Kan Zhang <kz...@apache.org>
Date: 2014-04-02T22:00:06Z
SPARK-1118 adding EXITED executor state and not relaunching normally exited executors
----
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-39398593
When the driver sends StopExecutor to executor (log msg "Driver commanded a shutdown"), it also ends up here. So it can be a normal exit depends on the exit code. See my comment here https://issues.apache.org/jira/browse/SPARK-1118?focusedCommentId=13958329&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13958329
There are 2 other places where FAILED is used. One is when exception is caught in this same method. The other is when exception is caught in LaunchExecutor() in Worker.scala.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11235230
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -290,10 +290,11 @@ private[spark] class Master(
appInfo.removeExecutor(exec)
exec.worker.removeExecutor(exec)
+ val normalExit = exitStatus.exists(_ == 0)
--- End diff --
My line 370 is " addressToWorker.get(address).foreach(removeWorker)". I'm not sure this is what you meant.
> the current implementation set the Executor status to KILLED no matter it is exists normally or is due to the ctrl+c
the current implementation sets Executor status to FAILED when it exits normally and that's what I wanted to correct here, by introducing EXITED status, which could mean either normal or abnormal exit.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11234306
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -290,10 +290,11 @@ private[spark] class Master(
appInfo.removeExecutor(exec)
exec.worker.removeExecutor(exec)
+ val normalExit = exitStatus.exists(_ == 0)
--- End diff --
I think the tricky part is in L370, when you run a standalone application, it exists as as normal, but will trigger a DisassociatedEvent to the master, the current implementation set the Executor status to KILLED no matter it is exists normally or is due to the ctrl+c.....(I didn't test it carefully...)
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-40124236
Could someone review and merge this patch?
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-39399935
Ah, I see, your test program SparkPI just does everything in a standard way, call new SparkContext and then sc.stop()
so everything seems straightforward.....
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-46105771
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/15801/
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11234468
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -290,10 +290,11 @@ private[spark] class Master(
appInfo.removeExecutor(exec)
exec.worker.removeExecutor(exec)
+ val normalExit = exitStatus.exists(_ == 0)
--- End diff --
L370?
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/306
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11237143
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/ExecutorRunner.scala ---
@@ -142,7 +142,7 @@ private[spark] class ExecutorRunner(
// long-lived processes only. However, in the future, we might restart the executor a few
// times on the same machine.
val exitCode = process.waitFor()
- state = ExecutorState.FAILED
--- End diff --
BTW, when Worker kills an executor, the ExecutorRunner.workerThread got interrupted, while the Executor process itself is destroyed. The above line in ExecutorRunner will not get executed, since the thread got interrupted. The executor state will not be set here. Instead, it is set in the ExecutorRunner.kill() as KILLED.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11233840
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/ExecutorRunner.scala ---
@@ -142,7 +142,7 @@ private[spark] class ExecutorRunner(
// long-lived processes only. However, in the future, we might restart the executor a few
// times on the same machine.
val exitCode = process.waitFor()
- state = ExecutorState.FAILED
--- End diff --
so, when will you use FAILED? I think at least this place, it should be FAILED...because the executor is actually stopped by InterruptException, if your execution flow comes to this place, it means it is certainly failed
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-42698455
Since SPARK-1118 has been closed as duplicate of SPARK-937, I'm re-submitting this PR as SPARK-937.
@aarondav feel free to take over if you still plan to work on SPARK-937.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11237180
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/ExecutorRunner.scala ---
@@ -142,7 +142,7 @@ private[spark] class ExecutorRunner(
// long-lived processes only. However, in the future, we might restart the executor a few
// times on the same machine.
val exitCode = process.waitFor()
- state = ExecutorState.FAILED
--- End diff --
yes, that's what I was saying, I thought this line would never be executed in normal case, once executed, it should be FAILED, (I missed the sc.stop() calling )
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-39400244
@kanzhang I think the tricky part is just here, a normally exited program seems the same with the ones being interrupted during the running .....though I think your PR can solve the program and in the other hand, we can remind user to call sc.stop() after everything is done in 1.0 document.....
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-39399719
so, you are assuming that the user always explicitly call sc.stop(), but that's not always true, even the example program in spark's document does not do that
http://spark.apache.org/docs/latest/quick-start.html#a-standalone-app-in-scala
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-39399983
That's ok. If the driver doesn't instruct executor to shutdown, the process won't exit, right? The executor will eventually be KILLED by Master when app disassociates.
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-46106514
@aarondav thanks for reviewing this patch and tracking remaining issues. Pls let me know if there is anything I can do to improve this patch.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-39404001
Thanks for your comments. I just did a test of the ctrl+c case using Spark Shell. When I used ctrl+c to exit the shell, both Executor log and Master log showed that they detected app/driver disassociated. The Executor tries to shut itself down with exit code 1. However, the Master's finishApplication() wins in my test. Worker's log shows it accepted Master's request to kill the executor and the executor's final state is KILLED. It could have happened the other way with the executor exited with code 1, Worker marked its state as EXITED, and Master tried to launch a replacement executor (since exit code is not 0), which eventually got KILLED when Master detects app disassociated. Either way is fine in my opinion. I don't think we need to force users to call stop(). It's just that if it is not called, its executors may get KILLED.
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-46105232
Merged build triggered.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-40303520
test
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-46098404
@aarondav can you take a look again at this?
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-46105181
I agree that this may not cause the status to be EXITED in all cases, but this definitely seems like an improvement over the prior behavior.
One thing I noticed while testing this PR is that if you do exit cleanly, the Master Web UI now actually forgets about all the Executors! So you wouldn't see the "EXITED" status at all. I created [SPARK-2147](https://issues.apache.org/jira/browse/SPARK-2147) to track this issue.
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-46105183
Jenkins, test this please.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-39391986
Can one of the admins verify this patch?
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-39409752
Code comments updated.
Any more review comments? Thx.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11234539
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -290,10 +290,11 @@ private[spark] class Master(
appInfo.removeExecutor(exec)
exec.worker.removeExecutor(exec)
+ val normalExit = exitStatus.exists(_ == 0)
--- End diff --
Line 370
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by kanzhang <gi...@git.apache.org>.
Github user kanzhang commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11236480
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/ExecutorRunner.scala ---
@@ -142,7 +142,7 @@ private[spark] class ExecutorRunner(
// long-lived processes only. However, in the future, we might restart the executor a few
// times on the same machine.
val exitCode = process.waitFor()
- state = ExecutorState.FAILED
--- End diff --
Sure, I'll update the comments.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11235592
--- Diff: core/src/main/scala/org/apache/spark/deploy/worker/ExecutorRunner.scala ---
@@ -142,7 +142,7 @@ private[spark] class ExecutorRunner(
// long-lived processes only. However, in the future, we might restart the executor a few
// times on the same machine.
val exitCode = process.waitFor()
- state = ExecutorState.FAILED
--- End diff --
BTW, do you mind fixing the comments... I think it's out of date according to your finding......the user program can call sc.stop() to explicitly exit the Executor now, and the return value is 0....
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-46129232
Cool, merged into master and branch-1.0. Thanks!
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-46105236
Merged build started.
---
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.
---
[GitHub] spark pull request: SPARK-1118
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on a diff in the pull request:
https://github.com/apache/spark/pull/306#discussion_r11235339
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -290,10 +290,11 @@ private[spark] class Master(
appInfo.removeExecutor(exec)
exec.worker.removeExecutor(exec)
+ val normalExit = exitStatus.exists(_ == 0)
--- End diff --
Sorry, L367, no...I mean when the driver program exists normally or due to ctrl + c
---
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.
---
[GitHub] spark pull request: [SPARK-937] adding EXITED executor state and n...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/306#issuecomment-46105770
Merged build finished. All automated tests 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.
---