util: aggregate write RUv2 into commit details#1939
util: aggregate write RUv2 into commit details#1939ti-chi-bot[bot] merged 4 commits intotikv:masterfrom
Conversation
Signed-off-by: disksing <i@disksing.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded safe cloning and aggregation of Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
util/execdetails.go (1)
249-275:⚠️ Potential issue | 🟡 Minor
PrewriteReqNumis still dropped inMergeandClone.This change preserves
WriteRUV2, butPrewriteReqNumis still neither accumulated nor copied. Any merged or clonedCommitDetailswill silently lose that counter.🐛 Proposed fix
func (cd *CommitDetails) Merge(other *CommitDetails) { cd.ResolveLock.ResolveLockTime += other.ResolveLock.ResolveLockTime cd.WriteKeys += other.WriteKeys cd.WriteSize += other.WriteSize cd.PrewriteRegionNum += other.PrewriteRegionNum + cd.PrewriteReqNum += other.PrewriteReqNum cd.TxnRetry += other.TxnRetry cd.Mu.CommitBackoffTime += other.Mu.CommitBackoffTime @@ commit := &CommitDetails{ GetCommitTsTime: cd.GetCommitTsTime, GetLatestTsTime: cd.GetLatestTsTime, LagDetails: cd.LagDetails, PrewriteTime: cd.PrewriteTime, @@ WriteKeys: cd.WriteKeys, WriteSize: cd.WriteSize, PrewriteRegionNum: cd.PrewriteRegionNum, + PrewriteReqNum: cd.PrewriteReqNum, TxnRetry: cd.TxnRetry, ResolveLock: cd.ResolveLock, }Also applies to: 331-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/execdetails.go` around lines 249 - 275, The Merge implementation for CommitDetails is missing aggregation of the PrewriteReqNum counter and the Clone method likewise doesn't copy it, causing that metric to be lost; update CommitDetails.Merge to add cd.PrewriteReqNum += other.PrewriteReqNum (alongside the other += accumulations) and update CommitDetails.Clone to set the cloned object's PrewriteReqNum = original.PrewriteReqNum so clones preserve the value (refer to the methods named CommitDetails.Merge and CommitDetails.Clone and the field PrewriteReqNum).
🧹 Nitpick comments (1)
util/execdetails_test.go (1)
294-307: Also mutateExecutorInputsin the copy-isolation checks.
cloneRUV2()'s tricky part is the nestedExecutorInputspointer, but these assertions only modify top-level scalars. An aliasing regression there would still pass.🧪 Suggested test additions
cloned.Mu.WriteRUV2.KvEngineCacheMiss = 999 assert.Equal(t, uint64(1), orig.Mu.WriteRUV2.KvEngineCacheMiss) + cloned.Mu.WriteRUV2.ExecutorInputs.TikvCoprocessorExecutorWorkTotalBatchIndexScan = 999 + assert.Equal(t, uint64(6), orig.Mu.WriteRUV2.ExecutorInputs.TikvCoprocessorExecutorWorkTotalBatchIndexScan) @@ agg.KvEngineCacheMiss = 999 assert.Equal(t, uint64(111), commitDetails.GetWriteRUV2().KvEngineCacheMiss) + agg.ExecutorInputs.TikvCoprocessorExecutorWorkTotalBatchIndexScan = 999 + assert.Equal(t, uint64(444), commitDetails.GetWriteRUV2().ExecutorInputs.TikvCoprocessorExecutorWorkTotalBatchIndexScan)Also applies to: 318-332
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@util/execdetails_test.go` around lines 294 - 307, The test misses verifying deep-copy isolation for the nested ExecutorInputs pointer in cloneRUV2; update the copy-isolation checks to mutate cloned.Mu.WriteRUV2.ExecutorInputs (e.g., change a field or append/remove an entry) and then assert orig.Mu.WriteRUV2.ExecutorInputs remains unchanged, and apply the same additional assertion in the second test block (the similar checks around lines 318-332) so the tests will catch aliasing of the nested ExecutorInputs pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@util/execdetails.go`:
- Around line 249-275: The Merge implementation for CommitDetails is missing
aggregation of the PrewriteReqNum counter and the Clone method likewise doesn't
copy it, causing that metric to be lost; update CommitDetails.Merge to add
cd.PrewriteReqNum += other.PrewriteReqNum (alongside the other += accumulations)
and update CommitDetails.Clone to set the cloned object's PrewriteReqNum =
original.PrewriteReqNum so clones preserve the value (refer to the methods named
CommitDetails.Merge and CommitDetails.Clone and the field PrewriteReqNum).
---
Nitpick comments:
In `@util/execdetails_test.go`:
- Around line 294-307: The test misses verifying deep-copy isolation for the
nested ExecutorInputs pointer in cloneRUV2; update the copy-isolation checks to
mutate cloned.Mu.WriteRUV2.ExecutorInputs (e.g., change a field or append/remove
an entry) and then assert orig.Mu.WriteRUV2.ExecutorInputs remains unchanged,
and apply the same additional assertion in the second test block (the similar
checks around lines 318-332) so the tests will catch aliasing of the nested
ExecutorInputs pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4f7ed72-16d5-4c75-b87a-19ae81479152
📒 Files selected for processing (2)
util/execdetails.goutil/execdetails_test.go
Signed-off-by: disksing <i@disksing.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@integration_tests/go.mod`:
- Around line 180-181: Add a short explanatory comment near the replace
directives in integration_tests/go.mod (the lines redirecting
github.com/pingcap/tidb and github.com/pingcap/tidb/pkg/parser to
github.com/oh-my-tidb/tidb) stating that these replaces are temporary for
testing the commit-details feature against the oh-my-tidb fork, and note that
they should be removed once upstream TiDB includes the commit-details changes;
alternatively add the same note to the integration test setup README if you
prefer docs over inline comments.
- Line 20: Update the grpc module version in go.mod from "google.golang.org/grpc
v1.75.1" to "google.golang.org/grpc v1.79.3" (or later) to remediate the
GHSA-p77j-4mvh-x3m3 authorization bypass; after changing the module line, run
"go mod tidy" (and optionally "go test ./...") to refresh the dependency graph
and ensure no other modules pin the older grpc version.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: adafe0a0-04b0-4e26-9c91-2e194003e093
⛔ Files ignored due to path filters (1)
integration_tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
integration_tests/go.mod
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ekexium, nolouch 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 |
This reverts commit d1ffcbb. Signed-off-by: disksing <i@disksing.com>
Summary
CommitDetailsRUV2instead of the wholeExecDetailsV2GetWriteRUV2()accessor for TiDB-side statement metrics aggregationTests
go test ./util -run 'TestCommitDetails|TestLockKeysDetails' -count=1Summary by CodeRabbit
Chores
Tests