dongjoon-hyun commented on code in PR #58582:
URL: https://github.com/apache/spark/pull/58582#discussion_r3954365792
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorResizePlugin.scala:
##########
@@ -124,8 +129,14 @@ class ExecutorResizeDriverPlugin extends DriverPlugin with
Logging {
c.getResources.getLimits.containsKey("memory")).foreach { c =>
val limit =
Quantity.getAmountInBytes(c.getResources.getLimits.get("memory"))
.longValue()
- if (usage > limit * threshold) {
- val newLimit = (limit * (1.0 + factor)).toLong
+ if (usage > limit * threshold && limit >= maxMemory) {
Review Comment:
Thank you for the suggestion. I nested the cap check inside the single
threshold block as you proposed.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorResizePlugin.scala:
##########
@@ -124,8 +129,14 @@ class ExecutorResizeDriverPlugin extends DriverPlugin with
Logging {
c.getResources.getLimits.containsKey("memory")).foreach { c =>
val limit =
Quantity.getAmountInBytes(c.getResources.getLimits.get("memory"))
.longValue()
- if (usage > limit * threshold) {
- val newLimit = (limit * (1.0 + factor)).toLong
+ if (usage > limit * threshold && limit >= maxMemory) {
+ if (cappedExecutors.add(execId)) {
Review Comment:
Good point. Since the plugin already lists executor pods every interval, I
added a `retainAll` on that list so entries for removed executors are dropped
each round, without needing an executor-removal hook. The skip test now also
covers this cleanup.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
