Fix/ruv2 executor accounting upstream#68577
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
|
Hi @meiking. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Welcome @meiking! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Hi @meiking. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR refactors RUv2 insert-row metrics from statement-level idempotent snapshots to delta-based per-batch recording, measuring insert rows multiplied by inserted column count, with updated documentation, new stateless recording helpers, wiring through insert execution paths, and expanded executor type mappings. ChangesRUv2 Insert-Row Metrics Recording Refactor
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
What problem does this PR solve?
Issue Number: ref #68576
Problem Summary:
RU v2 executor accounting may undercount TiDB-side compute cost for some common executors because the executor-level mapping relies on concrete type strings, and some entries no longer match the current executor package/type names. In addition, the insert-specific L5 metric should account for inserted rows multiplied by inserted column count, but the old path used affected rows, which does not match the fitted
executor-l5-insert-rowsmodel input.What changed and how does it work?
InsertValuesasrowCount * len(insertColumns)after successful insert execution, including batched insert and insert-select paths.executor-l5-insert-rowsconfig/metric comments to describe row-column multiplication.pkg/executor/internal/exectests.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.