KVStore: reduce log#10813
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
📝 WalkthroughWalkthroughThe KVStore persistence APIs switch extra-message parameters from C-style Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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
added
approved
needs-1-more-lgtm
![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
🧹 Nitpick comments (1)
dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp (1)
38-47: Minor: simplifycallerconstruction.
fmt::format("{}", PersistRegionReasonMap[reason_id])is an unnecessary alloc+format step over a plainconst char *. You can collapse it to a single conditional expression and avoid the intermediate string whenextra_msgis empty.♻️ Proposed refactor
auto reason_id = magic_enum::enum_underlying(reason); - std::string caller = fmt::format("{}", PersistRegionReasonMap[reason_id]); - if (!extra_msg.empty()) - caller = fmt::format("{} {}", caller, extra_msg); + const char * reason_name = PersistRegionReasonMap[reason_id]; + std::string caller = extra_msg.empty() + ? std::string(reason_name) + : fmt::format("{} {}", reason_name, extra_msg);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp` around lines 38 - 47, The current construction of caller does an unnecessary fmt::format of PersistRegionReasonMap[reason_id] and then conditionally appends extra_msg; instead, fetch the base C-string from PersistRegionReasonMap using reason_id and build caller only when extra_msg is non-empty (e.g., const char* base = PersistRegionReasonMap[reason_id]; if extra_msg.empty() caller = base; else caller = fmt::format("{} {}", base, extra_msg)); update the code that logs the message (LOG_INFO using region.getDebugString(), region.dataSize(), caller) to use this simplified caller construction so we avoid the extra allocation/format when extra_msg is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp`:
- Line 31: The call in RaftCommandsKVS.cpp passes
raft_cmdpb::AdminCmdType_Name(cmd_type).c_str() into persistRegion which
converts to a std::string_view, creating a dangling pointer because
AdminCmdType_Name returns a temporary std::string; fix by passing the
std::string result directly or storing it in a local std::string variable before
calling persistRegion (e.g., auto extra =
raft_cmdpb::AdminCmdType_Name(cmd_type); then pass extra or pass extra as
std::string_view) so persistRegion receives a stable string lifetime; ensure
changes reference the persistRegion call and AdminCmdType_Name usage.
---
Nitpick comments:
In `@dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp`:
- Around line 38-47: The current construction of caller does an unnecessary
fmt::format of PersistRegionReasonMap[reason_id] and then conditionally appends
extra_msg; instead, fetch the base C-string from PersistRegionReasonMap using
reason_id and build caller only when extra_msg is non-empty (e.g., const char*
base = PersistRegionReasonMap[reason_id]; if extra_msg.empty() caller = base;
else caller = fmt::format("{} {}", base, extra_msg)); update the code that logs
the message (LOG_INFO using region.getDebugString(), region.dataSize(), caller)
to use this simplified caller construction so we avoid the extra
allocation/format when extra_msg is empty.
🪄 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: b2c35c4b-35c0-430e-8304-b59f5e1672e8
📒 Files selected for processing (2)
dbms/src/Storages/KVStore/KVStore.hdbms/src/Storages/KVStore/MultiRaft/Persistence.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp (1)
39-41: Minor: collapse the caller-string construction into a single allocation.
fmt::format("{}", PersistRegionReasonMap[reason_id])is an extra allocation for a no-op format, and the re-assignment on line 41 pays for another temporary. You can produce the final string in one step.♻️ Proposed simplification
- auto reason_id = magic_enum::enum_underlying(reason); - std::string caller = fmt::format("{}", PersistRegionReasonMap[reason_id]); - if (!extra_msg.empty()) - caller = fmt::format("{} {}", caller, extra_msg); + auto reason_id = magic_enum::enum_underlying(reason); + const char * reason_str = PersistRegionReasonMap[reason_id]; + std::string caller = extra_msg.empty() + ? std::string(reason_str) + : fmt::format("{} {}", reason_str, extra_msg);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp` around lines 39 - 41, Collapse the two-step caller construction into a single allocation by calling fmt::format once to produce the final string: use PersistRegionReasonMap[reason_id] as the first component and append extra_msg only when non-empty (inserting a preceding space only if extra_msg is present), replacing the current two statements that set caller and then reassign it when extra_msg is non-empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dbms/src/Storages/KVStore/MultiRaft/Persistence.cpp`:
- Around line 39-41: Collapse the two-step caller construction into a single
allocation by calling fmt::format once to produce the final string: use
PersistRegionReasonMap[reason_id] as the first component and append extra_msg
only when non-empty (inserting a preceding space only if extra_msg is present),
replacing the current two statements that set caller and then reassign it when
extra_msg is non-empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5fe60d4d-7cf8-4cf9-bb26-6b2d831f4c88
📒 Files selected for processing (2)
dbms/src/Storages/KVStore/KVStore.hdbms/src/Storages/KVStore/MultiRaft/Persistence.cpp
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, JinheLin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/cherry-pick release-8.5 |
|
@JaySon-Huang: new pull request created to branch DetailsIn response to this:
Instructions 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 ti-community-infra/tichi repository. |
4 participants
What problem does this PR solve?
Issue Number: ref #10814
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit