Skip to content

Conversation

hanxizh9910
Copy link
Contributor

@hanxizh9910 hanxizh9910 commented Oct 20, 2025

Address: #2693

I add an additional check to ensure that all nodes recognize the full cluster (4 nodes) after a new node is added. This helps prevent the test from failing due to cluster gossip not being fully propagated.

I ran the test 100 times for both the unmodified and modified versions, and all runs passed:

Original (unmodified): CI run

Modified (with fix): CI run

For reference, this test previously failed once in https://github.com/valkey-io/valkey/actions/runs/18266295942/job/52001092528

@hanxizh9910 hanxizh9910 changed the title Fix/cluster slot migration flaky test 2693 [Deflake ]Fix/cluster slot migration flaky test 2693 Oct 20, 2025
@hanxizh9910 hanxizh9910 changed the title [Deflake ]Fix/cluster slot migration flaky test 2693 [Deflake] Fix/cluster slot migration flaky test 2693 Oct 20, 2025
@hanxizh9910 hanxizh9910 changed the title [Deflake] Fix/cluster slot migration flaky test 2693 [DEFLAKE] Fix/cluster slot migration flaky test Oct 20, 2025
Copy link

codecov bot commented Oct 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.46%. Comparing base (b4c93cc) to head (8df0ded).
⚠️ Report is 3 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2756      +/-   ##
============================================
- Coverage     72.59%   72.46%   -0.13%     
============================================
  Files           128      128              
  Lines         71301    71303       +2     
============================================
- Hits          51759    51671      -88     
- Misses        19542    19632      +90     

see 21 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

} else {
fail "Cluster gossip hasn't propagated all node IDs"
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have wait_for_cluster_size 4 in line 276, it is indedd checking the cluster_known_nodes

# Check if cluster size is consistent.
proc cluster_size_consistent {cluster_size} {
    for {set j 0} {$j < $cluster_size} {incr j} {
        if {[CI $j cluster_known_nodes] ne $cluster_size} {
            return 0
        }
    }
    return 1
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right! i will remove this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants