mrhhsg commented on code in PR #67845:
URL: https://github.com/apache/doris/pull/67845#discussion_r4226693464
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -602,6 +602,21 @@ bool WorkloadGroupMgr::handle_process_memory_exceeded_(
return false;
}
+ // The process memory pressure may have been relieved by cache reclamation
or by other
+ // queries that finished. Check it before routing the query below,
otherwise a query in a
+ // workload group that uses less than its min memory limit has to wait for
the timeout.
+ const size_t test_memory_size = std::max<size_t>(query_it->reserve_size_,
32L * 1024 * 1024);
Review Comment:
Fixed in f1e5281b2b5. The recovery check in
`handle_process_memory_exceeded_()` now tests the recorded `reserve_size_`
directly. `is_exceed_soft_mem_limit(bytes)` evaluates the same two conditions
as `try_reserve_process_memory(bytes)` (process usage + bytes against the soft
limit, and system available memory - bytes against the warning water mark), so
the query is resumed exactly when retrying its request would succeed. Added
`process_mem_exceeded_resumes_when_reservation_fits_soft_limit` (16 MiB
headroom below the soft limit) and
`process_mem_exceeded_resumes_when_reservation_fits_sys_mem_available` (16 MiB
above the warning water mark); both assert that a 32 MiB probe would still
report pressure.
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -620,11 +635,11 @@ bool WorkloadGroupMgr::handle_process_memory_exceeded_(
// TODO revoke from memtable
- // Fallback: if we have waited too long and cannot revoke from anywhere,
- // cancel the query or disable reserve memory to let it proceed.
- if (query_it->elapsed_time() > config::spill_in_paused_queue_timeout_ms) {
- // Cannot spill (no revocable memory), cannot revoke from other WGs,
- // and process memory is still exceeded. Cancel the query to protect
the system.
+ // Fallback: if the process reaches the hard limit or we have waited too
long and cannot
+ // revoke from anywhere, let this query spill or cancel it to protect the
process.
Review Comment:
Fixed in f1e5281b2b5. `revoke_memory_from_other_groups_()` now returns the
result of `max_wg->revoke_memory(...)`, i.e. the memory actually revoked, and
logs both the target and the freed amount. When it is zero the below-minimum
route falls through to the hard-limit/timeout fallback instead of setting
`revoking_memory_from_other_query_`. `ProcessMemoryNotEnough` expectations were
updated to the actual freed sizes, and
`process_mem_exceeded_below_min_memory_with_non_reclaimable_peer` covers an
over-minimum peer made of ten 30 MiB queries (all filtered by
`EXCLUDE_IS_SMALL`): the paused query stays paused below the hard limit without
the revoking flag being set, is cancelled at the hard limit, and no peer query
is cancelled.
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -824,38 +839,45 @@ bool WorkloadGroupMgr::handle_single_query_(const
std::shared_ptr<ResourceContex
return true;
}
} else {
- // Should not consider about process memory. For example, the query's
limit is 100g, workload
- // group's memlimit is 10g, process memory is 20g. The query reserve
will always failed in wg
- // limit, and process is always have memory, so that it will resume
and failed reserve again.
- const size_t test_memory_size = std::max<size_t>(size_to_reserve, 32L
* 1024 * 1024);
- if
(!GlobalMemoryArbitrator::is_exceed_soft_mem_limit(test_memory_size)) {
- LOG(INFO) << "Query: " << query_id
- << ", process limit not exceeded now, resume this query"
- << ", process memory info: "
- <<
GlobalMemoryArbitrator::process_memory_used_details_str()
- << ", wg info: " << wg->debug_string();
- requestor->task_controller()->set_memory_sufficient(true);
- return true;
- } 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.
- Status error_status = Status::MemoryLimitExceeded(
- "Query {} process memory is exceeded"
- ", and there is no cache now. And could not find task to
spill, disable "
- "reserve memory and resume it. "
- "Query memory usage: {}, limit: {}, reserved "
- "size: {}, try to reserve: {}, wg info: {}."
- " Maybe you should set the workload group's limit to a
lower value. {}",
- query_id, PrettyPrinter::print_bytes(memory_usage),
- PrettyPrinter::print_bytes(limit),
PrettyPrinter::print_bytes(reserved_size),
- PrettyPrinter::print_bytes(size_to_reserve),
wg->memory_debug_string(),
- doris::ProcessProfile::instance()
- ->memory_profile()
- ->process_memory_detail_str());
- LOG_LONG_STRING(INFO, error_status.to_string());
- requestor->task_controller()->cancel(error_status);
- return true;
+ // PROCESS_MEMORY_EXCEEDED. The caller
(handle_process_memory_exceeded_) has already
+ // resumed the query if the process is no longer above the soft memory
limit, so the
Review Comment:
Fixed in f1e5281b2b5. The process-pressure decision moved out of the shared
early exits into `resolve_process_memory_exceeded_query_()`, which
`handle_single_query_()` calls right after `get_revocable_info()`. A running
task still postpones the decision below the hard limit (spill needs idle
tasks), but at the hard limit the query is cancelled at once, since spilling is
unsafe while the task runs and the manager must not rely on memory gc. A query
with revocable tasks is still spilled at the hard limit; if the spill callback
resumes it without freeing memory, the retry fails, the query is paused again
without revocable tasks and the next round cancels it, so the loop converges.
Added `process_mem_exceeded_running_task_waits_below_hard_limit`,
`process_mem_exceeded_running_task_cancels_at_hard_limit` and
`process_mem_exceeded_spills_revocable_tasks_at_hard_limit` (including the
re-pause after a spill that freed nothing) via `MockQueryTaskController` knobs
for running/revocable task
s.
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -824,38 +839,45 @@ bool WorkloadGroupMgr::handle_single_query_(const
std::shared_ptr<ResourceContex
return true;
}
} else {
- // Should not consider about process memory. For example, the query's
limit is 100g, workload
- // group's memlimit is 10g, process memory is 20g. The query reserve
will always failed in wg
- // limit, and process is always have memory, so that it will resume
and failed reserve again.
- const size_t test_memory_size = std::max<size_t>(size_to_reserve, 32L
* 1024 * 1024);
- if
(!GlobalMemoryArbitrator::is_exceed_soft_mem_limit(test_memory_size)) {
- LOG(INFO) << "Query: " << query_id
- << ", process limit not exceeded now, resume this query"
- << ", process memory info: "
- <<
GlobalMemoryArbitrator::process_memory_used_details_str()
- << ", wg info: " << wg->debug_string();
- requestor->task_controller()->set_memory_sufficient(true);
- return true;
- } 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.
- Status error_status = Status::MemoryLimitExceeded(
- "Query {} process memory is exceeded"
- ", and there is no cache now. And could not find task to
spill, disable "
- "reserve memory and resume it. "
- "Query memory usage: {}, limit: {}, reserved "
- "size: {}, try to reserve: {}, wg info: {}."
- " Maybe you should set the workload group's limit to a
lower value. {}",
- query_id, PrettyPrinter::print_bytes(memory_usage),
- PrettyPrinter::print_bytes(limit),
PrettyPrinter::print_bytes(reserved_size),
- PrettyPrinter::print_bytes(size_to_reserve),
wg->memory_debug_string(),
- doris::ProcessProfile::instance()
- ->memory_profile()
- ->process_memory_detail_str());
- LOG_LONG_STRING(INFO, error_status.to_string());
- requestor->task_controller()->cancel(error_status);
- return true;
+ // PROCESS_MEMORY_EXCEEDED. The caller
(handle_process_memory_exceeded_) has already
+ // resumed the query if the process is no longer above the soft memory
limit, so the
+ // process memory is still exceeded here.
Review Comment:
Fixed in f1e5281b2b5. `resolve_process_memory_exceeded_query_()`
re-evaluates `is_exceed_soft_mem_limit(size_to_reserve)` right before the
spill/timeout/hard-limit decision and resumes the query if the pressure is
gone; the early check before the min-memory routing is kept for prompt
below-minimum recovery.
`process_mem_exceeded_recovery_before_timeout_decision_resumes` relieves the
pressure from a hook inside `get_revocable_info()` (between the two checks)
with the timer already expired and verifies the query is resumed instead of
cancelled.
--
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]
