Uh oh!
There was an error while loading. Please reload this page.
refactor: Set expected runtime config in error message when the used disk space during the spilling process has exceeded the allocation limit - #20375
Conversation
erenavsarogullari
commented
Feb 26, 2026
kosiew
left a comment
There was a problem hiding this comment.
Thanks @erenavsarogullari for your contribution.
| return resources_err!( | ||
| "The used disk space during the spilling process has exceeded the allowable limit of {}. Try increasing the `max_temp_directory_size` in the disk manager configuration.", | ||
| "The used disk space during the spilling process has exceeded the allowable limit of {}. \ | ||
| Please try increasing the config: `datafusion.runtime.max_temp_directory_size`.", |
There was a problem hiding this comment.
datafusion.runtime.max_temp_directory_size is now part of user-facing messaging; centralizing it would reduce drift risk if keys are renamed or normalized elsewhere.
There was a problem hiding this comment.
@kosiew Firstly, thanks for the review. Does centralizing mean to be defined as constant (e.g: under runtime_env.rs)? If so, AFAIS, DF configs do not have constant reference for config names and i think it can be useful to avoid config name duplication and drift risk as you mentioned. It can be extended for all configs (specially used under Rust source files (*.rs))
Also, can version property per ConfigEntry be useful by showing when the config is added (e.g: 53.0.0)? Spark uses version property per config as reference.
There was a problem hiding this comment.
DF configs do not have constant reference for config name
What you pointed out is absolutely right – existing ConfigEntry::new_… calls just inline the string and there isn’t a “global constants” module that every caller uses.
Let's leave it as a separate wider issue to be addressed later.
can version property per ConfigEntry be useful by showing when the config is added (e.g: 53.0.0)?
It would be useful but it's not in the existing ConfigEntry struct and no convention.
I think it would have to be a separate PR to update ConfigEntry with additional version field and helpers.
Until then the only way to know when a key was added is to look at the changelog/commit history.
There was a problem hiding this comment.
What you pointed out is absolutely right – existing ConfigEntry::new_… calls just inline the string and there isn’t a “global constants” module that every caller uses.
Let's leave it as a separate wider issue to be addressed later.
I think global constants covering all DF configs (specially Rust source files (*.rs)) can be useful for the end-users as well because when they extend/maintain DF fork, they can also use this new infra by avoiding config name duplication and drift risk. In terms of these, does it make sense to create a separate issue to support global constants covering all DF configs? If so, i can create and work on this.
It would be useful but it's not in the existing ConfigEntry struct and no convention.
I think it would have to be a separate PR to update ConfigEntry with additional version field and helpers.
Until then the only way to know when a key was added is to look at the changelog/commit history.
Yes, currently, ConfigEntrystruct does not have version property and we introduce new releases frequently so to track when the config/feature is added, can also be useful. Does it also make sense to create a separate issue for this?
Uh oh!
There was an error while loading. Please reload this page.
… during the spilling process has exceeded the allocation limit
04034d1 to
b69d590Compareb69d590 to
c8ead31CompareUh oh!
There was an error while loading. Please reload this page.
…disk space during the spilling process has exceeded the allocation limit (apache#20375) ## Which issue does this PR close? - Closesapache#20373. ## Rationale for this change Minor refactoring on error message by exposing required config name for the end user. This is follow-up PR to both PR: apache#20226 and apache#20372 by runtime config: `datafusion.runtime.max_temp_directory_size`. ## What changes are included in this PR? **Current:** ``` The used disk space during the spilling process has exceeded the allowable limit of {}. Try increasing the `max_temp_directory_size` in the disk manager configuration. ``` **New:** ``` The used disk space during the spilling process has exceeded the allowable limit of {}. \ Please try increasing the config: `datafusion.runtime.max_temp_directory_size`.", ``` ## Are these changes tested? Yes, legacy UT case has been updated by covering expected config name. ## Are there any user-facing changes? Yes, error message has been updated which is exposed to end-users.
Which issue does this PR close?
Rationale for this change
Minor refactoring on error message by exposing required config name for the end user. This is follow-up PR to both PR: #20226 and #20372 by runtime config:
datafusion.runtime.max_temp_directory_size.What changes are included in this PR?
Current:
New:
Are these changes tested?
Yes, legacy UT case has been updated by covering expected config name.
Are there any user-facing changes?
Yes, error message has been updated which is exposed to end-users.