dwsmith1983 commented on code in PR #6336:
URL: https://github.com/apache/datafusion-comet/pull/6336#discussion_r4380775424
##########
native/shuffle/src/writers/local/mod.rs:
##########
@@ -17,3 +17,18 @@
pub(crate) mod local_partition_writer;
mod spill;
+
+use datafusion::execution::memory_pool::MemoryReservation;
+
+const FALLBACK_BUFFER_SIZE: usize = 8 * 1024;
+
+/// Returns the capacity to allocate. A refused size falls back to a small
buffer charged with
+/// `grow`, so the task carries on and the reservation still matches what is
held.
+fn reserve_buffer(reservation: &MemoryReservation, size: usize) -> usize {
+ if reservation.try_grow(size).is_ok() {
Review Comment:
`reserve_buffer` and `restore_buffer` now fall back only when the root error
is `ResourcesExhausted`. Any other error, such as the `External` a Spark-backed
pool returns when a peer's spill throws, propagates from every buffer request
and from the post-spill hook. `write_burst_complete` returns a `Result`, and
`spill()` returns its error after freeing the reservation and recording the
spill metrics, with a write error taking precedence.
`buffer_pool_errors_other_than_refusal_fail_the_shuffle` runs the repartitioner
on a pool that throws for the buffer requests and expects the shuffle to fail
with nothing left reserved; `reserve_buffer_propagates_other_pool_errors`,
`restore_buffer_returns_errors_other_than_refusal` and
`spill_returns_burst_error_after_releasing_memory` cover the pieces.
I left the `resize` in `sync_buffer_reservation` as it is. Its grow goes
through `SparkMemory::acquire`, which on main turns a Java exception into a
zero grant plus overcommit debt instead of an error. Every later `try_grow`
asks Spark for that debt as well, so a repeated exception now surfaces from the
next buffer request, and whatever was booked is released when the repartitioner
drops. A charge booked from one swallowed exception can still sit there until
then. The repartitioner's own batch reservation also treats any `try_grow`
error as a refusal. Both paths are on main, so they are in #7244.
--
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]
