peterxcli commented on PR #5493: URL: https://github.com/apache/datafusion-comet/pull/5493#issuecomment-5444805259
Addressed the [bypass orphan-pages finding](https://github.com/apache/datafusion-comet/pull/5493#pullrequestreview-5044733895) in a1a4a84ef. You're right that `stop(false)` is skipped for fatal errors: both `write()` and Spark's `ShuffleWriteProcessor` catch only `Exception`, and `SparkOutOfMemoryError` is an `Error`. `CometBypassMergeSortShuffleWriter.write()` now catches `Throwable` and frees all partition writers' buffered pages before rethrowing, so a fatal failure (including the all-waiter fail-fast in the blocking allocator) can no longer orphan pool pages that an unrelated waiter would then wait on indefinitely. `freeMemory()` is idempotent, so the `stop(false)` that still runs on Exception paths is unaffected, and `CometUnsafeShuffleWriter` already handles generic throwables via its `success`/`finally` pattern (`sorter.cleanupResources()` runs in `finally`), so the bypass writer was the remaining gap. Added a regression test that drives the real `write()` (real `BlockManag er` from a local `SparkContext`, stub `ShuffleExecutorComponents`): it buffers three pages of the 1-MiB pool, fails mid-write with a `SparkOutOfMemoryError`, and then asserts a full-pool 1,048,576-byte allocation succeeds — impossible if any buffered page leaked. -- 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]
