You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by jerryshao <gi...@git.apache.org> on 2015/09/11 09:33:44 UTC
[GitHub] spark pull request: [SPARK-10588][Core] Fix wrong executor state i...
GitHub user jerryshao opened a pull request:
https://github.com/apache/spark/pull/8714
[SPARK-10588][Core] Fix wrong executor state in Master
`ExecutorAdded` can only be sent to `AppClient` when worker report back the executor state as `LOADING`, otherwise because of concurrency issue, `AppClient` will possibly receive `ExectuorAdded` at first, then `ExecutorStateUpdated` with `LOADING` state.
Also Master will change the executor state from `LAUNCHING` to `RUNNING`, then to `LOADING`, it should be `LAUNCHING` -> `LOADING` -> `RUNNING`.
Also it is wrongly shown in master UI, the state of executor should be `RUNNING` rather than `LOADING`:
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jerryshao/apache-spark SPARK-10558
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/8714.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 #8714
----
commit 57e750801610bd7b5d70070a98b0f1421dfb4d92
Author: jerryshao <ss...@hortonworks.com>
Date: 2015-09-11T07:03:59Z
Fix wrong executor state in Master
----
---
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-10558][Core] Fix wrong executor state i...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the pull request:
https://github.com/apache/spark/pull/8714#issuecomment-140790730
@andrewor14 , would you please help to review this patch? Thanks a lot.
---
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-10558][Core] Fix wrong executor state i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/8714#discussion_r39256826
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -278,9 +278,24 @@ private[deploy] class Master(
execOption match {
case Some(exec) => {
val appInfo = idToApp(appId)
+ val oldState = exec.state
exec.state = state
- if (state == ExecutorState.RUNNING) { appInfo.resetRetryCount() }
+
+ if (state == ExecutorState.RUNNING) {
+ assert(oldState == ExecutorState.LOADING,
--- End diff --
Yes -- asserts in the JVM are disabled by default! this would not check anything then. I checked and it's not what the Scala method does though. However the Scala `assert` can be removed at compile time. And it generates an `AssertionError` which is extreme for this situation.
I think that if you intend it to always be present as argument/state checking, then `require` is good?
---
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-10558][Core] Fix wrong executor state i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/8714#discussion_r39254790
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -278,9 +278,24 @@ private[deploy] class Master(
execOption match {
case Some(exec) => {
val appInfo = idToApp(appId)
+ val oldState = exec.state
exec.state = state
- if (state == ExecutorState.RUNNING) { appInfo.resetRetryCount() }
+
+ if (state == ExecutorState.RUNNING) {
+ assert(oldState == ExecutorState.LOADING,
--- End diff --
Does `assert` do much the same as `require` in Scala? just making sure this isn't a Java-style `assert` underneath.
Does this cause the master to fail then if the state transitions happen out of order? I pretty much agree that ignoring it isn't good either, but is this fixing the problem? For example if you get `RUNNING` after `LAUNCHING`, you can assume `LOADING` happened, and ignore subsequent `LOADING` messages instead? or does that make more problems?
---
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-10558][Core] Fix wrong executor state i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/8714#issuecomment-139511643
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42316/
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-10588][Core] Fix wrong executor state i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/8714#issuecomment-139474541
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10558][Core] Fix wrong executor state i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/8714#issuecomment-139511642
Merged build finished. 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-10558][Core] Fix wrong executor state i...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/8714#issuecomment-139475969
[Test build #42316 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42316/consoleFull) for PR 8714 at commit [`57e7508`](https://github.com/apache/spark/commit/57e750801610bd7b5d70070a98b0f1421dfb4d92).
---
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-10558][Core] Fix wrong executor state i...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/8714#discussion_r39255623
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -278,9 +278,24 @@ private[deploy] class Master(
execOption match {
case Some(exec) => {
val appInfo = idToApp(appId)
+ val oldState = exec.state
exec.state = state
- if (state == ExecutorState.RUNNING) { appInfo.resetRetryCount() }
+
+ if (state == ExecutorState.RUNNING) {
+ assert(oldState == ExecutorState.LOADING,
--- End diff --
Is there problem using java-style assertion?
The master will not fail, this exception will be catched by RPC framework. I think if RPC framework can assure exact once message delivery and message ordering, this assertion will not bring further problems.
With assertion, this issue can also be fixed, but here I added some assertions is to make sure state transition is correct.
---
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-10558][Core] Fix wrong executor state i...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/8714#discussion_r39255025
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -278,9 +278,24 @@ private[deploy] class Master(
execOption match {
case Some(exec) => {
val appInfo = idToApp(appId)
+ val oldState = exec.state
exec.state = state
- if (state == ExecutorState.RUNNING) { appInfo.resetRetryCount() }
+
+ if (state == ExecutorState.RUNNING) {
+ assert(oldState == ExecutorState.LOADING,
+ s"executor $execId state transfer from $oldState to RUNNING is illegal")
+ appInfo.resetRetryCount()
+ }
+
exec.application.driver.send(ExecutorUpdated(execId, state, message, exitStatus))
+
+ if (state == ExecutorState.LOADING) {
+ assert(oldState == ExecutorState.LAUNCHING,
+ s"executor $execId state transfer from $oldState to LOADING is illegal")
+ exec.application.driver.send(
--- End diff --
Yes, driver only can be told when master get executor state from worker, that is a correct sequence for state transition.
---
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-10558][Core] Fix wrong executor state i...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/8714#discussion_r39254806
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -278,9 +278,24 @@ private[deploy] class Master(
execOption match {
case Some(exec) => {
val appInfo = idToApp(appId)
+ val oldState = exec.state
exec.state = state
- if (state == ExecutorState.RUNNING) { appInfo.resetRetryCount() }
+
+ if (state == ExecutorState.RUNNING) {
+ assert(oldState == ExecutorState.LOADING,
+ s"executor $execId state transfer from $oldState to RUNNING is illegal")
+ appInfo.resetRetryCount()
+ }
+
exec.application.driver.send(ExecutorUpdated(execId, state, message, exitStatus))
+
+ if (state == ExecutorState.LOADING) {
+ assert(oldState == ExecutorState.LAUNCHING,
+ s"executor $execId state transfer from $oldState to LOADING is illegal")
+ exec.application.driver.send(
--- End diff --
Nice, so the driver only is told that the executor is running once the master thinks it is now?
---
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-10558][Core] Fix wrong executor state i...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/8714#discussion_r39257093
--- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
@@ -278,9 +278,24 @@ private[deploy] class Master(
execOption match {
case Some(exec) => {
val appInfo = idToApp(appId)
+ val oldState = exec.state
exec.state = state
- if (state == ExecutorState.RUNNING) { appInfo.resetRetryCount() }
+
+ if (state == ExecutorState.RUNNING) {
+ assert(oldState == ExecutorState.LOADING,
--- End diff --
Thanks for your explanation, I will update 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 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-10588][Core] Fix wrong executor state i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/8714#issuecomment-139474584
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-10558][Core] Fix wrong executor state i...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/8714#issuecomment-139511484
[Test build #42316 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/42316/console) for PR 8714 at commit [`57e7508`](https://github.com/apache/spark/commit/57e750801610bd7b5d70070a98b0f1421dfb4d92).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `class ShuffleDependency[K: ClassTag, V: ClassTag, C: ClassTag](`
* `class CoGroupedRDD[K: ClassTag](`
* `class ShuffledRDD[K: ClassTag, V: ClassTag, C: ClassTag](`
---
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