You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "pan3793 (via GitHub)" <gi...@apache.org> on 2023/03/30 09:05:16 UTC
[GitHub] [spark] pan3793 opened a new pull request, #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
pan3793 opened a new pull request, #38732:
URL: https://github.com/apache/spark/pull/38732
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
Fail Spark Application when number of executor failures reach threshold.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Sometimes, the executors can not launch successfully because of the wrong configuration, but in K8s, Driver does not know that, and just keep requesting new executors.
This adds functionality similar to YARN[1] to K8s.
[1] [SPARK-6735](https://issues.apache.org/jira/browse/SPARK-6735)
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
New feature.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
New UT added, and manually tested in internal K8s cluster.
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1038973719
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -136,6 +151,10 @@ class ExecutorPodsAllocator(
def isDeleted(executorId: String): Boolean = deletedExecutorIds.contains(executorId.toLong)
+ private[k8s] def stopApplication(exitCode: Int): Unit = {
+ sys.exit(exitCode)
+ }
Review Comment:
I see the problem, and actually I don't know how to handle it properly ...
I use `sys.exit` here just want to kill the process or driver pod, so that `ShutdownHookManager` will be invoked.
--
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
[GitHub] [spark] yaooqinn commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1152961738
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -148,6 +163,10 @@ class ExecutorPodsAllocator(
def isDeleted(executorId: String): Boolean = deletedExecutorIds.contains(executorId.toLong)
+ private[k8s] def stopApplication(exitCode: Int): Unit = {
+ sys.exit(exitCode)
Review Comment:
not necessary to create a new func?
--
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
[GitHub] [spark] yaooqinn commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1152960657
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -117,6 +120,12 @@ class ExecutorPodsAllocator(
// if they happen to come up before the deletion takes effect.
@volatile protected var deletedExecutorIds = Set.empty[Long]
+ @volatile private var failedExecutorIds = Set.empty[Long]
+
+ private[k8s] val failureTracker = new FailureTracker(conf, clock)
+
+ def getNumExecutorsFailed: Int = failureTracker.numFailedExecutors
Review Comment:
is this exposed for testing?
--
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
[GitHub] [spark] yaooqinn commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1152954378
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -520,10 +552,46 @@ class ExecutorPodsAllocator(
private[spark] object ExecutorPodsAllocator {
+ private[spark] val EXIT_MAX_EXECUTOR_FAILURES = 11
+
// A utility function to split the available slots among the specified consumers
def splitSlots[T](consumers: Seq[T], slots: Int): Seq[(T, Int)] = {
val d = slots / consumers.size
val r = slots % consumers.size
consumers.take(r).map((_, d + 1)) ++ consumers.takeRight(consumers.size - r).map((_, d))
}
}
+
+private[spark] class FailureTracker(
Review Comment:
Shall we refect and reuse the existing FailureTracker ?
--
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
[GitHub] [spark] yaooqinn commented on pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #38732:
URL: https://github.com/apache/spark/pull/38732#issuecomment-1490050958
Does Kubernetes support other mechanisms to add a timeout during pod/container/app initialization? If not, we shall bring this feature in at the spark layer. Also cc @Yikun
--
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
[GitHub] [spark] holdenk commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
holdenk commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1034165349
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -136,6 +151,10 @@ class ExecutorPodsAllocator(
def isDeleted(executorId: String): Boolean = deletedExecutorIds.contains(executorId.toLong)
+ private[k8s] def stopApplication(exitCode: Int): Unit = {
+ sys.exit(exitCode)
+ }
Review Comment:
I'm not super sure on this logic here, generally speaking we want to do a more controlled shutdown than just a `sys.exit`, could we call into the shutdownhook like in YARN?
Or is there a reason why were using sys.ext here?
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1027519725
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -723,6 +723,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Maximum number of pending pods should be a positive integer")
.createWithDefault(Int.MaxValue)
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
Review Comment:
The equivalent yarn configuration is `spark.yarn.max.executor.failures`, naming suggestion is welcome
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1027559033
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -119,6 +126,12 @@ class ExecutorPodsAllocator(
}
}
snapshotsStore.addSubscriber(podAllocationDelay) {
+ maxNumExecutorFailuresOpt.foreach { maxNumExecutorFailures =>
+ if (failureTracker.numFailedExecutors > maxNumExecutorFailures) {
+ logError(s"Max number of executor failures ($maxNumExecutorFailures) reached")
+ SparkContext.getActive.foreach(_.stop(EXIT_MAX_EXECUTOR_FAILURES))
Review Comment:
It's ugly, but `AbstractPodsAllocator` is a developer api, pass `sc` will change the constructor signature. Suggestions are welcome.
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1042382362
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -136,6 +151,10 @@ class ExecutorPodsAllocator(
def isDeleted(executorId: String): Boolean = deletedExecutorIds.contains(executorId.toLong)
+ private[k8s] def stopApplication(exitCode: Int): Unit = {
+ sys.exit(exitCode)
+ }
Review Comment:
Emm, I see a similar usage
https://github.com/apache/spark/blob/1d2159f888139e65c80db2003d521b0f684df83a/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsSnapshotsStoreImpl.scala#L145-L147
--
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
[GitHub] [spark] pan3793 commented on pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #38732:
URL: https://github.com/apache/spark/pull/38732#issuecomment-1322122028
cc @attilapiros @holdenk @Yikun @dongjoon-hyun would you please take a look?
--
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
[GitHub] [spark] pan3793 commented on pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #38732:
URL: https://github.com/apache/spark/pull/38732#issuecomment-1506569028
@yaooqinn please go https://github.com/apache/spark/pull/40774
--
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
[GitHub] [spark] yaooqinn commented on pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #38732:
URL: https://github.com/apache/spark/pull/38732#issuecomment-1506565811
hi, @pan3793. can you create a new PR, I'm unable to open it
--
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
[GitHub] [spark] github-actions[bot] closed pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
URL: https://github.com/apache/spark/pull/38732
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1152967630
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -750,6 +750,26 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Gracefully shutdown period must be a positive time value")
.createWithDefaultString("20s")
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
+ .doc("Spark exit if the number of failed executors exceeds this threshold. This " +
+ s"configuration only take effect when ${KUBERNETES_ALLOCATION_PODS_ALLOCATOR.key} " +
+ s"is set to 'direct'.")
+ .version("3.5.0")
+ .intConf
+ .checkValue(value => value > 0, "Max number of failures must be a positive value")
+ .createOptional
+
+ val KUBERNETES_EXECUTOR_ATTEMPT_FAILURE_VALIDITY_INTERVAL_MS =
+ ConfigBuilder("spark.kubernetes.executor.failuresValidityInterval")
Review Comment:
Currently, only YARN supports this feature, is it good to define a `spark.executor.failuresValidityInterval` and clarify the limitation on the conf description?
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1028083213
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -136,6 +151,10 @@ class ExecutorPodsAllocator(
def isDeleted(executorId: String): Boolean = deletedExecutorIds.contains(executorId.toLong)
+ private[k8s] def stopApplication(exitCode: Int): Unit = {
+ SparkContext.getActive.foreach(_.stop(exitCode))
Review Comment:
It's not elegant, but `AbstractPodsAllocator` is a developer api, introduce parameter sc will change the signature of constructor, suggestions are welcome here.
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1027519725
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -723,6 +723,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Maximum number of pending pods should be a positive integer")
.createWithDefault(Int.MaxValue)
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
Review Comment:
The equivalent yarn configuration is `spark.yarn.max.executor.failures`
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1027559033
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -119,6 +126,12 @@ class ExecutorPodsAllocator(
}
}
snapshotsStore.addSubscriber(podAllocationDelay) {
+ maxNumExecutorFailuresOpt.foreach { maxNumExecutorFailures =>
+ if (failureTracker.numFailedExecutors > maxNumExecutorFailures) {
+ logError(s"Max number of executor failures ($maxNumExecutorFailures) reached")
+ SparkContext.getActive.foreach(_.stop(EXIT_MAX_EXECUTOR_FAILURES))
Review Comment:
It's ugly, but `AbstractPodsAllocator` is a developer api, pass `sc` will change the constructor signature. Suggestions are welcome.
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1141433663
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -746,6 +746,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Gracefully shutdown period must be a positive time value")
.createWithDefaultString("20s")
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
+ .doc("Spark exit if the number of failed executors exceeds this threshold. This " +
+ s"configuration only take effect when ${KUBERNETES_ALLOCATION_PODS_ALLOCATOR.key} " +
+ s"is set to 'direct'.")
+ .version("3.4.0")
+ .intConf
Review Comment:
added `checkValue`
--
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
[GitHub] [spark] pan3793 commented on pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #38732:
URL: https://github.com/apache/spark/pull/38732#issuecomment-1475351438
@holdenk I addressed all review comments, will appreciate it you can take a look again.
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1141431687
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -253,6 +272,21 @@ class ExecutorPodsAllocator(
case _ => false
}
+ val currentFailedExecutorIds = podsForRpId.filter {
+ case (_, PodFailed(pod)) =>
+ pod.getSpec
Review Comment:
removed.
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1043152569
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -746,6 +746,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Gracefully shutdown period must be a positive time value")
.createWithDefaultString("20s")
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
+ .doc("Spark exit if the number of failed executors exceeds this threshold. This " +
+ s"configuration only take effect when ${KUBERNETES_ALLOCATION_PODS_ALLOCATOR.key} " +
+ s"is set to 'direct'.")
+ .version("3.4.0")
+ .intConf
+ .createOptional
+
+ val KUBERNETES_EXECUTOR_ATTEMPT_FAILURE_VALIDITY_INTERVAL_MS =
Review Comment:
The val name is a little long ...
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1028084187
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -494,10 +525,46 @@ class ExecutorPodsAllocator(
private[spark] object ExecutorPodsAllocator {
+ private[spark] val EXIT_MAX_EXECUTOR_FAILURES = 11
Review Comment:
borrowed from YARN module `ApplicationMaster.EXIT_MAX_EXECUTOR_FAILURES`
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1043114690
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -723,6 +723,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Maximum number of pending pods should be a positive integer")
.createWithDefault(Int.MaxValue)
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
Review Comment:
I thought that, but it does not match the current style of K8s configurations.
--
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
[GitHub] [spark] github-actions[bot] commented on pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38732:
URL: https://github.com/apache/spark/pull/38732#issuecomment-1475045060
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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
[GitHub] [spark] pan3793 commented on pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #38732:
URL: https://github.com/apache/spark/pull/38732#issuecomment-1506560201
@yaooqinn changed as requested, please help reopen this PR, the new change is visible then
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1038973719
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -136,6 +151,10 @@ class ExecutorPodsAllocator(
def isDeleted(executorId: String): Boolean = deletedExecutorIds.contains(executorId.toLong)
+ private[k8s] def stopApplication(exitCode: Int): Unit = {
+ sys.exit(exitCode)
+ }
Review Comment:
I see the problem, and actually I don't know how to handle it properly ...
I use `sys.exit` here just want to kill the process or driver pod, so that `ShutdownHookManager` will be invoked.
--
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
[GitHub] [spark] yaooqinn commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1153042664
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -750,6 +750,26 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Gracefully shutdown period must be a positive time value")
.createWithDefaultString("20s")
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
+ .doc("Spark exit if the number of failed executors exceeds this threshold. This " +
+ s"configuration only take effect when ${KUBERNETES_ALLOCATION_PODS_ALLOCATOR.key} " +
+ s"is set to 'direct'.")
+ .version("3.5.0")
+ .intConf
+ .checkValue(value => value > 0, "Max number of failures must be a positive value")
+ .createOptional
+
+ val KUBERNETES_EXECUTOR_ATTEMPT_FAILURE_VALIDITY_INTERVAL_MS =
+ ConfigBuilder("spark.kubernetes.executor.failuresValidityInterval")
Review Comment:
This kind of feature should be applied to all supported cluster managers. I did see many cluster-manager-specific logics here.
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1152964045
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -148,6 +163,10 @@ class ExecutorPodsAllocator(
def isDeleted(executorId: String): Boolean = deletedExecutorIds.contains(executorId.toLong)
+ private[k8s] def stopApplication(exitCode: Int): Unit = {
+ sys.exit(exitCode)
Review Comment:
we can override the function for testing
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38732:
URL: https://github.com/apache/spark/pull/38732#issuecomment-1325361859
Can one of the admins verify this patch?
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1043148280
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -723,6 +723,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Maximum number of pending pods should be a positive integer")
.createWithDefault(Int.MaxValue)
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
Review Comment:
After looking at the current naming style of kubernetes, I think `spark.kubernetes.executor.maxNumFailures` is ok
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1141431687
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -253,6 +272,21 @@ class ExecutorPodsAllocator(
case _ => false
}
+ val currentFailedExecutorIds = podsForRpId.filter {
+ case (_, PodFailed(pod)) =>
+ pod.getSpec
Review Comment:
simply return a property value, no side-effect.
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1141438872
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -746,6 +746,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Gracefully shutdown period must be a positive time value")
.createWithDefaultString("20s")
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
+ .doc("Spark exit if the number of failed executors exceeds this threshold. This " +
+ s"configuration only take effect when ${KUBERNETES_ALLOCATION_PODS_ALLOCATOR.key} " +
+ s"is set to 'direct'.")
+ .version("3.4.0")
+ .intConf
+ .createOptional
+
+ val KUBERNETES_EXECUTOR_ATTEMPT_FAILURE_VALIDITY_INTERVAL_MS =
Review Comment:
there were such long name variables in existing configurations
--
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
[GitHub] [spark] yaooqinn commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1152951801
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -494,10 +525,46 @@ class ExecutorPodsAllocator(
private[spark] object ExecutorPodsAllocator {
+ private[spark] val EXIT_MAX_EXECUTOR_FAILURES = 11
Review Comment:
shall we move it to SparkExitCode？
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1043218785
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -130,8 +139,14 @@ class ExecutorPodsAllocator(
.waitUntilReady(driverPodReadinessTimeout, TimeUnit.SECONDS)
}
}
- snapshotsStore.addSubscriber(podAllocationDelay) {
- onNewSnapshots(applicationId, schedulerBackend, _)
+ snapshotsStore.addSubscriber(podAllocationDelay) { executorPodsSnapshot =>
+ onNewSnapshots(applicationId, schedulerBackend, executorPodsSnapshot)
+ maxNumExecutorFailuresOpt.foreach { maxNumExecutorFailures =>
+ if (failureTracker.numFailedExecutors > maxNumExecutorFailures) {
+ logError(s"Max number of executor failures ($maxNumExecutorFailures) reached")
+ stopApplication(EXIT_MAX_EXECUTOR_FAILURES)
Review Comment:
~hmm.. why the exitCode is EXIT_MAX_EXECUTOR_FAILURES?~
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1043161748
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -746,6 +746,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Gracefully shutdown period must be a positive time value")
.createWithDefaultString("20s")
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
+ .doc("Spark exit if the number of failed executors exceeds this threshold. This " +
+ s"configuration only take effect when ${KUBERNETES_ALLOCATION_PODS_ALLOCATOR.key} " +
+ s"is set to 'direct'.")
+ .version("3.4.0")
+ .intConf
Review Comment:
Is there a place to check the effectiveness? What happens if it is configured as `-1`?
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1043218785
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -130,8 +139,14 @@ class ExecutorPodsAllocator(
.waitUntilReady(driverPodReadinessTimeout, TimeUnit.SECONDS)
}
}
- snapshotsStore.addSubscriber(podAllocationDelay) {
- onNewSnapshots(applicationId, schedulerBackend, _)
+ snapshotsStore.addSubscriber(podAllocationDelay) { executorPodsSnapshot =>
+ onNewSnapshots(applicationId, schedulerBackend, executorPodsSnapshot)
+ maxNumExecutorFailuresOpt.foreach { maxNumExecutorFailures =>
+ if (failureTracker.numFailedExecutors > maxNumExecutorFailures) {
+ logError(s"Max number of executor failures ($maxNumExecutorFailures) reached")
+ stopApplication(EXIT_MAX_EXECUTOR_FAILURES)
Review Comment:
hmm.. why the exitCode is EXIT_MAX_EXECUTOR_FAILURES?
--
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
[GitHub] [spark] yaooqinn commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1152957287
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -750,6 +750,26 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Gracefully shutdown period must be a positive time value")
.createWithDefaultString("20s")
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
+ .doc("Spark exit if the number of failed executors exceeds this threshold. This " +
+ s"configuration only take effect when ${KUBERNETES_ALLOCATION_PODS_ALLOCATOR.key} " +
+ s"is set to 'direct'.")
+ .version("3.5.0")
+ .intConf
+ .checkValue(value => value > 0, "Max number of failures must be a positive value")
+ .createOptional
+
+ val KUBERNETES_EXECUTOR_ATTEMPT_FAILURE_VALIDITY_INTERVAL_MS =
+ ConfigBuilder("spark.kubernetes.executor.failuresValidityInterval")
Review Comment:
Shall we use `spark.executor.failuresValidityInterval` and deprecates `spark.yarn.xxx`?
Like what we do for memoryOverhead and others?
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1043111597
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -723,6 +723,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Maximum number of pending pods should be a positive integer")
.createWithDefault(Int.MaxValue)
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
Review Comment:
How about `spark. kubernetes.max.executor.failures`? Keep same name pattern as `yarn`
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala:
##########
@@ -723,6 +723,25 @@ private[spark] object Config extends Logging {
.checkValue(value => value > 0, "Maximum number of pending pods should be a positive integer")
.createWithDefault(Int.MaxValue)
+ val KUBERNETES_MAX_EXECUTOR_FAILURES =
+ ConfigBuilder("spark.kubernetes.executor.maxNumFailures")
Review Comment:
How about `spark.kubernetes.max.executor.failures`? Keep same name pattern as `yarn`
--
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
[GitHub] [spark] LuciferYang commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1043224262
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -253,6 +272,21 @@ class ExecutorPodsAllocator(
case _ => false
}
+ val currentFailedExecutorIds = podsForRpId.filter {
+ case (_, PodFailed(pod)) =>
+ pod.getSpec
Review Comment:
What is the effect of calling `pod.getSpec` ？
--
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
[GitHub] [spark] github-actions[bot] closed pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
URL: https://github.com/apache/spark/pull/38732
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1141438598
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -136,6 +151,10 @@ class ExecutorPodsAllocator(
def isDeleted(executorId: String): Boolean = deletedExecutorIds.contains(executorId.toLong)
+ private[k8s] def stopApplication(exitCode: Int): Unit = {
+ sys.exit(exitCode)
+ }
Review Comment:
@holdenk sorry for so long late reply. I think the `sys.exit` should be fine here, because
1) According to docs of `Runtime#addShutdownHook`, `sys.exit` will trigger `ShutdownHookManager`,
> Registers a new virtual-machine shutdown hook.
The Java virtual machine shuts down in response to two kinds of events:
The program exits normally, when the last non-daemon thread exits or when the exit (equivalently, System.exit) method is invoked, or
The virtual machine is terminated in response to a user interrupt, such as typing ^C, or a system-wide event, such as user logoff or system shutdown.
2) I read the code of YARN `ApplicationMaster`, the extra logic consists of
- clean staging dir
- resubmit if `exitCode != 0` and max attempts is unreached, or call `YarnRMClient.unregister` to unregister application.
IMO we don't need to do 2) in K8s, simply call `sys.exit(exitCode)` to terminate Pod and propagate the exit code to Pod state should be fine.
--
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
[GitHub] [spark] pan3793 commented on a diff in pull request #38732: [SPARK-41210][K8S] Window based executor failure tracking mechanism
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #38732:
URL: https://github.com/apache/spark/pull/38732#discussion_r1141438598
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodsAllocator.scala:
##########
@@ -136,6 +151,10 @@ class ExecutorPodsAllocator(
def isDeleted(executorId: String): Boolean = deletedExecutorIds.contains(executorId.toLong)
+ private[k8s] def stopApplication(exitCode: Int): Unit = {
+ sys.exit(exitCode)
+ }
Review Comment:
@holdenk sorry for so long late reply. I think the `sys.exit` should be fine here, because
1) According to docs of `Runtime#addShutdownHook`, `sys.exit` will trigger `ShutdownHookManager`,
> Registers a new virtual-machine shutdown hook.
The Java virtual machine shuts down in response to two kinds of events:
The program exits normally, when the last non-daemon thread exits or when the exit (equivalently, System.exit) method is invoked, or
The virtual machine is terminated in response to a user interrupt, such as typing ^C, or a system-wide event, such as user logoff or system shutdown.
2) I read the code of YARN `ApplicationMaster`, the extra logic consists of
- clean staging dir
- resubmit if `exitCode != 0` and max attempts is unreached, or call `YarnRMClient.unregister` to unregister application.
IMO we don't need to do 2) in K8s, simply call `sys.exit(exitCode)` to propagate the exit code to Pod state should be fine.
--
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