github-actions[bot] commented on code in PR #65695:
URL: https://github.com/apache/doris/pull/65695#discussion_r3594649286
##########
be/src/runtime/workload_group/workload_group.cpp:
##########
@@ -184,6 +181,38 @@ void WorkloadGroup::check_and_update(const
WorkloadGroupInfo& wg_info) {
// MemtrackerLimiter is not removed during query context release, so that
should remove it here.
int64_t WorkloadGroup::refresh_memory_usage() {
+ {
+ // In serverless mode, user may modify cgroup's memory limit directly
and workload group's config
+ // is not changed. So that we should update workload group's config
ignore version.
+ std::lock_guard<std::shared_mutex> wl {_mutex};
+ const int max_memory_percent =
_max_memory_percent.load(std::memory_order_relaxed);
Review Comment:
[P2] Keep the remaining direct-constructor tests independent of host memory
This makes `_max_memory_percent` authoritative on every refresh, so a test
that directly sets `WorkloadGroupInfo.memory_limit=200MB` but leaves the
percentage at its 100% default is silently switched to the runner's
`MemInfo::mem_limit()`. The patch applies a matching test limit to several
adjacent cases, but `update_queries_limit_restores_limit_none_policy`,
`update_queries_limit_restores_limit_dynamic_policy`, and
`limit_increases_when_concurrency_decreases` still refresh such groups. They
now depend on runner memory and no longer exercise the documented 200 MB state.
Please apply the same scoped `MemInfo` setup/matching percentage to all
affected tests, or construct them through the percentage-backed parser.
##########
be/src/runtime/workload_group/workload_group.cpp:
##########
@@ -184,6 +181,38 @@ void WorkloadGroup::check_and_update(const
WorkloadGroupInfo& wg_info) {
// MemtrackerLimiter is not removed during query context release, so that
should remove it here.
int64_t WorkloadGroup::refresh_memory_usage() {
+ {
+ // In serverless mode, user may modify cgroup's memory limit directly
and workload group's config
+ // is not changed. So that we should update workload group's config
ignore version.
+ std::lock_guard<std::shared_mutex> wl {_mutex};
+ const int max_memory_percent =
_max_memory_percent.load(std::memory_order_relaxed);
+ const std::string mem_limit_str = fmt::format("{}%",
max_memory_percent);
+ bool is_percent = true;
+ const int64_t new_memory_limit =
+ ParseUtil::parse_mem_spec(mem_limit_str, -1,
MemInfo::mem_limit(), &is_percent);
Review Comment:
[P1] Do not treat a failed cgroup sample as a limit increase
`MemInfo::refresh_proc_meminfo()` marks the cgroup sample invalid when
either the limit or usage read fails; a limit-read failure also replaces the
last limit with `INT64_MAX` and suppresses another limit read for thousands of
maintenance iterations. In that state it recomputes `MemInfo::mem_limit()` from
host `MemTotal`. Because the daemon refreshes `MemInfo` immediately before this
call, this new block now expands every workload-group and per-query limit in
the same 50 ms pass even though the container's lower limit is still active,
creating a kernel-OOM window. Please key this propagation to a valid limit
generation/sample (or preserve the last successful finite limit across read
failures) and add a failure/recovery test.
--
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]
