test: stabilize flaky TestHotTestSuite/TestBuckets#10937
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
[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 |
ti-chi-bot
Bot
added
contribution
|
Hi @flaky-claw. Thanks for your PR. I'm waiting for a tikv 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. |
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
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. |
ti-chi-bot
Bot
added
the
size/XS
📝 WalkthroughWalkthrough
ChangesHot peer cache test
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/pd-ctl/tests/hot/hot_test.go`:
- Line 558: The drained write stats check in hot_test.go is vacuous because
hotStat.GetHotPeerStats(utils.Write, 0) returns an allocated map even when
empty, so reusing re.NotNil does not verify population. Update the assertion
near the existing hotStat.GetHotPeerStats(utils.Write, 0) call in the hot test
to check for a non-empty result instead, using a size/length-based assertion or
equivalent on the returned map so the test actually confirms drained write peers
were recorded.
🪄 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: e871b664-3b07-479c-bd56-d6d5a427c97a
📒 Files selected for processing (1)
tools/pd-ctl/tests/hot/hot_test.go
| // Drain async hot peer updates before TearDownTest resets the hot cache directly. | ||
| re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0)) | ||
| re.NotNil(hotStat.GetHotPeerStats(utils.Read, 0)) | ||
| re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a non-vacuous assertion for drained write stats
Line 558 duplicates Line 556, and NotNil is ineffective here because GetHotPeerStats returns an allocated map even when no hot peers exist. This does not validate the drain/population condition.
Suggested fix
- re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0))
- re.NotNil(hotStat.GetHotPeerStats(utils.Read, 0))
- re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0))
+ writeStats := hotStat.GetHotPeerStats(utils.Write, 0)
+ re.NotEmpty(writeStats)
+ found := false
+ for _, peers := range writeStats {
+ if len(peers) > 0 {
+ found = true
+ break
+ }
+ }
+ re.True(found)
+ re.NotNil(hotStat.GetHotPeerStats(utils.Read, 0))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| re.NotNil(hotStat.GetHotPeerStats(utils.Write, 0)) | |
| writeStats := hotStat.GetHotPeerStats(utils.Write, 0) | |
| re.NotEmpty(writeStats) | |
| found := false | |
| for _, peers := range writeStats { | |
| if len(peers) > 0 { | |
| found = true | |
| break | |
| } | |
| } | |
| re.True(found) | |
| re.NotNil(hotStat.GetHotPeerStats(utils.Read, 0)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/pd-ctl/tests/hot/hot_test.go` at line 558, The drained write stats
check in hot_test.go is vacuous because hotStat.GetHotPeerStats(utils.Write, 0)
returns an allocated map even when empty, so reusing re.NotNil does not verify
population. Update the assertion near the existing
hotStat.GetHotPeerStats(utils.Write, 0) call in the hot test to check for a
non-empty result instead, using a size/length-based assertion or equivalent on
the returned map so the test actually confirms drained write peers were
recorded.
1 participant
What problem does this PR solve?
Issue Number: close #10651
Problem Summary:
Flaky test
TestHotTestSuite/TestBucketsingithub.com/tikv/pd/tools/pd-ctl/tests/hotintermittently fails, so this PR stabilizes that path.What changed and how does it work?
Root Cause
TestBuckets teardown could call CleanCache while a late async write hot-cache task still touched HotPeerCache maps.
Fix
The final write drain serializes teardown behind the last write-side hot-cache task without changing CLI assertions or product behavior.
Verification
Spec:
github.com/tikv/pd/tools/pd-ctl/tests/hot :: TestHotTestSuite/TestBucketspd.issue_scoped.v1BASELINE_ONLYbaseline_onlyObserved result:
Required flaky case was not skipped.
nested_target_json passed.
race_target passed.
Gate checklist:
Commands:
cd tools && go test -json -tags without_dashboard ./pd-ctl/tests/hot -run '^TestHotTestSuite/TestBuckets$' -count=1cd tools && go test -race -tags without_dashboard ./pd-ctl/tests/hot -run '^TestHotTestSuite/TestBuckets$' -count=5Release note
Fixes #10651
Summary by CodeRabbit