*: enhance mem tracker exclude rss_file & check limit for columnar#10867
*: enhance mem tracker exclude rss_file & check limit for columnar#10867yongman wants to merge 10 commits into
Conversation
Signed-off-by: yongman <yming0221@gmail.com> (cherry picked from commit d54c26a4afa31b362c538394190f29299b94970c)
Signed-off-by: yongman <yming0221@gmail.com> (cherry picked from commit ec4bf8011fa46f8c9750594a01f7303f7bd96689)
Signed-off-by: yongman <yming0221@gmail.com> (cherry picked from commit d946399942e70bf0d8aa1b7567ad5e498108f2e2)
Signed-off-by: yongman <yming0221@gmail.com> (cherry picked from commit 66c3e5a377acb05cbb9cd819e06b0fc94d8ada9a)
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR separates file-backed RSS from anonymous RSS, records rss_file via process metrics, computes effective RSS = max(real_rss - real_rss_file, 0), centralizes RSS-limit checks, integrates the check into allocation and storage read paths, and advances jemalloc epoch before metrics collection. ChangesRSS-aware memory tracking and limit enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
@yongman I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dbms/src/Common/MemoryTracker.cpp`:
- Line 190: The call to checkRssLimitImpl(...) (e.g., checkRssLimitImpl(/*
require_tracked_growth */ true, will_be, size)) can throw after tracked bytes
were already incremented, so you must roll back the tracked increase on
exception; wrap the checkRssLimitImpl call in a try/catch and on catch subtract
the previously added amount (e.g., call the corresponding decrease/undo method
or subtract from tracked_bytes by size) before rethrowing; apply the same
rollback pattern to the other allocation paths referenced (the block around
lines 235–290) so any early abort always undoes the tracked increment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fd99563b-e41a-4731-b09c-3a308a1afd56
📒 Files selected for processing (6)
dbms/src/Common/BackgroundTask.cppdbms/src/Common/MemoryTracker.cppdbms/src/Common/MemoryTracker.hdbms/src/Interpreters/AsynchronousMetrics.cppdbms/src/Interpreters/Settings.hdbms/src/Storages/StorageDisaggregatedColumnar.cpp
Signed-off-by: yongman <yming0221@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Lloyd-Pottiger The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp`:
- Line 850: The Exception constructor at the throw statement with "lock error"
message and ErrorCodes::COLUMNAR_SNAPSHOT_ERROR is using incorrect argument
order. Reorder the arguments so that the error code
ErrorCodes::COLUMNAR_SNAPSHOT_ERROR comes first as the first parameter, followed
by the message string "lock error" as the second parameter, to follow the
fmt-style Exception constructor pattern required by the coding guidelines.
- Line 1465: The Exception throw statement is using incorrect parameter order.
According to coding guidelines, the fmt-style Exception constructor requires the
error code as the first parameter followed by the message string. Locate the
Exception throw statement containing "read_block failed in columnar" and swap
the parameter order so that ErrorCodes::LOGICAL_ERROR appears first and the
message string appears second.
- Line 1635: The `likely` builtin macro at line 1635 has incorrect syntax. The
condition check currently reads `if likely (block && block.rows() > 0)` but
needs to properly wrap the condition within the `likely` macro's parentheses.
Fix the syntax by changing it to `if (likely(block && block.rows() > 0))` to
ensure the likely macro receives its argument with correct parentheses syntax.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c2c483e8-ecfa-42d0-9080-91a7e3ad222c
📒 Files selected for processing (1)
dbms/src/Storages/StorageDisaggregatedColumnar.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp`:
- Line 850: The Exception constructor at the throw statement with "lock error"
message and ErrorCodes::COLUMNAR_SNAPSHOT_ERROR is using incorrect argument
order. Reorder the arguments so that the error code
ErrorCodes::COLUMNAR_SNAPSHOT_ERROR comes first as the first parameter, followed
by the message string "lock error" as the second parameter, to follow the
fmt-style Exception constructor pattern required by the coding guidelines.
- Line 1465: The Exception throw statement is using incorrect parameter order.
According to coding guidelines, the fmt-style Exception constructor requires the
error code as the first parameter followed by the message string. Locate the
Exception throw statement containing "read_block failed in columnar" and swap
the parameter order so that ErrorCodes::LOGICAL_ERROR appears first and the
message string appears second.
- Line 1635: The `likely` builtin macro at line 1635 has incorrect syntax. The
condition check currently reads `if likely (block && block.rows() > 0)` but
needs to properly wrap the condition within the `likely` macro's parentheses.
Fix the syntax by changing it to `if (likely(block && block.rows() > 0))` to
ensure the likely macro receives its argument with correct parentheses syntax.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c2c483e8-ecfa-42d0-9080-91a7e3ad222c
📒 Files selected for processing (1)
dbms/src/Storages/StorageDisaggregatedColumnar.cpp
🛑 Comments failed to post (3)
dbms/src/Storages/StorageDisaggregatedColumnar.cpp (3)
850-850:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse fmt-style Exception constructor.
As per coding guidelines, Exception should use fmt-style constructor with error code first:
throw Exception(ErrorCodes::SOME_CODE, "message");🔧 Proposed fix
- throw Exception("lock error", ErrorCodes::COLUMNAR_SNAPSHOT_ERROR); + throw Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "lock error");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.throw Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "lock error");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp` at line 850, The Exception constructor at the throw statement with "lock error" message and ErrorCodes::COLUMNAR_SNAPSHOT_ERROR is using incorrect argument order. Reorder the arguments so that the error code ErrorCodes::COLUMNAR_SNAPSHOT_ERROR comes first as the first parameter, followed by the message string "lock error" as the second parameter, to follow the fmt-style Exception constructor pattern required by the coding guidelines.Source: Coding guidelines
1465-1465:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse fmt-style Exception constructor.
As per coding guidelines, Exception should use fmt-style constructor with error code first:
throw Exception(ErrorCodes::SOME_CODE, "message");🔧 Proposed fix
- throw Exception("read_block failed in columnar", ErrorCodes::LOGICAL_ERROR); + throw Exception(ErrorCodes::LOGICAL_ERROR, "read_block failed in columnar");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.throw Exception(ErrorCodes::LOGICAL_ERROR, "read_block failed in columnar");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp` at line 1465, The Exception throw statement is using incorrect parameter order. According to coding guidelines, the fmt-style Exception constructor requires the error code as the first parameter followed by the message string. Locate the Exception throw statement containing "read_block failed in columnar" and swap the parameter order so that ErrorCodes::LOGICAL_ERROR appears first and the message string appears second.Source: Coding guidelines
1635-1635:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix syntax error with likely macro.
The
likelybuiltin requires proper parentheses syntax. The current code will fail to compile.🐛 Proposed fix
- if likely (block && block.rows() > 0) + if (likely(block && block.rows() > 0))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (likely(block && block.rows() > 0))🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 1635-1635: syntax error
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp` at line 1635, The `likely` builtin macro at line 1635 has incorrect syntax. The condition check currently reads `if likely (block && block.rows() > 0)` but needs to properly wrap the condition within the `likely` macro's parentheses. Fix the syntax by changing it to `if (likely(block && block.rows() > 0))` to ensure the likely macro receives its argument with correct parentheses syntax.Source: Linters/SAST tools
|
/retest-required |
|
@yongman: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
2 participants
What problem does this PR solve?
Issue Number: ref #10844
Problem Summary:
What is changed and how it works?
rss_filepart can be auto freed by OS.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Chores