You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2026/09/28 22:37:22 UTC
[PR] [SPARK-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
dongjoon-hyun opened a new pull request, #59115:
URL: https://github.com/apache/spark/pull/59115
### What changes were proposed in this pull request?
This PR aims to make `ExecutorResizePlugin` read `spark.kubernetes.executor.resizeThreshold` and `spark.kubernetes.executor.resizeFactor` via their `ConfigEntry`s, like `ExecutorPVCResizePlugin` does.
### Why are the changes needed?
Currently, the raw key reads bypass `checkValue`, so invalid values are silently accepted. For example, `resizeFactor=0` patches the same memory limit repeatedly, a negative `resizeFactor` shrinks it, and `resizeThreshold>=1` never triggers a resize.
### Does this PR introduce _any_ user-facing change?
Yes, but only for invalid configurations. They now fail at driver startup with the config's validation error instead of being silently accepted.
### How was this patch tested?
Pass the CIs with the newly added test cases.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5.5
--
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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #59115: [SPARK-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor`
URL: https://github.com/apache/spark/pull/59115
--
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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59115:
URL: https://github.com/apache/spark/pull/59115#issuecomment-5879991963
cc @peter-toth
--
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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #59115:
URL: https://github.com/apache/spark/pull/59115#discussion_r4129713899
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorResizePlugin.scala:
##########
@@ -65,8 +65,8 @@ class ExecutorResizeDriverPlugin extends DriverPlugin with Logging {
val interval = Utils.timeStringAsSeconds(
sc.conf.get(EXECUTOR_RESIZE_INTERVAL.key, "1m"))
Review Comment:
While we are here: `resizeInterval` is still read by raw key one line above, so it bypasses `EXECUTOR_RESIZE_INTERVAL`'s `checkValue(_ >= 0)`, and the hard-coded default `1m` contradicts the entry's default of `0` and its doc, "To disable, set 0 (default)". Setting `0` as that doc says makes `scheduleAtFixedRate(..., 0, 0, SECONDS)` throw out of `init`, so the driver fails to start. `ExecutorPVCResizePlugin` does `val interval = sc.conf.get(PVC_RESIZE_INTERVAL); if (interval <= 0) return` - the same shape here would make all three knobs consistent, which is what this PR sets out to do.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorResizePlugin.scala:
##########
@@ -65,8 +65,8 @@ class ExecutorResizeDriverPlugin extends DriverPlugin with Logging {
val interval = Utils.timeStringAsSeconds(
sc.conf.get(EXECUTOR_RESIZE_INTERVAL.key, "1m"))
- val threshold = sc.conf.getDouble(EXECUTOR_RESIZE_THRESHOLD.key, 0.9)
- val factor = sc.conf.getDouble(EXECUTOR_RESIZE_FACTOR.key, 0.1)
+ val threshold = sc.conf.get(EXECUTOR_RESIZE_THRESHOLD)
Review Comment:
With the entries enforced, values that 4.2.x and 4.3.x accepted (`resizeFactor=1.5`, say) become startup failures, but neither the `.doc` strings ("The threshold to resize.", "The factor to resize.") nor the table in `docs/running-on-kubernetes.md` names the accepted ranges. Could the ranges `(0, 1)` and `(0, 1]` go into both, so a user who hits `INVALID_CONF_VALUE.REQUIREMENT` after an upgrade finds them documented?
--
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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59115:
URL: https://github.com/apache/spark/pull/59115#discussion_r4129755390
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorResizePlugin.scala:
##########
@@ -65,8 +65,8 @@ class ExecutorResizeDriverPlugin extends DriverPlugin with Logging {
val interval = Utils.timeStringAsSeconds(
sc.conf.get(EXECUTOR_RESIZE_INTERVAL.key, "1m"))
Review Comment:
Yes, you are right. This is handled separately in #59114 (SPARK-59842). It reads `EXECUTOR_RESIZE_INTERVAL` via its `ConfigEntry` and returns early on `0` like `ExecutorPVCResizePlugin`. It also changes the entry's default and doc to `1m`, which has been the actual default since 4.2.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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59115:
URL: https://github.com/apache/spark/pull/59115#issuecomment-5883840488
Thank you, @MaxGekk !
--
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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59115:
URL: https://github.com/apache/spark/pull/59115#issuecomment-5883190124
Could you review this PR when you have some time, @MaxGekk ?
--
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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59115:
URL: https://github.com/apache/spark/pull/59115#issuecomment-5883920665
I addressed the second comment here [98d6c50](https://github.com/apache/spark/pull/59115/commits/98d6c50355e7b9ede3a9279bf684335c969ad9b6).
--
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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59115:
URL: https://github.com/apache/spark/pull/59115#issuecomment-5890288136
Thank you so much, @HyukjinKwon !
--
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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #59115:
URL: https://github.com/apache/spark/pull/59115#issuecomment-5890375622
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/bc1642ad9814fce87416caca3ee4abd554f0c9ca
- merged into branch-4.x https://github.com/apache/spark/commit/a46410f48106dda5d03b39081b190d7adf4f705c
- merged into branch-4.3 https://github.com/apache/spark/commit/3b4afd88ef5a81ba4066190e0da9e70f33d08122
- merged into branch-4.2 https://github.com/apache/spark/commit/c86557190befdb5bfba426c82b371ad81790301d
*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-59843][K8S] Make `ExecutorResizePlugin` validate `resizeThreshold` and `resizeFactor` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #59115:
URL: https://github.com/apache/spark/pull/59115#discussion_r4129757147
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorResizePlugin.scala:
##########
@@ -65,8 +65,8 @@ class ExecutorResizeDriverPlugin extends DriverPlugin with Logging {
val interval = Utils.timeStringAsSeconds(
sc.conf.get(EXECUTOR_RESIZE_INTERVAL.key, "1m"))
- val threshold = sc.conf.getDouble(EXECUTOR_RESIZE_THRESHOLD.key, 0.9)
- val factor = sc.conf.getDouble(EXECUTOR_RESIZE_FACTOR.key, 0.1)
+ val threshold = sc.conf.get(EXECUTOR_RESIZE_THRESHOLD)
Review Comment:
Sure!
--
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