fix: account fair-pool memory across sibling reservations - #5847
Draft
sunchao wants to merge 2 commits into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Addresses the fair-memory-pool accounting in #5212. Overlaps #5466 and needs merge-order coordination with #5613.
Rationale for this change
The fair pool can force operators to spill even when they have room within their own allowance. Consider a 32 MiB pool shared by two consumers, each entitled to 16 MiB. If one holds 10 MiB and the other holds 6 MiB, the second should be able to acquire another 10 MiB. Today, Comet compares the pool's combined 16 MiB usage against that consumer's 16 MiB allowance and rejects the request. With more consumers, this can leave an increasingly large part of the configured pool unusable.
The correction also needs to distinguish a consumer, which owns the allowance, from its reservations, which account for individual buffers. One operator can hold several reservations under the same consumer registration. Checking only the reservation making the request would still let a consumer whose sibling reservations already hold 10 MiB and 6 MiB acquire more through an empty sibling, despite having exhausted its 16 MiB share.
There is a separate total-capacity constraint. An operator might fill the pool before a second consumer registers. The newcomer has a fair share on paper, but cannot allocate until some existing memory is released.
What changes are included in this PR?
The pool now treats all reservations belonging to one consumer as a single account. Growth is admitted only when it fits both that consumer's fair share and the space remaining in the whole pool. Splitting a reservation, moving its contents, or creating an empty sibling preserves the existing charge; releasing memory restores the consumer's available allowance. The pool also reports its configured finite capacity.
This preserves the current policy of dividing the pool among all registered consumers; changing how spillable consumers participate remains in #5465. This overlaps the fair-share correction in #5466 and extends it to cover siblings and total capacity. #5613 separately changes locking around JVM memory calls. These changes need an agreed merge order so its admission and rollback paths retain the same accounting guarantees.
How are these changes tested?
Seven regression tests exercise the actual pool admission logic with DataFusion reservations and simulated JVM grants. They cover the examples above, reservation splitting and transfer, release and retry, mixed consumers, oversized requests, and failed or partial grants.
The original PR revision passed native Rust CI. Its only failed test was a shared decimal codegen coverage assertion, which is corrected by upstream #5849 and now included in this branch. The assertion was reproduced before the correction and passed afterward in a focused local run; local formatting checks also passed.
See fresh CI for revision
865529d4for validation of the updated branch. A dedicated Spark off-heap integration run has not been performed.