You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "sarutak (via GitHub)" <gi...@apache.org> on 2026/10/01 13:47:09 UTC
[PR] [SPARK-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
sarutak opened a new pull request, #59187:
URL: https://github.com/apache/spark/pull/59187
### What changes were proposed in this pull request?
This PR fixes the "Valid values" list for `spark.kubernetes.executor.rollPolicy`, which was out of sync with the `ExecutorRollPolicy` enum in both `Config.scala` and `running-on-kubernetes.md`.
- `Config.scala`: added the missing `TOTAL_SHUFFLE_WRITE` and `DISK_USED`.
- `running-on-kubernetes.md`: added the missing `AVERAGE_DURATION`, `PEAK_JVM_ONHEAP_MEMORY`, `PEAK_JVM_OFFHEAP_MEMORY`, `TOTAL_SHUFFLE_WRITE`, `DISK_USED`, `ACTIVE_TASKS` and `OUTLIER_NO_FALLBACK`, including their per-policy descriptions.
Both lists now follow the enum definition order.
### Why are the changes needed?
The documented list of valid values listed only a subset of the supported policies, which is misleading.
### Does this PR introduce _any_ user-facing change?
No. Docs only. No behavior change.
### How was this patch tested?
Verified by inspection that the lists in both files match the `ExecutorRollPolicy` enum.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Kiro CLI / Claude
--
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-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59187:
URL: https://github.com/apache/spark/pull/59187#issuecomment-5960544082
To @sarutak , we need new backporting PRs from `branch-4.3` because the following is Apache Spark 4.4 feature. If you want to backport, please make new PRs.
- #59126
--
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-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59187:
URL: https://github.com/apache/spark/pull/59187#issuecomment-5960422544
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/25a9aa7861642a834af8e045de6416626b12bb15
- merged into branch-4.x https://github.com/apache/spark/commit/39f1c3aaade77ac9cfb7fcce4fba0ccb2aa1fff4
*Posted by `merge_spark_pr.py`*
--
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-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59187:
URL: https://github.com/apache/spark/pull/59187#discussion_r4158564232
##########
docs/running-on-kubernetes.md:
##########
@@ -2110,14 +2112,22 @@ See the [configuration page](configuration.html) for information on Spark config
TOTAL_DURATION policy chooses an executor with the biggest total task time.
AVERAGE_DURATION policy chooses an executor with the biggest average task time.
FAILED_TASKS policy chooses an executor with the most number of failed tasks.
+ PEAK_JVM_ONHEAP_MEMORY policy chooses an executor with the biggest peak JVM on-heap memory.
+ PEAK_JVM_OFFHEAP_MEMORY policy chooses an executor with the biggest peak JVM off-heap memory.
+ TOTAL_SHUFFLE_WRITE policy chooses an executor with the biggest total shuffle write.
+ DISK_USED policy chooses an executor with the biggest used disk size.
ACTIVE_TASKS policy chooses an executor with the smallest number of active tasks.
If there is a tie, it chooses an executor with the smallest add-time.
It is recommended to use it with spark.kubernetes.executor.minTasksPerExecutorBeforeRolling
because newly started executors usually have no active tasks.
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.
+ total task time, total task GC time, the number of failed tasks,
+ peak JVM on-heap memory, peak JVM off-heap memory, total shuffle write,
+ and disk used if exists.
Review Comment:
**Nit (P3):** This lists eight dimensions, but `outliersFromMultipleDimensions` concatenates the per-dimension outlier lists in a fixed order (average task time > total task time > total task GC time > failed tasks > peak JVM on-heap > peak JVM off-heap > total shuffle write > disk used) and `choose` takes the first element. When different executors are outliers in different dimensions, the documented list alone does not tell the reader which one is rolled. Since the list is written in that same order, one sentence such as "The dimensions are checked in this order and the first outlier found is chosen." would make it explicit. The same applies to the `Config.scala` doc string.
##########
docs/running-on-kubernetes.md:
##########
@@ -2110,14 +2112,22 @@ See the [configuration page](configuration.html) for information on Spark config
TOTAL_DURATION policy chooses an executor with the biggest total task time.
AVERAGE_DURATION policy chooses an executor with the biggest average task time.
FAILED_TASKS policy chooses an executor with the most number of failed tasks.
+ PEAK_JVM_ONHEAP_MEMORY policy chooses an executor with the biggest peak JVM on-heap memory.
+ PEAK_JVM_OFFHEAP_MEMORY policy chooses an executor with the biggest peak JVM off-heap memory.
+ TOTAL_SHUFFLE_WRITE policy chooses an executor with the biggest total shuffle write.
+ DISK_USED policy chooses an executor with the biggest used disk size.
Review Comment:
**Nit (P3):** "the biggest used disk size" can be read as the executor's local disk usage (scratch space or shuffle files). The metric behind `DISK_USED` is `ExecutorSummary.diskUsed`, which `AppStatusListener` accumulates only from `SparkListenerBlockUpdated` events for RDD, stream and broadcast blocks (`updateExecutorMemoryDiskInfo`). Shuffle files are not counted. An executor that holds large shuffle data but no disk-persisted blocks therefore never ranks first under this policy, and the same applies to the new "disk used" dimension of `OUTLIER`.
Suggestion: "DISK_USED policy chooses an executor with the biggest disk size used by its stored blocks (e.g., disk-persisted RDD blocks)." The same sentence is in the `Config.scala` doc string (line 264), which is outside this diff's hunks, so please update both.
--
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-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59187:
URL: https://github.com/apache/spark/pull/59187#issuecomment-5947035971
Thank you, @sarutak .
--
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-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #59187: [SPARK-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy`
URL: https://github.com/apache/spark/pull/59187
--
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-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59187:
URL: https://github.com/apache/spark/pull/59187#issuecomment-5966818891
Thank you, @dongjoon-hyun and @HyukjinKwon !
> To @sarutak , we need new backporting PRs from `branch-4.3` because the following is Apache Spark 4.4 feature. If you want to backport, please make new PRs.
>
> * [[SPARK-59851][K8S] Add `ACTIVE_TASKS` executor roll policy #59126](https://github.com/apache/spark/pull/59126)
Got it. Will open a PR for `branch-4.3`.
--
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-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #59187:
URL: https://github.com/apache/spark/pull/59187#discussion_r4156355559
##########
docs/running-on-kubernetes.md:
##########
@@ -2118,6 +2124,8 @@ See the [configuration page](configuration.html) for information on Spark config
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.
+ OUTLIER_NO_FALLBACK policy picks an outlier using the OUTLIER policy above.
Review Comment:
**Nit (P3):** This entry defines `OUTLIER_NO_FALLBACK` by pointing to the OUTLIER description above. That description (here and in the `Config.scala` doc string) still lists only average task time, total task time, total task GC time, and failed tasks. `ExecutorRollPlugin.outliersFromMultipleDimensions` also checks peak JVM on-heap memory, peak JVM off-heap memory, total shuffle write, and disk used. As written, users would expect an executor that is an outlier only in, say, disk used never to be rolled under `OUTLIER_NO_FALLBACK`, but it will be. Since this PR is syncing these docs with the enum, could you extend the OUTLIER dimension list in both files to all eight metrics?
**Verification:**
- **Inspection:** The documented OUTLIER dimensions in both files match the outliers(...) calls in ExecutorRollPlugin.outliersFromMultipleDimensions.
- **Compile:** The kubernetes core module still compiles, and the Scala source lines stay within the 100-character limit.
--
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-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on code in PR #59187:
URL: https://github.com/apache/spark/pull/59187#discussion_r4157304052
##########
docs/running-on-kubernetes.md:
##########
@@ -2118,6 +2124,8 @@ See the [configuration page](configuration.html) for information on Spark config
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.
+ OUTLIER_NO_FALLBACK policy picks an outlier using the OUTLIER policy above.
Review Comment:
Thanks, makes sense. Extended the OUTLIER dimension list in both files to all eight metrics checked by `outliersFromMultipleDimensions`.
--
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-59924][K8s][DOCS] Fix stale "Valid values" list for `spark.kubernetes.executor.rollPolicy` [spark]
Posted by "sarutak (via GitHub)" <gi...@apache.org>.
sarutak commented on PR #59187:
URL: https://github.com/apache/spark/pull/59187#issuecomment-5946473217
Thanks @dongjoon-hyun. I've addressed your comments:
- Clarified that `DISK_USED` reflects the disk size used by stored blocks (RDD/stream/broadcast), not shuffle files, since that is what `ExecutorSummary.diskUsed` accumulates.
- Documented that the OUTLIER dimensions are checked in the listed priority order and the first outlier found is chosen.
Updated both `running-on-kubernetes.md` and the `Config.scala` doc string.
--
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