github-actions[bot] commented on code in PR #67846:
URL: https://github.com/apache/doris/pull/67846#discussion_r4226886917
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -585,9 +585,51 @@ void WorkloadGroupMgr::handle_paused_queries() {
++query_it;
continue;
}
+
+ // 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);
+ if
(!GlobalMemoryArbitrator::is_exceed_soft_mem_limit(test_memory_size)) {
+ LOG(INFO) << "Query: " <<
print_id(resource_ctx->task_controller()->task_id())
+ << ", process limit not exceeded now, resume
this query"
+ << ", process memory info: "
+ <<
GlobalMemoryArbitrator::process_memory_used_details_str()
+ << ", wg info: " << wg->debug_string();
+
resource_ctx->task_controller()->set_memory_sufficient(true);
+ query_it = queries_list.erase(query_it);
+ continue;
+ }
+
// If workload group's memory usage > min memory, then it
means the workload group use too much memory
// in memory contention state. Should just spill
- if (wg->total_mem_used() > wg->min_memory_limit()) {
+ bool handle_query_now = wg->total_mem_used() >
wg->min_memory_limit();
+ if (!handle_query_now) {
+ // Other workload groups many use a lot of memory, should
revoke memory from other workload groups
+ // by cancelling their queries.
+ int64_t revoked_size = revoke_memory_from_other_groups_();
Review Comment:
[P2] Try other over-minimum groups before falling back to this query. If
group A is 140 MiB above its minimum with eight 30 MiB queries,
`EXCLUDE_IS_SMALL` makes its `revoke_memory()` return zero. Group B can be 130
MiB above its minimum with a revocable 230 MiB query, but
`revoke_memory_from_other_groups_()` only tries A. This fallback then cancels a
below-minimum query at timeout or the hard limit while B could relieve the
process pressure. Try the remaining eligible groups before treating zero from A
as no reclaimable memory.
##########
be/src/runtime/workload_group/workload_group_manager.cpp:
##########
@@ -585,9 +585,51 @@ void WorkloadGroupMgr::handle_paused_queries() {
++query_it;
continue;
}
+
+ // 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);
+ if
(!GlobalMemoryArbitrator::is_exceed_soft_mem_limit(test_memory_size)) {
Review Comment:
[P2] Preserve the process-pressure deadline across the global revocation
wake. A below-minimum query can wait 59 seconds, then cancel a newly eligible
peer whose memory remains tracked. The earlier Phase 3 wakes and erases this
query without checking pressure; its failed retry creates a fresh
`PausedQuery`, so the new 60-second timer starts over. That peer can be counted
again for three seconds, repeatedly bypassing the timeout and hard-limit
checks. Recheck pressure before waking process-paused queries and carry their
original deadline across retries.
##########
be/test/runtime/workload_group/workload_group_manager_test.cpp:
##########
@@ -691,15 +716,17 @@ TEST_F(WorkloadGroupManagerTest, ProcessMemoryNotEnough) {
wg3->refresh_memory_usage();
EXPECT_EQ(wg3->total_mem_used(), 1024 * 1024 * 500); // WG3 exceed 400MB
- EXPECT_EQ(40 * 1024 * 1024,
_wg_manager->revoke_memory_from_other_groups_());
+ // WG3 exceeds most, 40MB should be revoked, and query31 (500MB) is
cancelled for it.
+ EXPECT_EQ(500 * 1024 * 1024,
_wg_manager->revoke_memory_from_other_groups_());
wg1->refresh_memory_usage();
wg2->refresh_memory_usage();
wg3->refresh_memory_usage();
ASSERT_TRUE(query_context31->is_cancelled());
- // query31 is still in wg3, so that it is not cancel again.
- EXPECT_EQ(40 * 1024 * 1024,
_wg_manager->revoke_memory_from_other_groups_());
+ // query31 is still in wg3 and was cancelled recently, so that it is not
cancelled again,
+ // but its memory is still counted as being released.
+ EXPECT_EQ(500 * 1024 * 1024,
_wg_manager->revoke_memory_from_other_groups_());
Review Comment:
[P2] Remove the wall-time dependency from this repeat-call assertion. The
first call cancels `query_context31`, but `revoke_tasks_memory()` counts its
500 MiB on the next call only until `revoke_memory_max_tolerance_ms` elapses (3
seconds by default). If a loaded runner pauses this test between the calls, WG3
is still selected but returns zero, so this assertion fails even though the
reclamation code behaves correctly. Control cancellation age or assert this
path without requiring the real-time window.
--
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]
