github-actions[bot] commented on code in PR #67845:
URL: https://github.com/apache/doris/pull/67845#discussion_r4090348650
##########
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:
[P1] Test recovery with the recorded reservation size
The failed reserve used the exact `reserve_size_`, but this recovery gate
substitutes at least 32 MiB. After a 1 KiB request fails, another query can
release enough to leave (say) 16 MiB of soft-limit or system-warning headroom:
retrying the recorded 1 KiB now succeeds, while this 32 MiB probe still reports
pressure. The query therefore remains paused and is eventually cancelled at the
timeout even though its requested reservation fits. I found no invariant
imposing a 32 MiB minimum retry. Please use the recorded request (ideally the
same feasibility predicate as reservation) and add process-limit and
system-available boundary tests.
##########
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:
[P1] Enforce hard pressure before pipeline-state early exits
This check is still unreachable for production states that
`handle_single_query_()` exits through first. A sibling can remain `_running`
inside one slow operator call (the scheduler time-slice is checked only between
calls), causing the return at 744 before hard pressure is tested on every 50 ms
pass. The revocable path can also collect no task on its second snapshot—or
find a task no longer revocable—and resume through the spill callback without
proving any memory was freed or rechecking hard pressure. With
`disable_memory_gc=true`, that defeats the manager's claimed hard-limit
protection. Please make hard-pressure handling independent of these
spill-safety exits (or retain hard state and require actual reclaim plus a
fresh pressure check), and add production-shaped running/revocable-task tests.
##########
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:
[P1] Use the actual peer-reclamation result before bypassing the hard
fallback
This hard-limit fallback is still skipped when an over-min peer WG exists
but cannot cancel anything. `revoke_memory_from_other_groups_()` discards
`max_wg->revoke_memory(...)`'s actual return and returns the positive 10%
target; e.g. a peer more than 128 MiB over minimum whose usage is split among
<=32 MiB tasks has every task filtered by `EXCLUDE_IS_SMALL`, so actual
reclaimed bytes are zero. We then set `revoking_memory_from_other_query_`, and
the next pass resumes/removes this requestor; its retry creates a fresh timer,
so at `disable_memory_gc=true` this can loop even at the hard limit. Please
return/use the actual reclaim result and fall through here when it is zero,
with a below-min hard-limit test using a non-reclaimable over-min peer.
##########
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:
[P2] Recheck recovered pressure at the timeout decision
The soft-limit result from `handle_process_memory_exceeded_()` is not stable
across the work before this branch. For a query whose timer has expired, line
609 can observe pressure, then pipeline-map inspection or below-min peer-WG
reclamation can run while another query/cache releases memory; if no revocable
task remains, this branch cancels solely from the old observation even though
pressure is now clear. The previous check at this terminal location would have
resumed it. Please retain the early check for prompt below-min recovery, but
re-evaluate the same soft predicate immediately before timeout cancellation and
cover recovery synchronized between the two checks.
--
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]
