github-actions[bot] commented on code in PR #65695:
URL: https://github.com/apache/doris/pull/65695#discussion_r3594643758
##########
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};
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 =
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.
##########
be/src/runtime/workload_group/workload_group.cpp:
##########
@@ -155,10 +155,7 @@ void WorkloadGroup::check_and_update(const
WorkloadGroupInfo& wg_info) {
return;
}
std::lock_guard<std::shared_mutex> wl {_mutex};
- // 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.
- if (wg_info.version > _version ||
- (wg_info.version == _version && _memory_limit !=
wg_info.memory_limit)) {
+ if (wg_info.version > _version) {
Review Comment:
[P1] Preserve the first FE config for a version-0 `normal` group
Every BE creates its internal `normal` placeholder at version 0 with
`max_memory_percent=100`. During old-workload-group conversion,
`bindWorkloadGroupToComputeGroup()` copies the old properties into a new FE
group whose constructor also starts at version 0.
`get_or_create_workload_group()` first moves the BE placeholder to that FE id
and then reaches this check, so a migrated `normal` configured at (say) 50% is
now rejected. The new maintenance refresh then keeps recomputing from the
placeholder's stale 100% indefinitely; the removed same-version byte-difference
branch previously adopted the FE payload. Please make the first FE payload
replace the internal placeholder (or compare the incoming percentage
configuration explicitly) and cover this version-0 merge path with a
manager/listener 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]
