[bugfix](memory) should force refresh workload group's config when memlimit changed#65542
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29375 ms |
Contributor
TPC-DS: Total hot run time: 179879 ms |
Contributor
ClickBench: Total hot run time: 24.88 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found one blocking correctness issue in the workload-group refresh change.
Critical checkpoint conclusions:
- Goal/test coverage: the PR intends to refresh workload-group config when the cgroup-derived memory limit changes without a workload-group version change, but the current control flow does not accomplish that; I did not find a test covering same-version memory-limit refresh.
- Scope/focus: the code change is small and focused, but the version fast path was not updated with the new condition.
- Concurrency/lifecycle: no separate lock-order, lifecycle, or static-initialization issue was substantiated;
get_or_create_workload_group()serializes the update path with the manager lock. - Config/compatibility/parallel paths: the affected value comes from the existing cgroup/process memory refresh path and is consumed by existing reserve, watermark, min-limit, and per-query limit paths; no wire/storage compatibility issue was found.
- Performance/observability: no separate performance or observability issue was found.
- Validation: static review only. This runner has no
.worktree_initializedmarker orthirdparty/installed, and the review prompt forbids builds or source modifications.
User focus: no additional user-provided focus points were present.
Contributor
Author
|
/review |
github-actions
Bot
removed
the
approved
Contributor
There was a problem hiding this comment.
Automated review result: request changes.
Critical checkpoint conclusions:
- Goal/test: the PR addresses the same-version cgroup-derived memory-limit refresh path, but the current predicate introduces a lower-version rollback path. No test in the PR covers same-version refresh or stale lower-version rejection.
- Scope: the source change is narrow and limited to BE workload-group update logic.
- Concurrency/order: no new local lock/lifecycle issue was found, but FE topic delivery is asynchronous and BE has no ordering guard before applying parsed workload-group info.
- Dynamic config behavior: no new config is added; the issue is in dynamic refresh of BE-derived memory bytes from
MemInfo::mem_limit(). - Compatibility/persistence/static init: no thrift, persistence, mixed-version serialization, or static-initialization issue was found.
- Parallel paths/side effects: stale topic input also feeds scheduler/cgroup and IO-throttle upserts after
check_and_update(), so the version guard needs to cover the whole update path. - Observability/performance: existing workload-group logs/metrics remain; no separate performance issue was substantiated.
- User focus: no additional user-provided focus was present.
Validation: static review only, as requested by the review prompt. Round 2 convergence subagents returned NO_NEW_VALUABLE_FINDINGS after the accepted inline issue was incorporated.
| 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 || _memory_limit != wg_info.memory_limit) { |
Contributor
There was a problem hiding this comment.
This now lets stale lower-version topic payloads roll the group back. FE publishes workload-group topics through per-BE workers and only waits for a timeout; it does not cancel a worker that is still queued or running, so an older publish can arrive after a newer version was already applied. On the BE side, parse_topic_info() recomputes wg_info.memory_limit from the current local MemInfo::mem_limit() and the topic's max_memory_percent, so a stale payload can satisfy _memory_limit != wg_info.memory_limit after a cgroup/process memory refresh or simply because the old version carried a different memory percent, even though wg_info.version < _version. This block then assigns _version = wg_info.version and overwrites the CPU, watermark, thread, IO, slot, and memory-percent fields from the stale payload; the listener also continues to call scheduler and IO-throttle upserts with that same stale info after this method returns. Please keep version ordering monotonic for the whole update path, e.g. reject < _version, update all fields/side effects for > _version, and for == _version refresh only the BE-derived memory-limit/min-limit values needed for the serverless cgroup case.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review completed for PR #65542 at head da0cfff.
I did not find a new distinct inline issue beyond the existing workload-group version-ordering thread. The current hunk now handles the equal-version cgroup/process memory-limit refresh by updating _memory_limit and recomputing _min_memory_limit, and the checked memory-enforcement paths read those refreshed fields. The remaining stale lower-version topic side-effect concern is already covered by the existing inline discussion at discussion_r3575651836; I treated the scheduler/IO-throttle and topic cleanup observations as duplicates rather than re-posting them.
Checkpoint conclusions: goal is narrow and mostly addressed for the same-version memory-limit refresh; the change is small and localized; locking is simple but now always takes the workload-group write mutex for this path; no new config, persistence, or FE-BE schema fields are added; no additional parallel code path was found beyond the listener side effects already covered by the existing thread; no build or test was run in this review-only runner.
User focus: no additional user-provided review focus.
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29544 ms |
Contributor
TPC-DS: Total hot run time: 178477 ms |
Contributor
ClickBench: Total hot run time: 24.94 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 14, 2026
…mlimit changed (#65542) ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 14, 2026
…mlimit changed (#65542) ### What problem does this PR solve? Issue Number: close #xxx Related PR: #xxx Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)