Skip to content

Commit 03447a6

Browse files
committed
Make DiskManager max_temp_directory_size dynamically adjustable
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>
1 parent bd904b3 commit 03447a6

1 file changed

Lines changed: 96 additions & 17 deletions

File tree

datafusion/execution/src/disk_manager.rs

Lines changed: 96 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ impl DiskManagerBuilder {
7575
match self.mode {
7676
DiskManagerMode::OsTmpDirectory => Ok(DiskManager {
7777
local_dirs: Mutex::new(Some(vec![])),
78-
max_temp_directory_size: self.max_temp_directory_size,
78+
max_temp_directory_size: AtomicU64::new(self.max_temp_directory_size),
7979
used_disk_space: Arc::new(AtomicU64::new(0)),
8080
active_files_count: Arc::new(AtomicUsize::new(0)),
8181
}),
@@ -86,14 +86,14 @@ impl DiskManagerBuilder {
8686
);
8787
Ok(DiskManager {
8888
local_dirs: Mutex::new(Some(local_dirs)),
89-
max_temp_directory_size: self.max_temp_directory_size,
89+
max_temp_directory_size: AtomicU64::new(self.max_temp_directory_size),
9090
used_disk_space: Arc::new(AtomicU64::new(0)),
9191
active_files_count: Arc::new(AtomicUsize::new(0)),
9292
})
9393
}
9494
DiskManagerMode::Disabled => Ok(DiskManager {
9595
local_dirs: Mutex::new(None),
96-
max_temp_directory_size: self.max_temp_directory_size,
96+
max_temp_directory_size: AtomicU64::new(self.max_temp_directory_size),
9797
used_disk_space: Arc::new(AtomicU64::new(0)),
9898
active_files_count: Arc::new(AtomicUsize::new(0)),
9999
}),
@@ -167,8 +167,9 @@ pub struct DiskManager {
167167
/// If `None` an error will be returned (configured not to spill)
168168
local_dirs: Mutex<Option<Vec<Arc<TempDir>>>>,
169169
/// The maximum amount of data (in bytes) stored inside the temporary directories.
170-
/// Default to 100GB
171-
max_temp_directory_size: u64,
170+
/// Default to 100GB. Stored as `AtomicU64` so it can be adjusted at runtime
171+
/// without requiring exclusive (`&mut`) access to the `DiskManager`.
172+
max_temp_directory_size: AtomicU64,
172173
/// Used disk space in the temporary directories. Now only spilled data for
173174
/// external executors are counted.
174175
used_disk_space: Arc<AtomicU64>,
@@ -199,7 +200,7 @@ impl DiskManager {
199200
DiskManagerConfig::Existing(manager) => Ok(manager),
200201
DiskManagerConfig::NewOs => Ok(Arc::new(Self {
201202
local_dirs: Mutex::new(Some(vec![])),
202-
max_temp_directory_size: DEFAULT_MAX_TEMP_DIRECTORY_SIZE,
203+
max_temp_directory_size: AtomicU64::new(DEFAULT_MAX_TEMP_DIRECTORY_SIZE),
203204
used_disk_space: Arc::new(AtomicU64::new(0)),
204205
active_files_count: Arc::new(AtomicUsize::new(0)),
205206
})),
@@ -210,45 +211,48 @@ impl DiskManager {
210211
);
211212
Ok(Arc::new(Self {
212213
local_dirs: Mutex::new(Some(local_dirs)),
213-
max_temp_directory_size: DEFAULT_MAX_TEMP_DIRECTORY_SIZE,
214+
max_temp_directory_size: AtomicU64::new(DEFAULT_MAX_TEMP_DIRECTORY_SIZE),
214215
used_disk_space: Arc::new(AtomicU64::new(0)),
215216
active_files_count: Arc::new(AtomicUsize::new(0)),
216217
}))
217218
}
218219
DiskManagerConfig::Disabled => Ok(Arc::new(Self {
219220
local_dirs: Mutex::new(None),
220-
max_temp_directory_size: DEFAULT_MAX_TEMP_DIRECTORY_SIZE,
221+
max_temp_directory_size: AtomicU64::new(DEFAULT_MAX_TEMP_DIRECTORY_SIZE),
221222
used_disk_space: Arc::new(AtomicU64::new(0)),
222223
active_files_count: Arc::new(AtomicUsize::new(0)),
223224
})),
224225
}
225226
}
226227

228+
/// Atomically update the max temp directory size. Takes effect immediately
229+
/// for subsequent spill writes. Safe to call concurrently from any thread
230+
/// without requiring exclusive (`&mut`) access.
227231
pub fn set_max_temp_directory_size(
228-
&mut self,
232+
&self,
229233
max_temp_directory_size: u64,
230234
) -> Result<()> {
231-
// If the disk manager is disabled and `max_temp_directory_size` is not 0,
232-
// this operation is not meaningful, fail early.
233235
if self.local_dirs.lock().is_none() && max_temp_directory_size != 0 {
234236
return config_err!(
235237
"Cannot set max temp directory size for a disk manager that spilling is disabled"
236238
);
237239
}
238240

239-
self.max_temp_directory_size = max_temp_directory_size;
241+
self.max_temp_directory_size.store(max_temp_directory_size, Ordering::Release);
240242
Ok(())
241243
}
242244

245+
#[deprecated(since = "46.0.0", note = "Use set_max_temp_directory_size directly on &self instead")]
243246
pub fn set_arc_max_temp_directory_size(
244247
this: &mut Arc<Self>,
245248
max_temp_directory_size: u64,
246249
) -> Result<()> {
247250
if let Some(inner) = Arc::get_mut(this) {
248-
inner.set_max_temp_directory_size(max_temp_directory_size)?;
251+
inner.max_temp_directory_size.store(max_temp_directory_size, Ordering::Release);
249252
Ok(())
250253
} else {
251-
config_err!("DiskManager should be a single instance")
254+
// With AtomicU64 we can now update through shared reference
255+
this.set_max_temp_directory_size(max_temp_directory_size)
252256
}
253257
}
254258

@@ -266,7 +270,7 @@ impl DiskManager {
266270

267271
/// Returns the maximum temporary directory size in bytes
268272
pub fn max_temp_directory_size(&self) -> u64 {
269-
self.max_temp_directory_size
273+
self.max_temp_directory_size.load(Ordering::Acquire)
270274
}
271275

272276
/// Returns the current spilling progress
@@ -418,11 +422,12 @@ impl RefCountedTempFile {
418422

419423
// 3. Check if the updated global disk usage exceeds the configured limit
420424
let global_disk_usage = self.disk_manager.used_disk_space.load(Ordering::Relaxed);
421-
if global_disk_usage > self.disk_manager.max_temp_directory_size {
425+
let limit = self.disk_manager.max_temp_directory_size.load(Ordering::Acquire);
426+
if global_disk_usage > limit {
422427
return resources_err!(
423428
"The used disk space during the spilling process has exceeded the allowable limit of {}. \
424429
Please try increasing the config: `datafusion.runtime.max_temp_directory_size`.",
425-
human_readable_size(self.disk_manager.max_temp_directory_size as usize)
430+
human_readable_size(limit as usize)
426431
);
427432
}
428433

@@ -796,4 +801,78 @@ mod tests {
796801

797802
Ok(())
798803
}
804+
805+
#[test]
806+
fn test_dynamic_limit_adjustment_through_shared_ref() -> Result<()> {
807+
// Verify that set_max_temp_directory_size works through &self (not &mut self).
808+
// This is the key behavioral change: the limit can be adjusted at runtime
809+
// without exclusive access, enabling dynamic resize while queries are running.
810+
let dm = DiskManager::builder()
811+
.with_max_temp_directory_size(1024)
812+
.build()?;
813+
let dm = Arc::new(dm);
814+
815+
assert_eq!(dm.max_temp_directory_size(), 1024);
816+
817+
// Adjust through shared reference (simulates concurrent access via Arc)
818+
dm.set_max_temp_directory_size(2048)?;
819+
assert_eq!(dm.max_temp_directory_size(), 2048);
820+
821+
// Can also decrease
822+
dm.set_max_temp_directory_size(512)?;
823+
assert_eq!(dm.max_temp_directory_size(), 512);
824+
825+
Ok(())
826+
}
827+
828+
#[test]
829+
fn test_dynamic_limit_concurrent_access() -> Result<()> {
830+
// Verify that multiple threads can read and write the limit concurrently
831+
let dm = Arc::new(
832+
DiskManager::builder()
833+
.with_max_temp_directory_size(1000)
834+
.build()?,
835+
);
836+
837+
let handles: Vec<_> = (0..8)
838+
.map(|i| {
839+
let dm = Arc::clone(&dm);
840+
std::thread::spawn(move || {
841+
// Each thread sets a different limit and reads it back
842+
let new_limit = (i + 1) * 1000;
843+
dm.set_max_temp_directory_size(new_limit).unwrap();
844+
// Read should return SOME value set by one of the threads
845+
let current = dm.max_temp_directory_size();
846+
assert!(current >= 1000 && current <= 8000);
847+
})
848+
})
849+
.collect();
850+
851+
for h in handles {
852+
h.join().unwrap();
853+
}
854+
855+
// Final value should be one of the values set by threads
856+
let final_val = dm.max_temp_directory_size();
857+
assert!(final_val >= 1000 && final_val <= 8000);
858+
859+
Ok(())
860+
}
861+
862+
#[test]
863+
fn test_disabled_disk_manager_rejects_nonzero_limit() -> Result<()> {
864+
let dm = DiskManager::builder()
865+
.with_mode(DiskManagerMode::Disabled)
866+
.build()?;
867+
let dm = Arc::new(dm);
868+
869+
// Setting non-zero limit on disabled DiskManager should error
870+
let result = dm.set_max_temp_directory_size(1024);
871+
assert!(result.is_err());
872+
873+
// Setting zero is OK
874+
assert!(dm.set_max_temp_directory_size(0).is_ok());
875+
876+
Ok(())
877+
}
799878
}

0 commit comments

Comments
 (0)