Raviston6296 commented on PR #60020:
URL: https://github.com/apache/spark/pull/60020#issuecomment-6656189207
Thanks for the fix. It matches the config doc ("the maximum number of
executor pods to try to create"), and it looks correct to me:
- It also fixes the overflow that SPARK-53907 was meant to prevent. With the
old order, the first rejected attempt at the default `Int.MaxValue` left the
counter at `Int.MaxValue`. The next attempt would then `incrementAndGet()` to
`Int.MinValue`, pass the `>= podAllocationMaximum` check, and create a pod with
a negative executor ID. Checking before incrementing means the counter never
goes past the limit. The `(Int.MaxValue, Int.MaxValue - 1)` grid case, with its
second round of allocation, covers exactly that.
- Checking first and incrementing after is safe here, because
`requestNewExecutors` only runs from `onNewSnapshots`, and
`SnapshotsSubscriber.processSnapshotsInternal` serializes those calls with
`lock.tryLock()`. `setTotalExpectedExecutors` only calls `notifySubscribers()`.
If you'd rather not depend on that, `EXECUTOR_ID_COUNTER.getAndUpdate(id => if
(id >= podAllocationMaximum) id else id + 1)` would make the check and the
increment one atomic step. That's optional.
Since `spark.kubernetes.allocation.maximum` shipped in 4.1.0, this
off-by-one is in 4.1.x, 4.2.x and 4.3.0 as well, so it may be worth backporting
to `branch-4.1` and later.
--
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]
