Deflake slot migration waits by using the suite's 50s bound - #4490
Deflake slot migration waits by using the suite's 50s bound#4490KewangZhili wants to merge 1 commit into
Conversation
wait_for_migration, wait_for_migration_field and wait_for_countkeysinslot wait on work that completes asynchronously across the cluster, but each allowed only 10s. The rest of the cluster suite allows 50s for cluster-wide conditions, and this same file already uses 50s for its other waits, so these three were the outliers. Both reported failures show the operation in progress rather than wrong, having simply not finished inside 10s on the slowest CI configuration: valkey-io#4326 wait_for_migration: the source still owned slot 16383, so ownership had not transferred yet. valkey-io#4311 wait_for_migration_field: state was reading-establish-response, an intermediate state on the way to the expected one. Raise the three helpers to the 50s the suite already uses. The bound only takes effect when a wait is about to fail, so this costs nothing on the passing path: three runs before the change averaged 127s and three after averaged 124s, with 137 tests passing every time. The two waits on cluster-slot-migration-log-max-len and the one on a server restart keep their 10s, since those observe local effects rather than cluster-wide propagation. Signed-off-by: Himangshu Deka <hdeka@salesforce.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe migration test wait helpers now use 50-second timeouts for migration state, key-count, and slot-ownership conditions. Comments describe the asynchronous operations covered by these waits, and timeout messages report the new bound. ChangesCluster migration test waits
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized test-only change aligns three asynchronous migration waits with the suite's existing 50-second bound and corrects their timeout messages; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
1 participant
Fixes #4326 and #4311. Both are the same root cause in
tests/unit/cluster/cluster-migrateslots.tcl.What failed
wait_for_migrationwait_for_migration_fieldreading-establish-response, an intermediate stateBoth on
test-ubuntu-tls-io-threads, the slowest CI configuration. Neither shows a wrong result — both show an operation still in flight.Root cause
wait_for_condition 1000 50).flowchart LR S["MIGRATESLOTS issued"] --> P["migration in progress<br/>(reading-establish-response,<br/>ownership not yet moved)"] P -->|"10s bound expires here<br/>→ test FAILS"| F["reported as failure"] P -->|"work actually completes"| D["done, correct result"] style F fill:#ffe0e0 style D fill:#e0ffe0Fix
wait_for_migration— 10s → 50swait_for_migration_field— 10s → 50swait_for_countkeysinslot— 10s → 50swithin 10000 mstext in those three failure messages, which would otherwise report the wrong bound.Left at 10s deliberately:
cluster-slot-migration-log-max-lenwaits — observe a local config effectEvidence
Bound only applies when a wait is about to fail, so there is no cost on the passing path:
Not included
failover2.tcl, valgrind) — already uses the 50s bound, so there is no equivalent defect to fix. Needs valgrind to investigate, which I cannot run on this machine.sentinel/00-base.tcl) — different subsystem and a different failure shape (asserts a specific port), unrelated to these helpers.Both have
occurrences:1and I could not reproduce either, so I left them alone rather than guess.