mrhhsg commented on code in PR #67845:
URL: https://github.com/apache/doris/pull/67845#discussion_r3986991616
##########
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:
Fixed. `PausedQuery::enqueue_at` and `elapsed_time()` now use
`std::chrono::steady_clock`, and the timeout test backdates the enqueue time
instead of sleeping.
##########
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:
Fixed. `handle_process_memory_exceeded_()` now checks
`is_exceed_soft_mem_limit()` before the min-memory/revoke routing and resumes
the query right away when the pressure is gone, so both routes recover
promptly. Added `process_mem_exceeded_below_min_memory_resumes` for the
`total_mem_used() <= min_memory_limit()` case.
##########
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:
Fixed. The grace period now also stops when `is_exceed_hard_mem_limit()` is
true, so the fallback no longer depends on the daemon memory gc
(`disable_memory_gc`). The comment and the cancellation message were updated,
and `process_mem_exceeded_cancels_at_hard_limit` covers it.
##########
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:
This is pre-existing behavior shared by every cancellation that goes through
`release_query_memory_()` (query limit, workload group and process paths): the
cancelled entry is erased before `revoking_memory_from_other_query_` is set, so
the next round resumes all peers without waiting for
`wait_cancel_release_memory_ms`, and a re-paused query gets a fresh timer. It
is not introduced by this PR, although the longer grace period makes the timer
reset more visible. Changing when the cancelled entry is dropped affects the
existing phase3 semantics and tests, so I would rather handle it in a separate
PR than widen this one.
--
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]
