Make DiskManager max_temp_directory_size dynamically adjustable#22246
Open
Bukhtawar wants to merge 2 commits into
Open
Make DiskManager max_temp_directory_size dynamically adjustable#22246Bukhtawar wants to merge 2 commits into
Bukhtawar wants to merge 2 commits into
Conversation
| assert_eq!(dm.max_temp_directory_size(), 2048); | ||
|
|
||
| // Can also decrease | ||
| dm.set_max_temp_directory_size(512)?; |
Contributor
There was a problem hiding this comment.
What happens if queries are holding disk above the new configurable limit ? When we change the configuration from 1024 mb to 512 mb, what's the behavior.
Author
There was a problem hiding this comment.
Added tests, thanks for pointing this out
Change `max_temp_directory_size` from `u64` to `AtomicU64` so the spill disk limit can be adjusted at runtime without requiring exclusive (`&mut`) access to the DiskManager. Before this change, `set_max_temp_directory_size` required `&mut self` which was unavailable after the DiskManager was shared via `Arc` (as it always is in production through RuntimeEnv). The only workaround was `set_arc_max_temp_directory_size` which required `Arc::get_mut` — always failing when multiple sessions held references. After this change, `set_max_temp_directory_size` takes `&self` and uses an atomic store. Any thread can adjust the limit, and subsequent spill writes immediately see the new value. Use cases: - Adaptive spill limits based on available disk space - Runtime cluster setting changes without restart - Graceful degradation under disk pressure The `set_arc_max_temp_directory_size` method is deprecated but kept for backward compatibility — it now delegates to the `&self` method. Performance: `AtomicU64::load(Acquire)` adds ~1ns per spill write check. Negligible since spill writes take milliseconds. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
faad8d4 to
f22c753
Compare
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
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.
Change
max_temp_directory_sizefromu64toAtomicU64so the spill disk limit can be adjusted at runtime without requiring exclusive (&mut) access to the DiskManager.Before this change,
set_max_temp_directory_sizerequired&mut selfwhich was unavailable after the DiskManager was shared viaArc(as it always is in production through RuntimeEnv). The only workaround wasset_arc_max_temp_directory_sizewhich requiredArc::get_mut— always failing when multiple sessions held references.After this change,
set_max_temp_directory_sizetakes&selfand uses an atomic store. Any thread can adjust the limit, and subsequent spill writes immediately see the new value.Use cases:
The
set_arc_max_temp_directory_sizemethod is deprecated but kept for backward compatibility — it now delegates to the&selfmethod.Performance:
AtomicU64::load(Acquire)adds ~1ns per spill write check. Negligible since spill writes take milliseconds.Which issue does this PR close?
Rationale for this change
Ensure can be updated in-place based on available disk or in-place disk size increase
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?