Add ability to disable DiskManager #4330
Conversation
tustvold
force-pushed
the
disk-manager-disable
branch
from
fcb49df
to
60b17a9
Compare
There was a problem hiding this comment.
Looks great -- thank you @tustvold
I really like that this should allow unified implementations of spilling operators as well as "error if we run out"
cc @crepererum
| /// TempDirs to put temporary files in. | ||
| /// | ||
| /// If `Some(vec![])` a new OS specified temporary directory will be created | ||
| /// If `None` an error will be returned |
There was a problem hiding this comment.
| /// If `None` an error will be returned | |
| /// If `None` an error will be returned (configured not to spill) |
| let mut guard = self.local_dirs.lock(); | ||
| let local_dirs = guard.as_mut().ok_or_else(|| { | ||
| DataFusionError::ResourcesExhausted( | ||
| "Cannot spill to temporary file as DiskManager is disabled".to_string(), |
There was a problem hiding this comment.
If a user sees this it may not be clear what is happening or what they can do about it.
What do you think about changing the message to something like
| "Cannot spill to temporary file as DiskManager is disabled".to_string(), | |
| "Memory Exhausted (DiskManager is disabled, cannot spill to temporary file)".to_string(), |
As a follow up (which I will do), I think it would be excellent to pass in a context string to create_tmp_file to make the error more specific.
pub fn create_tmp_file(&self, context: &str) -> Result<NamedTempFile> {
|
I will improve the error messages in a follow on PR |
|
Benchmark runs are scheduled for baseline = 07f65bf and contender = df8aa7a. df8aa7a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
Follow on in #4348 |
Which issue does this PR close?
re #3941
Rationale for this change
I want to be able to enable a memory limit, but instead of spilling to disk return an error. The simplest way I could see to support this was to add the ability to create a DiskManager that is disabled.
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?