github-actions[bot] commented on code in PR #67845:
URL: https://github.com/apache/doris/pull/67845#discussion_r3986772695
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -836,17 +836,32 @@ bool WorkloadGroupMgr::handle_single_query_(const
std::shared_ptr<ResourceContex
<< ", wg info: " << wg->debug_string();
requestor->task_controller()->set_memory_sufficient(true);
return true;
+ } else if (time_in_queue < config::spill_in_paused_queue_timeout_ms) {
+ // The query has no revocable memory, cancelling it will not
release much memory.
+ // Keep it paused so that it can be resumed once other queries
release memory,
Review Comment:
This recovery check is reached every round only when
`handle_process_memory_exceeded_()` sees `wg->total_mem_used() >
wg->min_memory_limit()`. For a query at or below its WG minimum, that handler
tries other WGs and then only advances the iterator until the timeout; if cache
or non-WG reclamation has already cleared the process soft limit and no peer WG
is over its minimum, the query remains blocked for the default 60 seconds
before it can reach this check. Please test process-pressure recovery before
the minimum-memory/revoke routing (and add a `total_mem_used() <=
min_memory_limit()` recovery case) so the prompt-resume behavior applies to
this parallel path too.
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -836,17 +836,32 @@ bool WorkloadGroupMgr::handle_single_query_(const
std::shared_ptr<ResourceContex
<< ", wg info: " << wg->debug_string();
requestor->task_controller()->set_memory_sufficient(true);
return true;
+ } else if (time_in_queue < config::spill_in_paused_queue_timeout_ms) {
Review Comment:
The new bounded wait is driven by `PausedQuery::elapsed_time()`, but that
duration is computed from `std::chrono::system_clock`. If the host clock steps
backward while process pressure persists, this condition can keep the query
paused past `spill_in_paused_queue_timeout_ms`; a forward step can cancel it
early. Please store/measure the enqueue interval with a monotonic source
(`steady_clock` or the existing Doris monotonic helper), and preferably make
the timeout test advance/backdate that monotonic timestamp instead of sleeping
on wall time.
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -836,17 +836,32 @@ bool WorkloadGroupMgr::handle_single_query_(const
std::shared_ptr<ResourceContex
<< ", wg info: " << wg->debug_string();
requestor->task_controller()->set_memory_sufficient(true);
return true;
+ } else if (time_in_queue < config::spill_in_paused_queue_timeout_ms) {
+ // The query has no revocable memory, cancelling it will not
release much memory.
+ // Keep it paused so that it can be resumed once other queries
release memory,
+ // and cancel it only after it has waited for
`spill_in_paused_queue_timeout_ms`.
+ // If the process memory keeps growing, memory gc will cancel
queries when the
Review Comment:
This safety argument is not true when the supported mutable
`disable_memory_gc` setting is enabled: `Daemon::memory_gc()` returns
immediately, while this branch checks only the soft-limit predicate and can
keep the already-paused memory owner blocked for the full timeout after the
process reaches its hard limit. Allocation checks may fail some later
allocating thread, but they do not reclaim/cancel this paused query. Please
make the grace path stop at the hard threshold (and account for disabled GC),
or otherwise make the hard-limit fallback independent of the daemon GC setting.
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -836,17 +836,32 @@ bool WorkloadGroupMgr::handle_single_query_(const
std::shared_ptr<ResourceContex
<< ", wg info: " << wg->debug_string();
requestor->task_controller()->set_memory_sufficient(true);
return true;
+ } else if (time_in_queue < config::spill_in_paused_queue_timeout_ms) {
+ // The query has no revocable memory, cancelling it will not
release much memory.
+ // Keep it paused so that it can be resumed once other queries
release memory,
+ // and cancel it only after it has waited for
`spill_in_paused_queue_timeout_ms`.
+ // If the process memory keeps growing, memory gc will cancel
queries when the
+ // hard limit is reached.
+ LOG_EVERY_T(INFO, 1) << "Query: " << query_id
+ << " process memory is exceeded, and could
not find task to "
+ "spill, keep it paused. Waited "
+ << time_in_queue
+ << " ms, timeout: " <<
config::spill_in_paused_queue_timeout_ms
+ << " ms, process memory info: "
+ <<
GlobalMemoryArbitrator::process_memory_used_details_str()
+ << ", wg info: " << wg->debug_string();
+ return false;
} else {
- // if cannot find any memory to release, then let the query
continue to run as far as possible
- // or cancelled by gc if memory is really not enough.
+ // Waited long enough and still could not find any memory to
release,
Review Comment:
After this timeout cancellation, `release_query_memory_()` erases the
cancelled query's paused entry before setting
`revoking_memory_from_other_query_`. On the next maintenance pass
`cleanup_paused_queries_()` therefore cannot report that cancellation as still
releasing memory, so `resume_paused_queries_after_revoke_()` resumes every peer
query without checking pressure. With two no-revocable queries that reach this
timeout together, the second is resumed while the first can still hold its
memory; when it re-pauses it gets a fresh timer and escapes the timeout it
already reached. Please retain/separately track the cancelled entry through the
release-wait window (or recheck pressure before global resume) and cover the
two-query timeout case.
--
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]
