executor, variable: support cop_mvcc_read_amplification to trigger printing slow log (#66064)#66232
Conversation
|
@zimulala This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
|
@ti-chi-bot: 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. |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> (cherry picked from commit 9dec7f6)
9dec7f6 to
373e060
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new slow-log rule field Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/sessionctx/variable/slow_log.go (1)
193-194: Document the zero-threshold fallback on the exported field.
cop_mvcc_read_amplificationnow has a non-obvious contract: missingScanDetailorProcessedKeys <= 0only matches threshold0. Please capture that on the exported comment so callers do not have to read the matcher to understand the SQL-visible behavior.As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs" and "Keep exported-symbol doc comments, and prefer semantic constraints over name restatement."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sessionctx/variable/slow_log.go` around lines 193 - 194, Update the exported doc comment for SlowLogCopMVCCReadAmplification to state its non-obvious contract: when the ScanDetail is nil/missing or ScanDetail.ProcessedKeys <= 0, the matcher treats the amplification as matching only threshold 0 (i.e., there is a zero-threshold fallback), and otherwise it computes total_keys/processed_keys; reference ScanDetail and ProcessedKeys in the comment so callers understand the SQL-visible behavior without inspecting the matcher implementation.pkg/executor/adapter_test.go (1)
145-146: Add one case where this is the only effective field.These assertions prove the matcher, but they still don't cover lazy preparation for the new accessor: the earlier prep test gets
items.ExecDetailfromProcess_time, and this subtest pre-populatesitems.ExecDetailmanually. A small case with onlycop_mvcc_read_amplificationin the effective field set would cover the actual integration point.Also applies to: 341-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/adapter_test.go` around lines 145 - 146, Add a subtest that exercises lazy preparation by making cop_mvcc_read_amplification the only effective field: use variable.SlowLogRuleFieldAccessors[strings.ToLower(variable.SlowLogCopMVCCReadAmplification)] and call Match with a items value whose ExecDetail is either nil or manually populated, and with the effective field set containing only "cop_mvcc_read_amplification"; assert True for value < 0.5 and False for >= 0.5 to prove the accessor works when it is the sole effective field and triggers the lazy ExecDetail prep path. Ensure you reference Match, SlowLogCopMVCCReadAmplification, variable.SlowLogRuleFieldAccessors and items.ExecDetail in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/executor/adapter_test.go`:
- Around line 145-146: Add a subtest that exercises lazy preparation by making
cop_mvcc_read_amplification the only effective field: use
variable.SlowLogRuleFieldAccessors[strings.ToLower(variable.SlowLogCopMVCCReadAmplification)]
and call Match with a items value whose ExecDetail is either nil or manually
populated, and with the effective field set containing only
"cop_mvcc_read_amplification"; assert True for value < 0.5 and False for >= 0.5
to prove the accessor works when it is the sole effective field and triggers the
lazy ExecDetail prep path. Ensure you reference Match,
SlowLogCopMVCCReadAmplification, variable.SlowLogRuleFieldAccessors and
items.ExecDetail in the test.
In `@pkg/sessionctx/variable/slow_log.go`:
- Around line 193-194: Update the exported doc comment for
SlowLogCopMVCCReadAmplification to state its non-obvious contract: when the
ScanDetail is nil/missing or ScanDetail.ProcessedKeys <= 0, the matcher treats
the amplification as matching only threshold 0 (i.e., there is a zero-threshold
fallback), and otherwise it computes total_keys/processed_keys; reference
ScanDetail and ProcessedKeys in the comment so callers understand the
SQL-visible behavior without inspecting the matcher implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5c836734-bf81-4572-b2a8-be64e7ff788b
📒 Files selected for processing (3)
pkg/executor/adapter_test.gopkg/sessionctx/variable/slow_log.gopkg/sessionctx/variable/slow_log_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #66232 +/- ##
================================================
Coverage ? 55.2396%
================================================
Files ? 1821
Lines ? 654195
Branches ? 0
================================================
Hits ? 361375
Misses ? 266220
Partials ? 26600
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: XuHuaiyu, yibin87 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 |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/unhold |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
This is an automated cherry-pick of #66064
What problem does this PR solve?
Issue Number: close #48947, related #62959
Problem Summary:
tidb_slow_log_rulessupports multi-dimensional triggering, but it cannot trigger by MVCC read amplification (total_keys/processed_keys). This PR adds a dedicated rule item to support that use case.What changed and how does it work?
Add new slow log rule field:
cop_mvcc_read_amplificationBehavior:
Total_keys/Process_keys>= threshold.ScanDetail== nil orProcessedKeys<= 0), use stable scheme A:Code changes:
PrepareSlowLogItemsForRules/ShouldWriteSlowLog), new field works through existing accessor mechanism.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Improvements
Tests