Skip to content

Prefer topology views with full slot coverage during topology refresh… - #3854

Open
wmxl wants to merge 1 commit into
redis:mainfrom
wmxl:prefer-full-slot-coverage-2769
Open

Prefer topology views with full slot coverage during topology refresh…#3854
wmxl wants to merge 1 commit into
redis:mainfrom
wmxl:prefer-full-slot-coverage-2769

Conversation

@wmxl

@wmxl wmxl commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

Summary

Follow-up to the discussion in #2769 (reported by me). When a node joins a cluster (e.g. adding a replica), it briefly acts as a master without slots and has not yet synchronized the cluster topology. If a topology refresh queries such a node, it returns a view in which itself and some actual masters carry no slots. KnownMajority counts only how many known nodes a view contains, so this incorrect view ties with healthy views and can be adopted by the random tie-break — leaving the client throwing PartitionSelectorException: Cannot determine a partition for slot ... for a full refresh period.

determinePartitions now drops topology views without full slot coverage before consensus, as long as at least one view covers all 16384 slots.

Key Decisions & Assumptions

  • In PartitionSelectorException in Lettuce Client Triggered by Addition of New Slave Nodes in Redis Cluster #2769 I initially proposed discarding views that contain a master without slots, and @mp911de rightly objected that such a node can be legitimate (e.g. Pub/Sub usage). The check is therefore per view (total covered slots), not per node: a legitimate slotless master does not reduce the coverage of an otherwise complete view, so no view is discarded because of it.
  • If no view provides full slot coverage (clusters running with cluster-require-full-coverage no, bootstrap, lost slots), all views are retained and behavior is unchanged, so partial-coverage clusters keep working and the filter can never produce an empty candidate set.
  • PartitionsConsensus is intentionally untouched: it remains solely responsible for split-brain arbitration; view validation happens before consensus in determinePartitions. Network partitions do not unassign slots, so slot coverage is orthogonal to the split-brain signal that KNOWN_MAJORITY evaluates.
  • Adds RedisClusterNode#getSlotCount() (backed by BitSet.cardinality()) to compute coverage without materializing a slot List per node, following up on hasNoSlots() (Add hasNoSlots() Method to RedisClusterNode #3014).

Behavioral / Conceptual Changes

  • Both refresh paths are protected: KNOWN_MAJORITY (periodic/adaptive refresh) and HEALTHY_MAJORITY (initial topology load).
  • A view with partial slot coverage is now only adopted when every retrieved view has partial coverage. In the rare case of deliberately unassigning slots (CLUSTER DELSLOTS, forgetting a slot-holding node), clients may keep a stale full-coverage view for up to one additional refresh period until all views reflect the new state.
  • Subclasses overriding determinePartitions bypass the filter (unchanged extension point).

Testing

New unit tests cover determinePartitions directly: an incomplete view (as reported by a just-joined node) loses against a full-coverage view on both the known-majority and initial-load paths, all views are retained when none has full coverage, and a single partial view is still returned on initial load. Each assertion runs 10 iterations to rule out shuffle luck. getSlotCount() is covered for assigned, empty, and null slot sets.

We have also been running an equivalent filter in production since early 2024 (via a RedisClusterClient subclass overriding determinePartitions, as suggested in #2769) across a 30-shard cluster with frequent replica-scaling operations, with no recurrence of the exception. The original failure was reproducible by repeatedly re-adding a replica (del-node + add-node --cluster-slave) under a 1s refresh period, as described in #2769.

Notes

Fixes #2769.


Note

Medium Risk
Changes default cluster topology selection on every refresh path; low risk when full coverage exists, with a possible one-refresh delay after deliberate slot removal if some nodes still report full coverage.

Overview
Fixes intermittent PartitionSelectorException when topology refresh hits a newly joined node that still reports masters without slots: incomplete views can tie in KNOWN_MAJORITY and win a random tie-break.

determinePartitions now passes only views whose nodes sum to 16384 slots into HEALTHY_MAJORITY / KNOWN_MAJORITY when at least one such view exists; if every view is partial (e.g. cluster-require-full-coverage no), behavior is unchanged.

Adds RedisClusterNode#getSlotCount() (BitSet cardinality) for coverage checks without allocating slot lists, plus unit tests for determinePartitions and slot count.

Reviewed by Cursor Bugbot for commit c06220e. Bugbot is set up for automated code reviews on this repo. Configure here.

@a-TODO-rov

Copy link
Copy Markdown
Collaborator

Hey @wmxl
Thanks for contributing.
Will get back to you shortly. The team is a bit overwhelmed at the moment.

@wmxl
wmxl force-pushed the prefer-full-slot-coverage-2769 branch from ab61c3c to c06220e Compare August 19, 2026 09:18
@wmxl
wmxl force-pushed the prefer-full-slot-coverage-2769 branch from c06220e to 765cbde Compare August 19, 2026 09:46
@wmxl

wmxl commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Hi @a-TODO-rov , just a gentle follow-up on this one.

I've rebased the branch onto the latest main (post-7.7.0) — no conflicts — and updated the @since tag of the new getSlotCount() method to 7.8 accordingly. CI is green and this is ready for review whenever the team has bandwidth. Thanks!

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.

PartitionSelectorException in Lettuce Client Triggered by Addition of New Slave Nodes in Redis Cluster

2 participants