github-actions[bot] commented on code in PR #65713:
URL: https://github.com/apache/doris/pull/65713#discussion_r3594633569
##########
be/src/runtime/workload_group/workload_group.cpp:
##########
@@ -183,6 +180,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);
+ DCHECK(is_percent) << "mem_limit_str: " << mem_limit_str;
+ if (new_memory_limit != _memory_limit.load(std::memory_order_relaxed))
{
+ LOG(INFO) << fmt::format(
+ "Workload group id:{}, name:{}, "
+ "memory_limit changed from {} to {}",
+ _id, _name,
+
PrettyPrinter::print(_memory_limit.load(std::memory_order_relaxed),
+ TUnit::BYTES),
+ PrettyPrinter::print(new_memory_limit, TUnit::BYTES));
+
+ _memory_limit.store(new_memory_limit, std::memory_order_relaxed);
Review Comment:
[P1] Propagate shrunken limits when current usage is zero
This store can lower the workload-group limit, but
`refresh_workload_group_memory_state()` returns before
`update_queries_limit_()` whenever the summed query usage is zero. A registered
query can therefore keep its old, larger `MemTrackerLimiter` limit; with
reserve memory disabled, `check_limit()` checks only that stale tracker value,
so an allocation above the refreshed workload-group share can still be accepted
when process-wide headroom remains. This defeats propagation of the shrink to
that query and lets it violate the refreshed group quota until its tracker is
corrected. Please propagate query limits whenever a group limit changes even if
current usage is zero, and cover a registered zero-consumption query in the
test.
##########
be/src/runtime/workload_group/workload_group.cpp:
##########
@@ -183,6 +180,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);
+ DCHECK(is_percent) << "mem_limit_str: " << mem_limit_str;
+ if (new_memory_limit != _memory_limit.load(std::memory_order_relaxed))
{
+ LOG(INFO) << fmt::format(
+ "Workload group id:{}, name:{}, "
+ "memory_limit changed from {} to {}",
+ _id, _name,
+
PrettyPrinter::print(_memory_limit.load(std::memory_order_relaxed),
+ TUnit::BYTES),
+ PrettyPrinter::print(new_memory_limit, TUnit::BYTES));
+
+ _memory_limit.store(new_memory_limit, std::memory_order_relaxed);
Review Comment:
[P1] Restore trackers that were clamped before cgroup expansion
Raising `_memory_limit` here does not always raise existing query trackers.
Under policy `NONE`, suppose a query's user limit is `U` and the old group
limit clamped its live tracker to `L < U`. Once the cgroup grows so the new
group limit is at least `U`, `update_queries_limit_()` computes the desired
limit as `U`, but its `user_set_mem_limit() > query_weighted_mem_limit` test is
false at `U == U`, so `set_mem_limit()` is skipped and the tracker remains at
`L` across repeated maintenance passes. Existing queries can continue to fail
at the old capacity after the cgroup expands. Please set the live tracker to
the newly desired `min(user limit, weighted limit)` when it differs, and add a
shrink-then-expand regression.
--
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]
