alamb commented on code in PR #23522:
URL: https://github.com/apache/datafusion/pull/23522#discussion_r3587456738
##########
datafusion/physical-plan/src/spill/spill_pool.rs:
##########
@@ -723,7 +721,7 @@ impl Stream for SpillPoolReader {
}
// No files in queue - check if writer is done
- if shared.writer_dropped {
+ if shared.active_writer_count == 0 {
Review Comment:
I wonder (maybe as a follow on PR) if adding some sort of RAAI guard for
updating active_writer_count would make the code les error prone.
Or maybe even having a separate list of files being actively written (rather
than just a count 🤔 )
##########
datafusion/physical-plan/src/spill/spill_pool.rs:
##########
Review Comment:
I this these comments would be nice to update to reflect the new design
Specifically note that the `current_write_files` is a list of files that are
currently open but not being acively written to. When a file is being actively
written to, active writer count is incremented by one and an entry is removed
from current_write_files
##########
datafusion/physical-plan/src/spill/spill_pool.rs:
##########
@@ -259,31 +254,35 @@ impl Drop for SpillPoolWriter {
return;
}
- // Finalize the current file when the last writer is dropped
- if let Some(current_file) = shared.current_write_file.take() {
- // Release shared lock before locking file
+ // Finalize any spill files that were not finished yet
+
+ if !shared.current_write_files.is_empty() {
+ // Copy and clear `current_write_files` so we can release shared
lock before locking files
+ let files = shared.current_write_files.clone();
+ shared.current_write_files.clear();
Review Comment:
> I'm not entirely sure. My assumption was that this was already carefully
tuned, so I didn't want to change the lock nesting. If I understood it
correctly, the intention is to avoid IO while the shared lock is held.
InProgressSpilFile::finish can do IO so the shared lock is released before
calling finish.
we also need to be sure we don't have a deadlock due to lock inversion (aka
try to take the file/shared locks in different orders across different code
paths)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
