You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "tianhanhu (via GitHub)" <gi...@apache.org> on 2024/08/16 21:31:50 UTC
[PR] [SPARK-49252] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
tianhanhu opened a new pull request, #47793:
URL: https://github.com/apache/spark/pull/47793
<!--
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
'common/utils/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.
-->
Make the change such that `TaskSetExcludeList` and `HeathTracker` can be enabled independently.
### 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.
-->
Currently, when `spark.excludeOnFailure.enabled` is set to true, both task set level exclusion (`TaskSetExcludeList`) and application level (`HealthTracker`) would both be enabled.
In some cases, we only want to enable exclusion on a single dimension.
### 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'.
-->
Yes, introduced two new user facing configs `spark.excludeOnFailure.application.enabled` and `spark.excludeOnFailure.taskAndStage.enabled` that allows setting exclusion for taskset/application individually.
### 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 unit tests.
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "tianhanhu (via GitHub)" <gi...@apache.org>.
tianhanhu commented on code in PR #47793:
URL: https://github.com/apache/spark/pull/47793#discussion_r1735554271
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala:
##########
@@ -143,8 +143,18 @@ private[spark] class TaskSetManager(
private var calculatedTasks = 0
private[scheduler] val taskSetExcludelistHelperOpt: Option[TaskSetExcludelist] = {
- healthTracker.map { _ =>
- new TaskSetExcludelist(sched.sc.listenerBus, conf, stageId, taskSet.stageAttemptId, clock)
+ if (TaskSetExcludelist.isExcludeOnFailureEnabled(conf)) {
+ Some(new TaskSetExcludelist(sched.sc.listenerBus, conf, stageId,
+ taskSet.stageAttemptId, clock))
+ } else if (healthTracker.isDefined) {
+ // If we enabled exclusion at application level but not at taskset level exclusion, we create
+ // TaskSetExcludelist in dry run mode.
Review Comment:
I would think having an explicit conf for "whether creating TaskSetExcludelist in dry-run mode" would be overkilling, especially since we already have dozens of configs for exclusion feature.
The whole purpose of having a dry run mode for `TaskSetExcludeList` here is that in a case where only healthtracker is enabled and TaskSetExcludeList is not, we can still have a way to collect data for application level exclusion.
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #47793:
URL: https://github.com/apache/spark/pull/47793#discussion_r1721099466
##########
core/src/main/scala/org/apache/spark/internal/config/package.scala:
##########
@@ -918,6 +918,18 @@ package object config {
.booleanConf
.createOptional
+ private[spark] val EXCLUDE_ON_FAILURE_ENABLED_APPLICATION =
+ ConfigBuilder("spark.excludeOnFailure.application.enabled")
+ .version("3.5.3")
+ .booleanConf
+ .createOptional
+
+ private[spark] val EXCLUDE_ON_FAILURE_ENABLED_TASK_AND_STAGE =
+ ConfigBuilder("spark.excludeOnFailure.taskAndStage.enabled")
+ .version("3.5.3")
Review Comment:
New confs should better be added to 4.0.0
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on code in PR #47793:
URL: https://github.com/apache/spark/pull/47793#discussion_r1737651766
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala:
##########
@@ -143,8 +143,18 @@ private[spark] class TaskSetManager(
private var calculatedTasks = 0
private[scheduler] val taskSetExcludelistHelperOpt: Option[TaskSetExcludelist] = {
- healthTracker.map { _ =>
- new TaskSetExcludelist(sched.sc.listenerBus, conf, stageId, taskSet.stageAttemptId, clock)
+ if (TaskSetExcludelist.isExcludeOnFailureEnabled(conf)) {
+ Some(new TaskSetExcludelist(sched.sc.listenerBus, conf, stageId,
+ taskSet.stageAttemptId, clock))
+ } else if (healthTracker.isDefined) {
+ // If we enabled exclusion at application level but not at taskset level exclusion, we create
+ // TaskSetExcludelist in dry run mode.
Review Comment:
SGTM
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on code in PR #47793:
URL: https://github.com/apache/spark/pull/47793#discussion_r1735495020
##########
core/src/main/scala/org/apache/spark/scheduler/HealthTracker.scala:
##########
@@ -425,14 +425,16 @@ private[spark] object HealthTracker extends Logging {
private val DEFAULT_TIMEOUT = "1h"
/**
- * Returns true if the excludeOnFailure is enabled, based on checking the configuration
- * in the following order:
- * 1. Is it specifically enabled or disabled?
+ * Returns true if the excludeOnFailure is enabled on the application level,
+ * based on checking the configuration in the following order:
+ * 1. Is application level exclusion specifically enabled or disabled?
+ * 2. Is overall exclusion feature enabled or disabled?
* 2. Is it enabled via the legacy timeout conf?
* 3. Default is off
Review Comment:
```suggestion
* 3. Is it enabled via the legacy timeout conf?
* 4. Default is off
```
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala:
##########
@@ -143,8 +143,18 @@ private[spark] class TaskSetManager(
private var calculatedTasks = 0
private[scheduler] val taskSetExcludelistHelperOpt: Option[TaskSetExcludelist] = {
- healthTracker.map { _ =>
- new TaskSetExcludelist(sched.sc.listenerBus, conf, stageId, taskSet.stageAttemptId, clock)
+ if (TaskSetExcludelist.isExcludeOnFailureEnabled(conf)) {
+ Some(new TaskSetExcludelist(sched.sc.listenerBus, conf, stageId,
+ taskSet.stageAttemptId, clock))
+ } else if (healthTracker.isDefined) {
+ // If we enabled exclusion at application level but not at taskset level exclusion, we create
+ // TaskSetExcludelist in dry run mode.
Review Comment:
This seems to introduce the behaviour change. Before this PR, `TaskSetExcludelist` is created in non-dry-run mode when `EXCLUDE_ON_FAILURE_ENABLED` is on. After this PR, the non-dry-run mode creation requires another conf `EXCLUDE_ON_FAILURE_ENABLED_TASK_AND_STAGE` on implicitly. I think adding an explicit conf for "_whether creating TaskSetExcludelist in dry-run mode_" maybe a better solution.
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 closed pull request #47793: [SPARK-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent
URL: https://github.com/apache/spark/pull/47793
--
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-49252] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "tianhanhu (via GitHub)" <gi...@apache.org>.
tianhanhu commented on PR #47793:
URL: https://github.com/apache/spark/pull/47793#issuecomment-2294341165
@cloud-fan @jiangxb1987 can I get a review on this PR? Thx!
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on PR #47793:
URL: https://github.com/apache/spark/pull/47793#issuecomment-2319795533
Thanks, merged to master!
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "tianhanhu (via GitHub)" <gi...@apache.org>.
tianhanhu commented on PR #47793:
URL: https://github.com/apache/spark/pull/47793#issuecomment-2311339942
> It should be good to mention the dryrun mode introduced in this PR.
Done updating 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
Re: [PR] [SPARK-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #47793:
URL: https://github.com/apache/spark/pull/47793#issuecomment-2304766367
cc @Ngone51
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "tianhanhu-db (via GitHub)" <gi...@apache.org>.
tianhanhu-db commented on code in PR #47793:
URL: https://github.com/apache/spark/pull/47793#discussion_r1731969762
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSetExcludeList.scala:
##########
@@ -163,3 +167,18 @@ private[scheduler] class TaskSetExcludelist(
}
}
}
+
+private[scheduler] object TaskSetExcludelist {
+
+ /**
+ * Returns true if the excludeOnFailure is enabled on the task/stage level,
+ * based on checking the configuration in the following order:
+ * 1. Is taskset level exclusion specifically enabled or disabled?
+ * 2. Is overall exclusion feature enabled or disabled?
+ * 3. Default is off
+ */
+ def isExcludeOnFailureEnabled(conf: SparkConf): Boolean = {
Review Comment:
My thought is to have the taskset/application level configs as the new standard knobs, and treat `spark.excludeOnFailure.enabled` as a legacy fallback for backward compatibilities.
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "tianhanhu-db (via GitHub)" <gi...@apache.org>.
tianhanhu-db commented on code in PR #47793:
URL: https://github.com/apache/spark/pull/47793#discussion_r1727249577
##########
core/src/main/scala/org/apache/spark/internal/config/package.scala:
##########
@@ -918,6 +918,18 @@ package object config {
.booleanConf
.createOptional
+ private[spark] val EXCLUDE_ON_FAILURE_ENABLED_APPLICATION =
+ ConfigBuilder("spark.excludeOnFailure.application.enabled")
+ .version("3.5.3")
+ .booleanConf
+ .createOptional
+
+ private[spark] val EXCLUDE_ON_FAILURE_ENABLED_TASK_AND_STAGE =
+ ConfigBuilder("spark.excludeOnFailure.taskAndStage.enabled")
+ .version("3.5.3")
Review Comment:
Done
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "jiangxb1987 (via GitHub)" <gi...@apache.org>.
jiangxb1987 commented on PR #47793:
URL: https://github.com/apache/spark/pull/47793#issuecomment-2311046644
It should be good to mention the dryrun mode introduced in this PR.
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "jiangxb1987 (via GitHub)" <gi...@apache.org>.
jiangxb1987 commented on code in PR #47793:
URL: https://github.com/apache/spark/pull/47793#discussion_r1731791395
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSetExcludeList.scala:
##########
@@ -163,3 +167,18 @@ private[scheduler] class TaskSetExcludelist(
}
}
}
+
+private[scheduler] object TaskSetExcludelist {
+
+ /**
+ * Returns true if the excludeOnFailure is enabled on the task/stage level,
+ * based on checking the configuration in the following order:
+ * 1. Is taskset level exclusion specifically enabled or disabled?
+ * 2. Is overall exclusion feature enabled or disabled?
+ * 3. Default is off
+ */
+ def isExcludeOnFailureEnabled(conf: SparkConf): Boolean = {
Review Comment:
Shouldn't we check the overall exclusion feature first?
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "jiangxb1987 (via GitHub)" <gi...@apache.org>.
jiangxb1987 commented on PR #47793:
URL: https://github.com/apache/spark/pull/47793#issuecomment-2315951026
cc @jerryshao @mridulm @Ngone51
--
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-49252][CORE] Make`TaskSetExcludeList` and `HeathTracker` independent [spark]
Posted by "tianhanhu (via GitHub)" <gi...@apache.org>.
tianhanhu commented on code in PR #47793:
URL: https://github.com/apache/spark/pull/47793#discussion_r1735528233
##########
core/src/main/scala/org/apache/spark/scheduler/TaskSetManager.scala:
##########
@@ -143,8 +143,18 @@ private[spark] class TaskSetManager(
private var calculatedTasks = 0
private[scheduler] val taskSetExcludelistHelperOpt: Option[TaskSetExcludelist] = {
- healthTracker.map { _ =>
- new TaskSetExcludelist(sched.sc.listenerBus, conf, stageId, taskSet.stageAttemptId, clock)
+ if (TaskSetExcludelist.isExcludeOnFailureEnabled(conf)) {
+ Some(new TaskSetExcludelist(sched.sc.listenerBus, conf, stageId,
+ taskSet.stageAttemptId, clock))
+ } else if (healthTracker.isDefined) {
+ // If we enabled exclusion at application level but not at taskset level exclusion, we create
+ // TaskSetExcludelist in dry run mode.
Review Comment:
This would not introduce a behavior change.
`EXCLUDE_ON_FAILURE_ENABLED_TASK_AND_STAGE` is an optional config and would default back to current behavior if not set. So for existing users they can be unaware of this change, and setting `EXCLUDE_ON_FAILURE_ENABLED` would bring both `HealthTracker` and `TaskSetExcludeList` on.
--
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