sarutak commented on code in PR #58582:
URL: https://github.com/apache/spark/pull/58582#discussion_r3954298354
##########
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:
Nit: this evaluates `usage > limit * threshold` twice (here and in the `else
if`). Nesting the cap check inside a single threshold block avoids the
duplication and makes the intent clearer.
```scala
if (usage > limit * threshold) {
if (limit >= maxMemory) {
if (cappedExecutors.add(execId)) {
logInfo(log"Skip resizing executor ${MDC(EXECUTOR_ID, execId)} as
container " +
log"memory limit ${MDC(MEMORY_SIZE, limit)} already reached the
maximum " +
log"${MDC(MAX_MEMORY_SIZE, maxMemory)}.")
}
} else {
val newLimit = math.min((limit * (1.0 + factor)).toLong, maxMemory)
// ... existing resize logic ...
}
}
```
##########
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:
`cappedExecutors` is only ever added to, so it grows monotonically over the
driver's lifetime for long-running apps. The footprint is tiny (tens of bytes
per executor) and cleaning it up would need an executor-removal hook, so I
don't think it belongs here. Just noting it in case it's worth a follow-up.
--
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]
