You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "bozhang2820 (via GitHub)" <gi...@apache.org> on 2024/06/05 14:30:44 UTC
[PR] [SPARK-48541][CORE] Add new exit code for executors killed by TaskReaper [spark]
bozhang2820 opened a new pull request, #46883:
URL: https://github.com/apache/spark/pull/46883
### What changes were proposed in this pull request?
This change adds a new exit code, 53, for executors killed by TaskReaper.
### Why are the changes needed?
This is to better monitor the cases when executors are killed by TaskReaper.
### Does this PR introduce _any_ user-facing change?
Yes. The exit code for executors killed by TaskReaper will change.
### How was this patch tested?
Updated unit tests.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48541][CORE] Add a new exit code for executors killed by TaskReaper [spark]
Posted by "JoshRosen (via GitHub)" <gi...@apache.org>.
JoshRosen commented on code in PR #46883:
URL: https://github.com/apache/spark/pull/46883#discussion_r1645244221
##########
core/src/main/scala/org/apache/spark/executor/Executor.scala:
##########
@@ -1310,3 +1309,5 @@ private[spark] object Executor {
}
}
}
+
+class KilledByTaskReaperException(message: String) extends SparkException(message)
Review Comment:
Makes sense: this is effectively a control flow exception.
##########
core/src/main/scala/org/apache/spark/executor/ExecutorExitCode.scala:
##########
@@ -45,6 +45,10 @@ object ExecutorExitCode {
*/
val HEARTBEAT_FAILURE = 56
+ /** The default uncaught exception handler was reached and the exception was thrown by
+ * TaskReaper. */
+ val KILLED_BY_TASK_REAPER = 57
+
def explainExitCode(exitCode: Int): String = {
Review Comment:
@bozhang2820, actually, whoops, I had overlooked this, but it looks like we should probably add a new case to `explainExitCode` here for the new `KILLED_BY_TASK_REAPER`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48541][CORE] Add new exit code for executors killed by TaskReaper [spark]
Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on PR #46883:
URL: https://github.com/apache/spark/pull/46883#issuecomment-2151299075
@JoshRosen, do you mind taking a look at this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48541][CORE] Add a new exit code for executors killed by TaskReaper [spark]
Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on code in PR #46883:
URL: https://github.com/apache/spark/pull/46883#discussion_r1645294286
##########
core/src/main/scala/org/apache/spark/executor/ExecutorExitCode.scala:
##########
@@ -45,6 +45,10 @@ object ExecutorExitCode {
*/
val HEARTBEAT_FAILURE = 56
+ /** The default uncaught exception handler was reached and the exception was thrown by
+ * TaskReaper. */
+ val KILLED_BY_TASK_REAPER = 57
+
def explainExitCode(exitCode: Int): String = {
Review Comment:
Thanks! Added a new case in `explainExitCode`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48541][CORE] Add new exit code for executors killed by TaskReaper [spark]
Posted by "bozhang2820 (via GitHub)" <gi...@apache.org>.
bozhang2820 commented on code in PR #46883:
URL: https://github.com/apache/spark/pull/46883#discussion_r1630446351
##########
core/src/main/scala/org/apache/spark/util/SparkExitCode.scala:
##########
@@ -45,6 +45,10 @@ private[spark] object SparkExitCode {
OutOfMemoryError. */
val OOM = 52
+ /** The default uncaught exception handler was reached and the exception was thrown by
+ TaskReaper. */
+ val KILLED_BY_TASK_REAPER = 53
Review Comment:
Thanks for catching this! Will move KILLED_BY_TASK_REAPER to ExecutorExitCode and change the value to 57.
##########
core/src/test/scala/org/apache/spark/JobCancellationSuite.scala:
##########
@@ -455,6 +464,8 @@ class JobCancellationSuite extends SparkFunSuite with Matchers with BeforeAndAft
sc.cancelJobGroup("jobA")
val e = intercept[SparkException] { ThreadUtils.awaitResult(jobA, 15.seconds) }.getCause
assert(e.getMessage contains "cancel")
+ semExec.acquire(2)
+ assert(execLossReason == Seq("Command exited with code 53", "Command exited with code 53"))
Review Comment:
Sure. Will do.
##########
core/src/main/scala/org/apache/spark/executor/Executor.scala:
##########
@@ -1310,3 +1309,5 @@ private[spark] object Executor {
}
}
}
+
+class KilledByTaskReaperException(message: String) extends SparkException(message)
Review Comment:
I did not use error class since this is not user-facing, and we've been using class matching in SparkUncaughtExceptionHandler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-48541][CORE] Add new exit code for executors killed by TaskReaper [spark]
Posted by "JoshRosen (via GitHub)" <gi...@apache.org>.
JoshRosen commented on code in PR #46883:
URL: https://github.com/apache/spark/pull/46883#discussion_r1630167906
##########
core/src/main/scala/org/apache/spark/executor/Executor.scala:
##########
@@ -1310,3 +1309,5 @@ private[spark] object Executor {
}
}
}
+
+class KilledByTaskReaperException(message: String) extends SparkException(message)
Review Comment:
Before, it looks like we'd throw `.internalError(msg, category = "EXECUTOR")` but now we're throwing a new SparkException with no error class set, so the resulting error class now becomes `INTERNAL_ERROR` whereas before it became `INTERNAL_ERROR_EXECUTOR`
Maybe we should migrate this to the error classes framework?
If we don't error class migrate it now, perhaps we should have this be `extends SparkException(message, cause = null, errorClass = "INTERNAL_ERROR_EXECUTOR")` so that we preserve the error class?
On the other hand, I'm not sure that this exception can even bubble to user code so in practice the exact error class might not matter: we're really just using this for control flow.
##########
core/src/test/scala/org/apache/spark/JobCancellationSuite.scala:
##########
@@ -455,6 +464,8 @@ class JobCancellationSuite extends SparkFunSuite with Matchers with BeforeAndAft
sc.cancelJobGroup("jobA")
val e = intercept[SparkException] { ThreadUtils.awaitResult(jobA, 15.seconds) }.getCause
assert(e.getMessage contains "cancel")
+ semExec.acquire(2)
+ assert(execLossReason == Seq("Command exited with code 53", "Command exited with code 53"))
Review Comment:
We could use the `ExecutorExitCode` constant here to make find usages work a little clearer.
##########
core/src/main/scala/org/apache/spark/util/SparkExitCode.scala:
##########
@@ -45,6 +45,10 @@ private[spark] object SparkExitCode {
OutOfMemoryError. */
val OOM = 52
+ /** The default uncaught exception handler was reached and the exception was thrown by
+ TaskReaper. */
+ val KILLED_BY_TASK_REAPER = 53
Review Comment:
I did a bit of digging to figure out whether exit code 53 might have had any special meaning in Spark or in other systems and found https://stackoverflow.com/questions/45428145/do-exit-codes-and-exit-statuses-mean-anything-in-spark, which reminded me of the existence of `ExecutorExitCode.scala` where it's already used for a BlockManager-related exit condition:
https://github.com/apache/spark/blob/0f21df0b29cc18f0e0c7b12543f3a037e4032e65/core/src/main/scala/org/apache/spark/executor/ExecutorExitCode.scala#L30-L34
Given that the task reaper kill is an executor-specific behavior, I think we should be adding a new exit code in `ExecutorExitCode.scala`. Let's maybe use the next free code, 57, for that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org