Make DiskManager max_temp_directory_size dynamically adjustable#22246
Open
Bukhtawar wants to merge 1 commit into
Open
Make DiskManager max_temp_directory_size dynamically adjustable#22246Bukhtawar wants to merge 1 commit into
Bukhtawar wants to merge 1 commit 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
Bukhtawar
force-pushed
the
dynamic-disk-limit
branch
from
faad8d4 to
f22c753
Compare
Bukhtawar
force-pushed
the
dynamic-disk-limit
branch
from
3386b55 to
9478f44
Compare
xudong963
reviewed
May 17, 2026
Comment on lines
268
to
278
| pub fn set_arc_max_temp_directory_size( | ||
| this: &mut Arc<Self>, | ||
| max_temp_directory_size: u64, | ||
| ) -> Result<()> { | ||
| if let Some(inner) = Arc::get_mut(this) { | ||
| inner.set_max_temp_directory_size(max_temp_directory_size)?; | ||
| Ok(()) | ||
| inner.set_max_temp_directory_size(max_temp_directory_size) | ||
| } else { | ||
| config_err!("DiskManager should be a single instance") | ||
| // No exclusive access — use the atomic path | ||
| this.update_max_temp_directory_size(max_temp_directory_size) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
let's collapse set_arc_max_temp_directory_size. Both branches now do the same thing. Replace the body with:
this.update_max_temp_directory_size(max_temp_directory_size)and add #[deprecated(note = "use update_max_temp_directory_size")]. The Arc::get_mut dance no longer serves any purpose.
#[deprecated(note = "use `update_max_temp_directory_size` instead")]
pub fn set_arc_max_temp_directory_size(
this: &Arc<Self>,
max_temp_directory_size: u64,
) -> Result<()> {
this.update_max_temp_directory_size(max_temp_directory_size)
}
xudong963
reviewed
May 17, 2026
| /// | ||
| /// For runtime adjustment without `&mut self`, use | ||
| /// [`Self::update_max_temp_directory_size`] instead. | ||
| pub fn set_max_temp_directory_size( |
Member
There was a problem hiding this comment.
set_max_temp_directory_size(&mut self, ...) is now redundant. It does exactly the same thing as update_max_temp_directory_size but requires &mut. Keeping both is mildly confusing
Bukhtawar
force-pushed
the
dynamic-disk-limit
branch
from
41c5be9 to
ecaa3d8
Compare
Change `max_temp_directory_size` from `u64` to `AtomicU64` and add a new `update_max_temp_directory_size(&self)` method that allows adjusting the spill disk limit at runtime without requiring exclusive access. The existing `set_max_temp_directory_size(&mut self)` is preserved unchanged for backward compatibility. The new method enables: - 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` now falls through to the atomic path when `Arc::get_mut` fails (multiple references exist), instead of returning an error. When the limit is decreased below current usage: - Existing spill files remain (not deleted — would break running queries) - New spill writes fail immediately (used > new limit) - As old queries complete, usage naturally drops below the new limit - New spills succeed once usage < limit Performance: `AtomicU64::load(Acquire)` adds ~1ns per spill write check. Spill writes take milliseconds — negligible impact. Signed-off-by: Bukhtawar Khan <bukhtawa@amazon.com>
Bukhtawar
force-pushed
the
dynamic-disk-limit
branch
from
cfcb50c to
dd8ed5c
Compare
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
3 participants
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?