github-actions[bot] commented on code in PR #70474:
URL: https://github.com/apache/doris/pull/70474#discussion_r4545962271
##########
be/src/exec/spill/spill_file.cpp:
##########
@@ -88,47 +87,11 @@ void SpillFile::gc() {
return;
}
_dir_created = false;
- // Delete the spill directory (or object key prefix) directly instead of
moving it to a
- // GC directory. No existence check: for object storage a "directory"
never exists as an
- // object, while deleting a missing local directory or an empty prefix is
a no-op.
- // The filesystem is pinned when the file is created; later files may use
another vault.
- auto fs = _fs;
- DORIS_CHECK(fs != nullptr) << "spill store " << _data_dir->path() << " is
not ready";
- Status status = fs->delete_directory(_spill_dir);
- DBUG_EXECUTE_IF("fault_inject::spill_file::gc", {
- status = Status::Error<INTERNAL_ERROR>("fault_inject spill_file gc
failed");
- });
- if (status.ok()) {
- _data_dir->release(written_bytes);
- _data_dir->release_persisted_bytes(std::exchange(_persisted_bytes, 0));
- _data_dir->record_spill_file_deleted();
- LOG(INFO) << "[spill][delete] " << file_context
- << ", cleanup=" << (_data_dir->is_remote() ? "object_prefix"
: "directory")
- << ", status=OK";
- return;
- }
- if (_data_dir->is_remote()) {
- auto reconcile_status =
- reconcile_remote_spill_bytes(_data_dir, _fs, _spill_dir,
&_persisted_bytes);
- if (!reconcile_status.ok()) {
- LOG_EVERY_T(WARNING, 60)
- << "failed to reconcile remote spill bytes after deletion:
" << file_context
- << ", status=" << reconcile_status;
- }
- }
- const int64_t persisted_bytes = std::exchange(_persisted_bytes, 0);
- // The data is still stored: keep it charged until a retry of the manager
deletes it.
auto* manager = ExecEnv::GetInstance()->spill_file_mgr();
- LOG(WARNING) << "[spill][delete] " << file_context
- << ", persisted_bytes_remaining=" << persisted_bytes
- << ", retry_queued=" << (manager != nullptr ? "true" :
"false")
- << ", status=" << status;
- if (manager == nullptr) {
- _data_dir->release(written_bytes);
- return;
- }
- manager->retry_spill_directory_deletion(_data_dir, _fs, _spill_dir,
written_bytes,
- persisted_bytes, _query_id,
{file_context});
+ DORIS_CHECK(manager != nullptr);
+ manager->queue_spill_directory_deletion(_data_dir, _fs, _spill_dir,
written_bytes,
Review Comment:
[P2] Avoid quadratic work when queuing normal spill cleanup
Every written file now reaches `_add_pending_directory()` here, which scans
the entire pending vector twice while holding
`_pending_spill_directories_mutex`. Distinct sort runs or build/probe partition
paths are siblings, so they do not merge. If a spilling query releases N files
while GC is sleeping or waiting on an object-store delete, enqueueing alone
performs O(N^2) work on the query thread and blocks cleanup enqueueing for
other queries. This previously affected failed deletes only; it now affects
successful normal cleanup too, and remote query teardown does not add a parent
entry to collapse the backlog. Use an indexed/amortized constant-time enqueue
or defer coalescing to the GC thread.
--
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]
