You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/01/31 20:38:49 UTC
[GitHub] [spark] alexholmes opened a new pull request #35373: SPARK-38047 - Provide an option to only roll executors if they are outliers
alexholmes opened a new pull request #35373:
URL: https://github.com/apache/spark/pull/35373
### What changes were proposed in this pull request?
This PR aims to add a new option to the executor rolling logic which allows users to skip rolling in cases where there are no outlier executors.
### Why are the changes needed?
As currently implemented an executor is always rolled every `spark.kubernetes.executor.rollInterval` interval. In environments where starting of executors can introduce latencies it may be desirable for users to have the option to determine if rolling should only happen when outliers are found.
### Does this PR introduce _any_ user-facing change?
No, this is an additional option being added to a new feature in Apache Spark 3.3.
### How was this patch tested?
Pass the CIs with the newly added test cases.
--
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] dongjoon-hyun closed pull request #35373: SPARK-38047 - Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #35373:
URL: https://github.com/apache/spark/pull/35373
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796058165
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -182,6 +182,16 @@ private[spark] object Config extends Logging {
.checkValue(_ >= 0, "The minimum number of tasks should be non-negative.")
.createWithDefault(0)
+ val EXECUTOR_ROLL_OUTLIERS_ONLY =
+ ConfigBuilder("spark.kubernetes.executor.onlyRollOutliers")
+ .doc("Only roll an executor if it is an outlier. An outlier is defined to be at least two " +
+ "standard deviation outside of the mean. If no outlier is found then no executor will " +
+ "be rolled."
+ )
Review comment:
Please place `)` at line 189.
--
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] dongjoon-hyun commented on pull request #35373: SPARK-38047 - Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35373:
URL: https://github.com/apache/spark/pull/35373#issuecomment-1026198695
It seems that your GitHub Action is not responding still for some reason.
- https://github.com/alexholmes/spark/actions/workflows/build_and_test.yml
--
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] alexholmes commented on pull request #35373: SPARK-38047 - Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
alexholmes commented on pull request #35373:
URL: https://github.com/apache/spark/pull/35373#issuecomment-1026197865
Thank you @dongjoon-hyun !
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796143993
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorRollPlugin.scala
##########
@@ -117,29 +117,36 @@ class ExecutorRollDriverPlugin extends DriverPlugin with Logging {
listWithoutDriver.sortBy(e => e.totalDuration.toFloat / Math.max(1, e.totalTasks)).reverse
case ExecutorRollPolicy.FAILED_TASKS =>
listWithoutDriver.sortBy(_.failedTasks).reverse
- case ExecutorRollPolicy.OUTLIER =>
- // We build multiple outlier lists and concat in the following importance order to find
- // outliers in various perspective:
- // AVERAGE_DURATION > TOTAL_DURATION > TOTAL_GC_TIME > FAILED_TASKS
- // Since we will choose only first item, the duplication is okay. If there is no outlier,
- // We fallback to TOTAL_DURATION policy.
- outliers(listWithoutDriver.filter(_.totalTasks > 0), e => e.totalDuration / e.totalTasks) ++
- outliers(listWithoutDriver, e => e.totalDuration) ++
- outliers(listWithoutDriver, e => e.totalGCTime) ++
- outliers(listWithoutDriver, e => e.failedTasks) ++
- listWithoutDriver.sortBy(_.totalDuration).reverse
+ case ExecutorRollPolicy.OUTLIER => outliers(listWithoutDriver)
+ case ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION =>
+ // If there is no outlier we fallback to TOTAL_DURATION policy.
+ outliers(listWithoutDriver) ++ listWithoutDriver.sortBy(_.totalDuration).reverse
}
sortedList.headOption.map(_.id)
}
+ /**
+ * We build multiple outlier lists and concat in the following importance order to find
+ * outliers in various perspective:
+ * AVERAGE_DURATION > TOTAL_DURATION > TOTAL_GC_TIME > FAILED_TASKS
+ * Since we will choose only first item, the duplication is okay.
+ */
+ private def outliers(listWithoutDriver: Seq[v1.ExecutorSummary]) =
+ outliersForFn(
+ listWithoutDriver.filter(_.totalTasks > 0),
+ e => e.totalDuration / e.totalTasks) ++
Review comment:
Please merge line 135 ~ 137 as a one liner if possible.
--
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] alexholmes commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
alexholmes commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796123613
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -182,6 +182,16 @@ private[spark] object Config extends Logging {
.checkValue(_ >= 0, "The minimum number of tasks should be non-negative.")
.createWithDefault(0)
+ val EXECUTOR_ROLL_OUTLIERS_ONLY =
+ ConfigBuilder("spark.kubernetes.executor.onlyRollOutliers")
+ .doc("Only roll an executor if it is an outlier. An outlier is defined to be at least two " +
+ "standard deviation outside of the mean. If no outlier is found then no executor will " +
+ "be rolled."
+ )
Review comment:
I removed the config so I'm going to resolve 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
[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796138698
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorRollPlugin.scala
##########
@@ -117,29 +117,36 @@ class ExecutorRollDriverPlugin extends DriverPlugin with Logging {
listWithoutDriver.sortBy(e => e.totalDuration.toFloat / Math.max(1, e.totalTasks)).reverse
case ExecutorRollPolicy.FAILED_TASKS =>
listWithoutDriver.sortBy(_.failedTasks).reverse
- case ExecutorRollPolicy.OUTLIER =>
- // We build multiple outlier lists and concat in the following importance order to find
- // outliers in various perspective:
- // AVERAGE_DURATION > TOTAL_DURATION > TOTAL_GC_TIME > FAILED_TASKS
- // Since we will choose only first item, the duplication is okay. If there is no outlier,
- // We fallback to TOTAL_DURATION policy.
- outliers(listWithoutDriver.filter(_.totalTasks > 0), e => e.totalDuration / e.totalTasks) ++
- outliers(listWithoutDriver, e => e.totalDuration) ++
- outliers(listWithoutDriver, e => e.totalGCTime) ++
- outliers(listWithoutDriver, e => e.failedTasks) ++
- listWithoutDriver.sortBy(_.totalDuration).reverse
+ case ExecutorRollPolicy.OUTLIER => outliers(listWithoutDriver)
Review comment:
If you don't mind, please put `outliers(listWithoutDriver)` at the next line.
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796141733
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorRollPlugin.scala
##########
@@ -117,29 +117,36 @@ class ExecutorRollDriverPlugin extends DriverPlugin with Logging {
listWithoutDriver.sortBy(e => e.totalDuration.toFloat / Math.max(1, e.totalTasks)).reverse
case ExecutorRollPolicy.FAILED_TASKS =>
listWithoutDriver.sortBy(_.failedTasks).reverse
- case ExecutorRollPolicy.OUTLIER =>
- // We build multiple outlier lists and concat in the following importance order to find
- // outliers in various perspective:
- // AVERAGE_DURATION > TOTAL_DURATION > TOTAL_GC_TIME > FAILED_TASKS
- // Since we will choose only first item, the duplication is okay. If there is no outlier,
- // We fallback to TOTAL_DURATION policy.
- outliers(listWithoutDriver.filter(_.totalTasks > 0), e => e.totalDuration / e.totalTasks) ++
- outliers(listWithoutDriver, e => e.totalDuration) ++
- outliers(listWithoutDriver, e => e.totalGCTime) ++
- outliers(listWithoutDriver, e => e.failedTasks) ++
- listWithoutDriver.sortBy(_.totalDuration).reverse
+ case ExecutorRollPolicy.OUTLIER => outliers(listWithoutDriver)
+ case ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION =>
+ // If there is no outlier we fallback to TOTAL_DURATION policy.
+ outliers(listWithoutDriver) ++ listWithoutDriver.sortBy(_.totalDuration).reverse
}
sortedList.headOption.map(_.id)
}
+ /**
+ * We build multiple outlier lists and concat in the following importance order to find
+ * outliers in various perspective:
+ * AVERAGE_DURATION > TOTAL_DURATION > TOTAL_GC_TIME > FAILED_TASKS
+ * Since we will choose only first item, the duplication is okay.
+ */
+ private def outliers(listWithoutDriver: Seq[v1.ExecutorSummary]) =
+ outliersForFn(
+ listWithoutDriver.filter(_.totalTasks > 0),
+ e => e.totalDuration / e.totalTasks) ++
+ outliersForFn(listWithoutDriver, e => e.totalDuration) ++
+ outliersForFn(listWithoutDriver, e => e.totalGCTime) ++
+ outliersForFn(listWithoutDriver, e => e.failedTasks)
+
/**
* Return executors whose metrics is outstanding, '(value - mean) > 2-sigma'. This is
* a best-effort approach because the snapshot of ExecutorSummary is not a normal distribution.
* Outliers can be defined in several ways (https://en.wikipedia.org/wiki/Outlier).
* Here, we borrowed 2-sigma idea from https://en.wikipedia.org/wiki/68-95-99.7_rule.
* In case of normal distribution, this is known to be 2.5 percent roughly.
*/
- private def outliers(
+ private def outliersForFn(
Review comment:
Let's keep the original method name because this is focusing on a single dimension outlier.
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796145098
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorRollPluginSuite.scala
##########
@@ -132,7 +132,9 @@ class ExecutorRollPluginSuite extends SparkFunSuite with PrivateMethodTester {
}
test("A one-item executor list") {
- ExecutorRollPolicy.values.foreach { value =>
+ ExecutorRollPolicy.values
+ .filter(_ != ExecutorRollPolicy.OUTLIER)
+ .foreach { value =>
Review comment:
The following would be shorter and reasonable.
```scala
ExecutorRollPolicy.values.filter(_ != ExecutorRollPolicy.OUTLIER).foreach { value =>
```
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796148262
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorRollPluginSuite.scala
##########
@@ -172,10 +174,53 @@ class ExecutorRollPluginSuite extends SparkFunSuite with PrivateMethodTester {
plugin.invokePrivate(_choose(list, ExecutorRollPolicy.AVERAGE_DURATION)))
}
- test("Policy: OUTLIER - Work like TOTAL_DURATION if there is no outlier") {
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Use TOTAL_DURATION if there is no outlier") {
assertEquals(
plugin.invokePrivate(_choose(list, ExecutorRollPolicy.TOTAL_DURATION)),
- plugin.invokePrivate(_choose(list, ExecutorRollPolicy.OUTLIER)))
+ plugin.invokePrivate(_choose(list, ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION)))
+ }
+
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Detect an average task duration outlier") {
+ val outlier = new ExecutorSummary("9999", "host:port", true, 1,
+ 0, 0, 1, 0, 0,
+ 3, 0, 1, 300,
+ 20, 0, 0,
+ 0, false, 0, new Date(1639300001000L),
+ Option.empty, Option.empty, Map(), Option.empty, Set(), Option.empty, Map(), Map(), 1,
+ false, Set())
+ assertEquals(
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.AVERAGE_DURATION)),
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION)))
+ }
+
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Detect a total task duration outlier") {
+ val outlier = new ExecutorSummary("9999", "host:port", true, 1,
+ 0, 0, 1, 0, 0,
+ 3, 0, 1000, 1000,
+ 0, 0, 0,
+ 0, false, 0, new Date(1639300001000L),
+ Option.empty, Option.empty, Map(), Option.empty, Set(), Option.empty, Map(), Map(), 1,
+ false, Set())
+ assertEquals(
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.TOTAL_DURATION)),
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION)))
+ }
+
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Detect a total GC time outlier") {
+ val outlier = new ExecutorSummary("9999", "host:port", true, 1,
+ 0, 0, 1, 0, 0,
+ 3, 0, 1, 100,
+ 1000, 0, 0,
+ 0, false, 0, new Date(1639300001000L),
+ Option.empty, Option.empty, Map(), Option.empty, Set(), Option.empty, Map(), Map(), 1,
+ false, Set())
+ assertEquals(
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.TOTAL_GC_TIME)),
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION)))
+ }
+
+ test("Policy: OUTLIER - Return None if not outliers") {
Review comment:
Please move this test case after `test("A one-item executor list")`.
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796143439
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -165,7 +166,9 @@ private[spark] object Config extends Logging {
"OUTLIER policy chooses an executor with outstanding statistics which is bigger than" +
"at least two standard deviation from the mean in average task time, " +
"total task time, total task GC time, and the number of failed tasks if exists. " +
- "If there is no outlier, it works like TOTAL_DURATION policy.")
+ "If there is no outlier then no executor will be rolled." +
Review comment:
We need a space at the end of the string. `rolled. "`
--
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] dongjoon-hyun closed pull request #35373: [SPARK-38047][K8S] Add `OUTLIER_NO_FALLBACK` executor roll policy
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #35373:
URL: https://github.com/apache/spark/pull/35373
--
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] dongjoon-hyun commented on pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35373:
URL: https://github.com/apache/spark/pull/35373#issuecomment-1026199988
FYI, I revised your PR title according to the community style, `[SPARK-38047][K8S]`. Each Apache community has different style unfortunately.
--
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] alexholmes commented on pull request #35373: [SPARK-38047][K8S] Provide a new executor roll policy called OUTLIER_NO_FALLBACK that only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
alexholmes commented on pull request #35373:
URL: https://github.com/apache/spark/pull/35373#issuecomment-1027124870
@dongjoon-hyun Thanks for your feedback. I've made the following changes:
1. Kept the behavior of OUTLIERS policy unchanged, along with its associated unit tests.
2. Introduced a new OUTLIER_NO_FALLBACK policy which doesn't fallback to TOTAL_DURATION in cases where there are no outliers
3. Updated the title and content of this PR to reflect these changes
4. Addressed your formatting comments above
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796143196
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -147,7 +147,8 @@ private[spark] object Config extends Logging {
.createWithDefault(0)
object ExecutorRollPolicy extends Enumeration {
- val ID, ADD_TIME, TOTAL_GC_TIME, TOTAL_DURATION, AVERAGE_DURATION, FAILED_TASKS, OUTLIER = Value
+ val ID, ADD_TIME, TOTAL_GC_TIME, TOTAL_DURATION, AVERAGE_DURATION, FAILED_TASKS,
+ OUTLIER, OUTLIER_OR_TOTAL_DURATION = Value
Review comment:
indentation?
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796141461
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorRollPlugin.scala
##########
@@ -117,29 +117,36 @@ class ExecutorRollDriverPlugin extends DriverPlugin with Logging {
listWithoutDriver.sortBy(e => e.totalDuration.toFloat / Math.max(1, e.totalTasks)).reverse
case ExecutorRollPolicy.FAILED_TASKS =>
listWithoutDriver.sortBy(_.failedTasks).reverse
- case ExecutorRollPolicy.OUTLIER =>
- // We build multiple outlier lists and concat in the following importance order to find
- // outliers in various perspective:
- // AVERAGE_DURATION > TOTAL_DURATION > TOTAL_GC_TIME > FAILED_TASKS
- // Since we will choose only first item, the duplication is okay. If there is no outlier,
- // We fallback to TOTAL_DURATION policy.
- outliers(listWithoutDriver.filter(_.totalTasks > 0), e => e.totalDuration / e.totalTasks) ++
- outliers(listWithoutDriver, e => e.totalDuration) ++
- outliers(listWithoutDriver, e => e.totalGCTime) ++
- outliers(listWithoutDriver, e => e.failedTasks) ++
- listWithoutDriver.sortBy(_.totalDuration).reverse
+ case ExecutorRollPolicy.OUTLIER => outliers(listWithoutDriver)
+ case ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION =>
+ // If there is no outlier we fallback to TOTAL_DURATION policy.
+ outliers(listWithoutDriver) ++ listWithoutDriver.sortBy(_.totalDuration).reverse
}
sortedList.headOption.map(_.id)
}
+ /**
+ * We build multiple outlier lists and concat in the following importance order to find
+ * outliers in various perspective:
+ * AVERAGE_DURATION > TOTAL_DURATION > TOTAL_GC_TIME > FAILED_TASKS
+ * Since we will choose only first item, the duplication is okay.
+ */
+ private def outliers(listWithoutDriver: Seq[v1.ExecutorSummary]) =
Review comment:
`outliers` -> `outliersFromMultipleDimentions`?
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796139138
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorRollPlugin.scala
##########
@@ -117,29 +117,36 @@ class ExecutorRollDriverPlugin extends DriverPlugin with Logging {
listWithoutDriver.sortBy(e => e.totalDuration.toFloat / Math.max(1, e.totalTasks)).reverse
case ExecutorRollPolicy.FAILED_TASKS =>
listWithoutDriver.sortBy(_.failedTasks).reverse
- case ExecutorRollPolicy.OUTLIER =>
- // We build multiple outlier lists and concat in the following importance order to find
- // outliers in various perspective:
- // AVERAGE_DURATION > TOTAL_DURATION > TOTAL_GC_TIME > FAILED_TASKS
- // Since we will choose only first item, the duplication is okay. If there is no outlier,
- // We fallback to TOTAL_DURATION policy.
- outliers(listWithoutDriver.filter(_.totalTasks > 0), e => e.totalDuration / e.totalTasks) ++
- outliers(listWithoutDriver, e => e.totalDuration) ++
- outliers(listWithoutDriver, e => e.totalGCTime) ++
- outliers(listWithoutDriver, e => e.failedTasks) ++
- listWithoutDriver.sortBy(_.totalDuration).reverse
+ case ExecutorRollPolicy.OUTLIER => outliers(listWithoutDriver)
+ case ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION =>
+ // If there is no outlier we fallback to TOTAL_DURATION policy.
+ outliers(listWithoutDriver) ++ listWithoutDriver.sortBy(_.totalDuration).reverse
}
sortedList.headOption.map(_.id)
}
+ /**
+ * We build multiple outlier lists and concat in the following importance order to find
+ * outliers in various perspective:
+ * AVERAGE_DURATION > TOTAL_DURATION > TOTAL_GC_TIME > FAILED_TASKS
Review comment:
Could you add two more leading space 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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796138290
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -165,7 +166,9 @@ private[spark] object Config extends Logging {
"OUTLIER policy chooses an executor with outstanding statistics which is bigger than" +
"at least two standard deviation from the mean in average task time, " +
"total task time, total task GC time, and the number of failed tasks if exists. " +
- "If there is no outlier, it works like TOTAL_DURATION policy.")
+ "If there is no outlier then no executor will be rolled." +
+ "OUTLIER_OR_TOTAL_DURATION policy picks an outlier using the OUTLIER policy above. " +
Review comment:
Could you keep the previous behavior by making `OUTLIER_OR_TOTAL_DURATION` as the default?
```scala
- .createWithDefault(ExecutorRollPolicy.OUTLIER.toString)
+ .createWithDefault(ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION.toString)
```
--
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796147629
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorRollPluginSuite.scala
##########
@@ -172,10 +174,53 @@ class ExecutorRollPluginSuite extends SparkFunSuite with PrivateMethodTester {
plugin.invokePrivate(_choose(list, ExecutorRollPolicy.AVERAGE_DURATION)))
}
- test("Policy: OUTLIER - Work like TOTAL_DURATION if there is no outlier") {
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Use TOTAL_DURATION if there is no outlier") {
assertEquals(
plugin.invokePrivate(_choose(list, ExecutorRollPolicy.TOTAL_DURATION)),
- plugin.invokePrivate(_choose(list, ExecutorRollPolicy.OUTLIER)))
+ plugin.invokePrivate(_choose(list, ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION)))
+ }
+
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Detect an average task duration outlier") {
+ val outlier = new ExecutorSummary("9999", "host:port", true, 1,
+ 0, 0, 1, 0, 0,
+ 3, 0, 1, 300,
+ 20, 0, 0,
+ 0, false, 0, new Date(1639300001000L),
+ Option.empty, Option.empty, Map(), Option.empty, Set(), Option.empty, Map(), Map(), 1,
+ false, Set())
+ assertEquals(
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.AVERAGE_DURATION)),
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION)))
+ }
+
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Detect a total task duration outlier") {
+ val outlier = new ExecutorSummary("9999", "host:port", true, 1,
+ 0, 0, 1, 0, 0,
+ 3, 0, 1000, 1000,
+ 0, 0, 0,
+ 0, false, 0, new Date(1639300001000L),
+ Option.empty, Option.empty, Map(), Option.empty, Set(), Option.empty, Map(), Map(), 1,
+ false, Set())
+ assertEquals(
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.TOTAL_DURATION)),
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION)))
+ }
+
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Detect a total GC time outlier") {
+ val outlier = new ExecutorSummary("9999", "host:port", true, 1,
+ 0, 0, 1, 0, 0,
+ 3, 0, 1, 100,
+ 1000, 0, 0,
+ 0, false, 0, new Date(1639300001000L),
+ Option.empty, Option.empty, Map(), Option.empty, Set(), Option.empty, Map(), Map(), 1,
+ false, Set())
+ assertEquals(
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.TOTAL_GC_TIME)),
+ plugin.invokePrivate(_choose(list :+ outlier, ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION)))
+ }
+
+ test("Policy: OUTLIER - Return None if not outliers") {
Review comment:
`not outliers` -> `there is no outlier`.
--
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] dongjoon-hyun commented on pull request #35373: SPARK-38047 - Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35373:
URL: https://github.com/apache/spark/pull/35373#issuecomment-1026197754
Let me close and reopen this to trigger your GitHub Action properly.
--
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] dongjoon-hyun commented on pull request #35373: SPARK-38047 - Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35373:
URL: https://github.com/apache/spark/pull/35373#issuecomment-1026198995
Could you do force-push this on your branch?
--
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 #35373: SPARK-38047 - Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35373:
URL: https://github.com/apache/spark/pull/35373#issuecomment-1026195020
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] dongjoon-hyun commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796146992
##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorRollPluginSuite.scala
##########
@@ -172,10 +174,53 @@ class ExecutorRollPluginSuite extends SparkFunSuite with PrivateMethodTester {
plugin.invokePrivate(_choose(list, ExecutorRollPolicy.AVERAGE_DURATION)))
}
- test("Policy: OUTLIER - Work like TOTAL_DURATION if there is no outlier") {
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Use TOTAL_DURATION if there is no outlier") {
assertEquals(
plugin.invokePrivate(_choose(list, ExecutorRollPolicy.TOTAL_DURATION)),
- plugin.invokePrivate(_choose(list, ExecutorRollPolicy.OUTLIER)))
+ plugin.invokePrivate(_choose(list, ExecutorRollPolicy.OUTLIER_OR_TOTAL_DURATION)))
+ }
+
+ test("Policy: OUTLIER_OR_TOTAL_DURATION - Detect an average task duration outlier") {
Review comment:
Actually, this is orthogonal to your proposal. We usually put one theme in a PR and avoid mixing different things.
Could you make a separate PR for adding more test coverage for `OUTLIER_OR_TOTAL_DURATION`?
--
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] alexholmes commented on a change in pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
alexholmes commented on a change in pull request #35373:
URL: https://github.com/apache/spark/pull/35373#discussion_r796123613
##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -182,6 +182,16 @@ private[spark] object Config extends Logging {
.checkValue(_ >= 0, "The minimum number of tasks should be non-negative.")
.createWithDefault(0)
+ val EXECUTOR_ROLL_OUTLIERS_ONLY =
+ ConfigBuilder("spark.kubernetes.executor.onlyRollOutliers")
+ .doc("Only roll an executor if it is an outlier. An outlier is defined to be at least two " +
+ "standard deviation outside of the mean. If no outlier is found then no executor will " +
+ "be rolled."
+ )
Review comment:
I removed the config so this should no longer apply
--
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] alexholmes commented on pull request #35373: [SPARK-38047][K8S] Provide an option to only roll executors if they are outliers
Posted by GitBox <gi...@apache.org>.
alexholmes commented on pull request #35373:
URL: https://github.com/apache/spark/pull/35373#issuecomment-1026284965
@dongjoon-hyun I've updated the PR with the following changes:
1. Introduced a new policy, OUTLIER_OR_TOTAL_DURATION, which provides a fallback of TOTAL_DURATION policy for outliers (basically the prior implementation of OUTLIER).
2. Updated the OUTLIER policy to not provide a fallback, therefore allowing no executors to be rolled when there are no outliners
--
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