github-actions[bot] commented on code in PR #66328:
URL: https://github.com/apache/doris/pull/66328#discussion_r3703291210
##########
be/src/exec/spill/spill_file_manager.cpp:
##########
@@ -162,6 +163,24 @@ void SpillFileManager::delete_spill_file(SpillFileSPtr
spill_file) {
spill_file->gc();
}
+void SpillFileManager::delete_query_spill_directory(const std::string&
query_id,
+ SpillDataDir* data_dir) {
+ const auto query_dir = data_dir->get_spill_data_path(query_id);
+ const auto& fs = io::global_local_filesystem();
+ auto status = Status::OK();
+
DBUG_EXECUTE_IF("fault_inject::spill_file_manager::delete_query_spill_directory",
{
+ status = Status::Error<INTERNAL_ERROR>("injected query spill directory
deletion failure");
+ });
+ if (status.ok()) {
+ status = fs->delete_directory(query_dir);
Review Comment:
[P2] Hand finished query trees to the spill GC path
This calls `std::filesystem::remove_all` synchronously from the thread that
releases the final query/PFC reference (commonly a fixed `TaskScheduler`
worker), so a large exceptional spill tree stalls that worker. More
importantly, any transient failure is only logged and then forgotten when
`_spill_data_dirs` dies; periodic `gc()` scans only `spill_gc`, not
`<root>/spill/<query-id>`. The new failure test demonstrates this by
intentionally leaving one root behind, and when per-file GC also failed those
residual bytes were already removed from `_spill_data_bytes`. Rename the
finished tree to a unique `spill_gc` child (or enqueue it on an equivalent
lifetime-safe retry queue) and let the existing GC worker delete/retry it; test
both nonblocking handoff and eventual removal after an injected failure.
##########
be/src/runtime/query_context.h:
##########
@@ -329,6 +335,9 @@ class QueryContext : public
std::enable_shared_from_this<QueryContext> {
MonotonicStopWatch _query_watcher;
bool _is_nereids = false;
+ std::mutex _spill_data_dirs_mutex;
+ std::unordered_set<SpillDataDir*> _spill_data_dirs;
Review Comment:
[P2] Destroy the touched-root set while the query tracker is attached
`record_spill_data_dir()` allocates this set's nodes/buckets on
query-attached execution threads, but the switch local in `~QueryContext()`
ends before C++ destroys member subobjects. Not every last-owner path has an
outer query attachment: for example, `FragmentMgr::cancel_query()` destroys its
`SCOPED_ATTACH_TASK` guard before its earlier-declared local
`shared_ptr<QueryContext>`, and receiver/shutdown ownership can also be last.
The set is then freed against the caller while its bytes remain on the query
tracker; `MemTrackerLimiter::~MemTrackerLimiter()` treats that as inaccurate
accounting and can `LOG(FATAL)` when `crash_in_memory_tracker_inaccurate` is
enabled. Swap/destroy `_spill_data_dirs` inside the destructor body before the
switch guard exits (a plain `clear()` may retain buckets).
##########
be/src/runtime/query_context.cpp:
##########
@@ -247,6 +252,12 @@ QueryContext::~QueryContext() {
obj_pool.clear();
_merge_controller_handler.reset();
+ if (auto* spill_file_mgr = _exec_env->spill_file_mgr()) {
Review Comment:
[P2] Drop rerunnable ownership on FINAL_CLOSE
The cleanup here only runs when the last `QueryContext` reference drops, but
recursive-CTE child fragments store a strong `query_ctx` in
`FragmentMgr::_rerunnable_params_map`. `rerun_fragment(FINAL_CLOSE)`
closes/removes the final PFC without erasing that saved entry; only
`remove_query_context()` erases it, and that is called from the `QueryContext`
destructor itself (or cancellation). Normal success can therefore retain the
context and its query spill tree until execution timeout, cancellation, or
shutdown instead of cleaning at final close. Erase this fragment's saved entry
on successful `FINAL_CLOSE` outside `_rerunnable_params_lock` (to avoid
destructor re-entry) and add a recursive-CTE spill lifecycle test.
--
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]
