sunchao opened a new pull request, #5847: URL: https://github.com/apache/datafusion-comet/pull/5847
## Which issue does this PR close? Addresses the fair-pool accounting portion of #5212. Related to #5466 and #5613; this draft needs merge-order coordination with both before it is ready. ## Rationale for this change `CometFairMemoryPool` currently compares total pool usage against one consumer's fair share. A 32-byte pool with two consumers holding 10 and 6 bytes rejects the second consumer's next 10 bytes, even though that consumer would remain within its 16-byte share. Checking only the requesting reservation, as proposed in #5466, also misses sibling reservations: `new_empty`, `split`, and `take` retain one consumer registration but have independent sizes. With two consumers and a 100-byte pool, siblings holding 30 and 20 bytes have already used their consumer's 50-byte share. Neither sibling should receive another allowance. Separately, a consumer registering after another fills the pool must not allocate beyond the configured capacity. ## What changes are included in this PR? - Account for all reservations belonging to the same DataFusion consumer ID and compare their combined usage to the fair share. - Enforce the configured aggregate pool capacity independently of the consumer check. - Release each consumer's charge as its reservations shrink or drop, and report the finite configured memory limit. - Preserve the current policy of counting all registered consumers; the spillability policy remains tracked in #5465. The JNI acquisition/release locking arrangement is unchanged. #5613 changes that arrangement and will need to retain the consumer and pool accounting checks in its admission/rollback paths. This draft includes the original fair-share correction addressed by #5466, plus the sibling and total-capacity requirements raised in its review; it should be rebased or consolidated according to the agreed merge order. ## How are these changes tested? Seven focused tests use real `CometFairMemoryPool` admission and DataFusion reservation operations, replacing only JVM memory grants/releases with a test manager. Coverage includes the 10+6+10 allocation, sibling/split/take lifetimes, identical consumer names, registration after allocation, mixed consumers under the existing policy, overflow rejection, and rollback after partial or failed acquisition. - `cargo fmt --all --manifest-path native/Cargo.toml -- --check`: passed. - `git diff --check`: passed. - An isolated `rustc` harness passed all seven added regressions using the changed pool code and current upstream DataFusion consumer/reservation source. The harness substitutes the mutex/error/JNI adapters; this is behavioral evidence, not a native build or Spark integration run. - `cargo test --locked --manifest-path native/Cargo.toml -p datafusion-comet --lib fair_pool::tests --jobs 4`: blocked before compilation. The approved registry does not currently contain `aws-smithy-runtime-api` 1.16.0 required by the unchanged upstream lockfile (available versions stop at 1.15.0). No Spark integration run is claimed. Native CI and merge-order resolution remain required before removing draft status. -- 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]
