andygrove commented on PR #7096: URL: https://github.com/apache/datafusion-comet/pull/7096#issuecomment-6319895582
Thanks @dwsmith1983 and @viirya. The review feedback is in 6a1c4dffca. For dwsmith1983's points: - **A sort refused while another consumer holds the share.** I added `a_consumer_that_can_spill_is_refused_once_the_others_hold_the_tasks_share`, which pins the shape you describe, and named it in the migration note, the tuning guide and the contributor guide. I did not let a consumer that holds nothing fall through to Spark, because that changes behavior the benchmarks did not measure. I am happy to add it if you or viirya think it should go in here. - **The executor-wide count.** `create_memory_pool` now goes through `create_executor_pool`, and `the_tasks_of_an_executor_share_one_count` fails if that passes a fresh `RunningTasks`. - **The task total against `pool_size`.** `the_tasks_total_is_checked_against_the_tasks_share_not_the_pool` follows your sketch and fails if the check uses `pool_size`. - **The churn test.** It now shares one count across the eight threads and asserts it is 0 afterwards. I confirmed it fails when the drop skips its `fetch_sub`. - **The off-heap storage region.** It is in the guide next to the two task-count differences. - **The review skill.** It now says `getMemoryConfig` also passes `off_heap_size`. I checked the new tests by breaking the line each one guards. `cargo test` for `iceberg_write`, `delta_dv` and `memory_pools` passes, clippy is clean, and `CometIcebergWriteActionSuite` passes 111 of 111 on Spark 4.1. -- 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]
