feat: log sort shuffle spills at WARN, keep write completion at DEBUG#2091
Merged
Merged
Conversation
milenkovicm
approved these changes
Jul 18, 2026
milenkovicm
left a comment
milenkovicm
left a comment
Contributor
There was a problem hiding this comment.
thanks @andygrove, just one comment, but fell free to ignore it if you dont agree
| elapsed_secs, | ||
| ); | ||
| } else { | ||
| info!( |
Contributor
There was a problem hiding this comment.
can info be debug in this case?
it might be important at this moment but in day-to-day operations probably would be just noise? wdyt ?
Contributor
There was a problem hiding this comment.
I agree this should probably be debug
Member
Author
There was a problem hiding this comment.
Claude also wanted debug, so it's 3 against 1 at this point. Updated!
phillipleblanc
approved these changes
Jul 19, 2026
| elapsed_secs, | ||
| ); | ||
| } else { | ||
| info!( |
Contributor
There was a problem hiding this comment.
I agree this should probably be debug
Log sort shuffle spills at warn! with job, stage, and input partition context so memory pressure is visible at the default log level. Keep the per-partition write completion message at debug! and add the output row count to it.
andygrove
force-pushed
the
worktree-shuffle-write-logging
branch
from
July 19, 2026 13:53
f1223b2 to
3a0741b
Compare
andygrove
changed the title
andygrove
marked this pull request as draft
July 19, 2026 14:19
Member
Author
|
The timings in the log messages are very misleading because they include the time to execute the query that produces the shuffle data. Moving this to draft for now. |
…apsed The previous log reported now.elapsed(), which spans the entire input stream consumption and therefore includes upstream query execution, not just shuffle write cost. Report the existing repart_time, spill_time, and write_time metrics instead, which isolate the partition, spill, and finalize phases.
andygrove
marked this pull request as ready for review
July 20, 2026 13:47
…ite-logging # Conflicts: # ballista/core/src/execution_plans/sort_shuffle/writer.rs
phillipleblanc
approved these changes
Jul 20, 2026
andygrove
added a commit
to andygrove/datafusion-ballista
that referenced
this pull request
Jul 20, 2026
Resolve conflicts from the merged sort-shuffle spill-cap work (apache#2091): adopt the optional proto field and Option<u64> serde handling from main, keep the per-task budget default at 0 (uncapped), and preserve this branch's INFO-level shuffle-write logging while folding in main's repart/spill/write timing breakdown. Take main's refreshed benchmarking.md results as the base for the pending SF1000 re-run.
andygrove
added a commit
to andygrove/datafusion-ballista
that referenced
this pull request
Jul 20, 2026
Resolve conflicts from the merged sort-shuffle spill-cap work (apache#2091): adopt the optional proto field and Option<u64> serde handling from main, keep the per-task budget default at 0 (uncapped), and preserve this branch's INFO-level shuffle-write logging while folding in main's repart/spill/write timing breakdown. Take main's refreshed benchmarking.md results as the base for the pending SF1000 re-run.
andygrove
added a commit
to andygrove/datafusion-ballista
that referenced
this pull request
Jul 21, 2026
Drop this branch's change of the sort-shuffle per-task spill-cap default to 0 (uncapped); restore the 256 MiB default from main. The serde plumbing for the value stays (it landed on main via apache#2091). This PR no longer alters the shipped spill-cap default.
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.
Which issue does this PR close?
Part of the observability work tracked in #2081. This extracts the sort
shuffle write/spill logging from the WIP PR #2084 into a small, standalone
change.
Rationale for this change
When a sort shuffle task spills to disk under memory pressure it produced no
log output at the default level, so spilling was effectively invisible.
Spilling is a key signal when diagnosing slow or memory-constrained TPC-H
stages, so it is worth surfacing even at the default log level.
Per reviewer feedback, the routine per-partition write completion message
stays at
debug!to avoid adding day-to-day noise.What changes are included in this PR?
warn!that names the job, stage, andinput partition and reports the bytes, batches, and events spilled under
memory pressure.
debug!, and addthe output row count to that message.
Are there any user-facing changes?
Executor logs now report sort shuffle spill activity at
WARN. The routinewrite completion message remains at
DEBUG. There are no API orconfiguration changes.