test: stabilize flaky TestFlashbackClusterWithManyDBs#70057
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
the
size/L
📝 WalkthroughWalkthroughThe recovery test now constructs a TiKV-mocked flashback cluster with atomic safe-timestamp control, session and resource-manager setup, RPC interception, and explicit cleanup. Bazel dependencies and test imports were updated accordingly. ChangesFlashback cluster test stabilization
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🧹 Nitpick comments (1)
pkg/executor/test/recovertest/recover_test.go (1)
748-793: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore the schema lease after the test instead of leaving it mutated globally.
vardef.SetSchemaLease(500 * time.Millisecond)at line 778 permanently overwrites the global schema lease with no backup/restore, unlike thekv.TxnEntrySizeLimitbackup pattern used a few lines later inTestFlashbackClusterWithManyDBs(Lines 814-818: save old value,t.Cleanuprestores it). Since this is a global package-level setting, it can leak into other tests executed later in the same test binary/package, undermining the test-isolation goal of this flakiness fix.🔧 Proposed fix
+ prevLease := vardef.SchemaLease.Load() vardef.SetSchemaLease(500 * time.Millisecond) session.DisableStats4Test() domain.DisablePlanReplayerBackgroundJob4Test() domain.DisableDumpHistoricalStats4Test() dom, err := session.BootstrapSession(wrappedStore) require.NoError(t, err) dom.SetStatsUpdating(true) t.Cleanup(func() { dom.Close() view.Stop() require.NoError(t, wrappedStore.Close()) resourcemanager.InstanceResourceManager.Reset() + vardef.SetSchemaLease(prevLease) })(Adjust the getter to whatever accessor
vardefexposes for the current lease value.)As per coding guidelines, "Go code: Follow existing package-local conventions first and keep style consistent with nearby files."
🤖 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 `@pkg/executor/test/recovertest/recover_test.go` around lines 748 - 793, Update newFlashbackClusterTestStore to capture the current vardef schema lease before calling vardef.SetSchemaLease, then register a t.Cleanup callback that restores the captured value. Keep the 500-millisecond lease during the test and follow the existing backup/restore pattern used by TestFlashbackClusterWithManyDBs, using vardef’s available getter.
🤖 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 `@pkg/executor/test/recovertest/BUILD.bazel`:
- Around line 15-16: Reorder the deps entries in the test target’s BUILD
definition so //pkg/ddl/util appears before //pkg/domain, preserving Gazelle’s
lexical alphabetical ordering and leaving all other dependencies unchanged.
---
Nitpick comments:
In `@pkg/executor/test/recovertest/recover_test.go`:
- Around line 748-793: Update newFlashbackClusterTestStore to capture the
current vardef schema lease before calling vardef.SetSchemaLease, then register
a t.Cleanup callback that restores the captured value. Keep the 500-millisecond
lease during the test and follow the existing backup/restore pattern used by
TestFlashbackClusterWithManyDBs, using vardef’s available getter.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d5a193c-7822-4236-8be2-da23da0c7db8
📒 Files selected for processing (2)
pkg/executor/test/recovertest/BUILD.bazelpkg/executor/test/recovertest/recover_test.go
| "//pkg/domain", | ||
| "//pkg/ddl/util", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Dep list out of alphabetical order — likely cause of the bazel_prepare failure.
//pkg/domain (line 15) is listed before //pkg/ddl/util (line 16), but "ddl" sorts before "domain" lexically. Gazelle-generated deps lists are alphabetically sorted, and the PR description notes bazel_prepare failed — this ordering mismatch is a likely culprit.
🔧 Proposed fix
"//pkg/config",
"//pkg/config/kerneltype",
- "//pkg/domain",
"//pkg/ddl/util",
+ "//pkg/domain",
"//pkg/errno",📝 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.
| "//pkg/domain", | |
| "//pkg/ddl/util", | |
| "//pkg/ddl/util", | |
| "//pkg/domain", |
🤖 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 `@pkg/executor/test/recovertest/BUILD.bazel` around lines 15 - 16, Reorder the
deps entries in the test target’s BUILD definition so //pkg/ddl/util appears
before //pkg/domain, preserving Gazelle’s lexical alphabetical ordering and
leaving all other dependencies unchanged.
|
@flaky-claw: 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #70057 +/- ##
================================================
- Coverage 76.3266% 73.9381% -2.3885%
================================================
Files 2041 2058 +17
Lines 559579 578881 +19302
================================================
+ Hits 427108 428014 +906
- Misses 131570 150512 +18942
+ Partials 901 355 -546
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
What problem does this PR solve?
Issue Number: close #68328
Problem Summary:
Flaky test
TestFlashbackClusterWithManyDBsinpkg/executor/test/recovertestintermittently fails, so this PR stabilizes that path.What changed and how does it work?
Root Cause
TestFlashbackClusterWithManyDBs relied on failpoint-only flashback bypasses while raw validation used the real non-TiKV guard.
Fix
A mock TiKV-shaped store preserves the flashback-cluster DDL path and removes the target test’s failpoint harness dependency.
Verification
Spec:
pkg/executor/test/recovertest :: TestFlashbackClusterWithManyDBstidb.go_flaky.defaultBASELINE_ONLYGO_TEST_WITH_TAGSintest, deadlockbaseline_onlyObserved result:
Required flaky case was not skipped.
target_acceptance passed.
build passed.
ready_lint passed.
Gate checklist:
Commands:
go test -json -tags=intest,deadlock ./pkg/executor/test/recovertest -run '^TestFlashbackClusterWithManyDBs$' -count=1make buildmake lintmake bazel_prepareCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Fixes #68328
Summary by CodeRabbit