Repository navigation
fix: let executor spills use the work dir's free space by default - #2797
Conversation
DataFusion caps spill files at 100 GB per runtime. At TPC-H SF1000 on two executors, q5 and q18 spill more than that and fail, although the executor's work dir is a 1 TB volume. When --max-temp-directory-size is not set, use the space free on the work dir's filesystem at startup, never less than DataFusion's default.
2e84162 to
d039b0e
Compare
villebro
left a comment
villebro
left a comment
There was a problem hiding this comment.
Not sure of the runtime overhead implications, but by default, I feel it'd be better if we could check this limit from available work-dir space while the executor is running, instead of relying on a startup snapshot? As an operator, by default, I’d expect 1 TB free on the work-dir to allow workloads to spill beyond 100 GB when needed. With the current rule, a query can hit the 100 GB cap and fail despite that free space, which seems like unexpected default behavior.
| /// that session's tasks). When unset, DataFusion's default (100 GB) | ||
| /// applies. Accepts the same values as `--memory-pool-size`. | ||
| /// that session's tasks). When unset, the space free on the work dir's | ||
| /// filesystem at startup, or DataFusion's default (100 GB) if that is | ||
| /// smaller. Accepts the same values as `--memory-pool-size`. |
There was a problem hiding this comment.
This was slightly difficult to parse. Maybe we could reword this to something more readable, like
When unset, defaults to the smaller of the work-dir filesystem’s free space at startup and DataFusion’s 100 GB default.
There was a problem hiding this comment.
Good call, reworded. One small correction to the suggested text though: it's the larger of the two, not the smaller. The default never goes below DataFusion's 100 GB, so with 1 TB free the limit is about 1 TB.
milenkovicm
left a comment
milenkovicm
left a comment
There was a problem hiding this comment.
One comment regarding free space calculation
|
|
||
| /// Bytes available to unprivileged users on the filesystem holding `path`, or | ||
| /// on the closest existing parent when `path` itself does not exist yet. | ||
| fn free_disk_space(path: &str) -> Option<u64> { |
There was a problem hiding this comment.
We have sysinfo already as dependency, I believe it has free space, can we use it ?
There was a problem hiding this comment.
Switched to sysinfo::Disks, thanks! One wrinkle: on Linux sysinfo leaves tmpfs and network mounts out of its list by default. So I match the work dir to its disk by device id rather than path prefix. Otherwise a work dir on tmpfs would get the free space of /. If no listed disk matches, we fall back to DataFusion's default as before.
Replace the unsafe libc statvfs call with sysinfo's disk list, matching the work dir to its disk by device id on unix, and reword the --max-temp-directory-size docs to say the default is the larger of the free space and DataFusion's 100 GB.
|
@villebro I think the old wording made this read backwards, sorry about that! With 1 TB free on the work dir at startup, the limit is about 1 TB, so the scenario you describe (failing at 100 GB despite the free space) is what this PR fixes. A live check is doable since DataFusion's |
Which issue does this PR close?
No single issue. Split out of #2792. Related: #2520 (performance review tracking).
Rationale for this change
DataFusion caps spill files at 100 GB per runtime. At TPC-H SF1000 on two executors, q5 and q18 spill more than that and fail, although the executor's work dir is on a 1 TB volume.
What changes are included in this PR?
When
--max-temp-directory-sizeis not set, the executor uses the space available on the work dir's filesystem at startup (statvfs, unix only), never less than DataFusion's 100 GB default. If the free space cannot be read, a warning is logged and DataFusion's default applies as before. A configured value is used unchanged. A work dir that does not exist yet is measured on its closest existing parent. The default applies to every runtime the executor produces, including one fromoverride_runtime_producer, as an explicit--max-temp-directory-sizealready does (the shipped executor binary always installs one).Are there any user-facing changes?
Yes. With
--max-temp-directory-sizeunset, the executor's spill limit is the work dir's free space at startup instead of 100 GB. The flag's help text is updated. The limit is per session runtime and measured once at startup, as the flag's limit is. Concurrent sessions can therefore together spill more than the free space, and shuffle files in the same dir are not counted. Set the flag where that matters, for example on a tmpfs or shared volume.