andygrove opened a new pull request, #5919: URL: https://github.com/apache/datafusion-comet/pull/5919
## Which issue does this PR close? N/A. This fixes a flaky Rust unit test seen in CI; no issue was filed. ## Rationale for this change `execution::operators::iceberg_write::tests::cancelling_abort_keeps_the_guard_armed` fails intermittently in the `rust-test` CI job with: ``` panicked at core/src/execution/operators/iceberg_write.rs:2937:13: expected the deletes to yield so the abort can be cancelled mid-flight ``` The test polls `AbortOnDrop::abort()` once with a no-op waker and asserts the result is `Pending`, so that dropping the future models a mid-flight cancellation. It used the filesystem `FileIO` because the memory backend completes every delete in a single poll. But the filesystem backend only *usually* yields: every delete is an opendal `stat` plus `remove_file`, each of which is a tokio `spawn_blocking` whose join handle is already ready if the blocking thread finishes before the first poll. The assertion therefore depends on winning eight races in a row. On Linux under CPU contention the freshly woken blocking thread tends to preempt the thread that spawned it, so all eight complete before they are polled and the abort future returns `Ready` on its first poll. On a saturated 32-core Ubuntu box the unmodified test failed 90 of 1000 runs. On macOS it never failed in 4000 runs, which is why it does not reproduce locally. CI runs `cargo nextest run` with the whole suite in parallel on ubuntu-24.04, so this matches the observed flakiness. ## What changes are included in this PR? Test-only changes in `native/core/src/execution/operators/iceberg_write.rs`: - Add `YieldBeforeDeleteStorageFactory` and `YieldBeforeDeleteStorage`, a test `StorageFactory` that wraps the opendal memory backend and calls `tokio::task::yield_now()` before every delete. Everything else delegates untouched. - Point `cancelling_abort_keeps_the_guard_armed` at that storage instead of a temp directory on the filesystem backend. The abort future is now guaranteed to be `Pending` on its first poll, so the mid-flight cancellation is constructible on every run rather than only when a real backend happens to yield. No production code changes. ## How are these changes tested? - The rewritten test passes 1000 of 1000 runs on the same saturated Linux box where the original failed 90 of 1000, and 300 of 300 runs on macOS. - The test still catches the regression it exists for: temporarily clearing `armed` before the await in `abort()` makes it fail on the ownership assertion. - `cargo fmt --check` and `cargo clippy -p datafusion-comet --all-targets -- -D warnings` are clean. -- 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]
