KVStore: fix tiflash_raft_throughput_bytes warp problem (#10700)

[ti-chi-bot]

This is an automated cherry-pick of #10700

What problem does this PR solve?

Issue Number: close #10701

Problem Summary:

What is changed and how it works?

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No code

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Fixed an issue where Grafana's Raft throughput might incorrectly display very large values.

Summary by CodeRabbit

• Chores
  • Improved write-size tracking for write operations, consolidating payload handling to more accurately reflect throughput.
  • Unified duplicate-check behavior across store variants for consistent processing.
  • Consolidated insert and error-handling paths, preserving existing counters and metrics while simplifying internal logic.

[ti-chi-bot]

@JaySon-Huang This PR has conflicts, I have hold it.
Please resolve them or ask others to resolve them, then comment /unhold to remove the hold label.

[ti-chi-bot]

@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide.

Details

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.

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces an update_write_size helper and centralizes write_size adjustments by using payloads returned from doInsert, unifies duplicate-check selection across Raftstore versions, and consolidates Put handling paths in RaftCommands.cpp. No exported/public API changes.

Changes

Cohort / File(s)	Summary
RaftCommands change dbms/src/Storages/KVStore/MultiRaft/RaftCommands.cpp	Adds update_write_size helper to safely adjust write_size from Put payloads, replaces per-branch dup-check branching with a unified selection, consolidates Put payload handling and write_size updates after doInsert, and minor structural/comment adjustments. Review write_size edge handling and unified dup_check logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• KVStore: fix tiflash_raft_throughput_bytes warp problem #10700: Applies the same changes in RaftCommands.cpp—centralizes write_size adjustments and consolidates payload/dup_check handling.

Suggested labels

size/S

Suggested reviewers

• JinheLin

Poem

🐰 I hopped through code with nimble toes,

Found payloads shifting, off they go—
One helper now counts every bite,
Dup checks settled, tidy and right,
Metrics hum softly through the night. 🌙

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The PR description follows the template structure but lacks critical implementation details in the 'What is changed and how it works?' section.	Add detailed explanation of changes in the commit-message section and verify that the Problem Summary section contains the actual problem being solved, not just the issue reference.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing a tiflash_raft_throughput_bytes warp problem in the KVStore component, which aligns with the code modifications shown in the summary.
Linked Issues check	✅ Passed	The code changes align with fixing tiflash_raft_throughput_bytes metric issues by consolidating write_size adjustments through payload handling, though the linked issue lacks reproduction details.
Out of Scope Changes check	✅ Passed	All code changes in RaftCommands.cpp are focused on write_size metric handling and DupCheck logic, which are directly related to fixing the tiflash_raft_throughput_bytes issue.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@dbms/src/Storages/KVStore/MultiRaft/RaftCommands.cpp`:
- Around line 351-380: Remove the unresolved merge conflict markers (<<<<<<<,
=======, >>>>>>>) and keep the intended new logic block that defines
deleting_lock_keys, computes lock_count from cmds.cmd_cf, reserves
deleting_lock_keys, and defines update_write_size (handling positive and
negative payloads with the INT64_MIN-safe decrement); ensure the final code
contains only the new logic (no conflict markers) and apply the same cleanup for
the other conflicted region that mirrors this change (the block around the
update_write_size/deleting_lock_keys logic).

🧹 Nitpick comments (1)

dbms/src/Storages/KVStore/MultiRaft/RaftCommands.cpp (1)

360-379: Rename new variables to camelCase per C++ guidelines.
The newly introduced identifiers use snake_case (deleting_lock_keys, lock_count, update_write_size) but this codebase guideline requires camelCase.

♻️ Suggested rename

-    std::vector<TiKVKey> deleting_lock_keys;
-    const size_t lock_count
+    std::vector<TiKVKey> deletingLockKeys;
+    const size_t lockCount
         = std::count_if(cmds.cmd_cf, cmds.cmd_cf + cmds.len, [](auto cf) { return cf == ColumnFamilyType::Lock; });
-    deleting_lock_keys.reserve(lock_count);
-    auto update_write_size = [&write_size](Int64 payload) {
+    deletingLockKeys.reserve(lockCount);
+    auto updateWriteSize = [&write_size](Int64 payload) {
         if (payload > 0)
         {
             write_size += static_cast<size_t>(payload);
         }
         else if (payload < 0)
         {
             // A lock being rewritten could lead to a negative payload.
             // Try to turn the negative payload into a positive decrement on `write_size`.
             const auto dec = static_cast<size_t>(-(payload + 1)) + 1; // avoid INT64_MIN overflow
             if (write_size >= dec)
                 write_size -= dec;
             else
                 write_size = 0;
         }
     };
@@
-                update_write_size(payload);
+                updateWriteSize(payload);

As per coding guidelines, method and variable names in C++ must use camelCase.

Also applies to: 418-418

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [JaySon-Huang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[JaySon-Huang]

/unhold

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2026-02-05 13:43:35.639598255 +0000 UTC m=+360886.740996974: ☑️ agreed by JaySon-Huang.