Exclude zero-token nodes from query routing - #252
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change tracks whether host token metadata is present and marks hosts with explicitly empty token sets as ignored. Round-robin policies exclude these hosts from query plans and restore routing when tokens are later assigned. Tests cover host distance transitions, policy routing, and connection-pool creation after token acquisition. Build and test targets now use serialized MSBuild execution settings. Sequence Diagram(s)sequenceDiagram
participant HostMetadata
participant Host
participant LoadBalancingPolicy
participant PrepareHandler
HostMetadata->>Host: SetInfo(tokens)
Host->>Host: update distance and UP/DOWN state
LoadBalancingPolicy->>Host: evaluate IsZeroTokenNode
Host-->>LoadBalancingPolicy: Ignored or routable
PrepareHandler->>LoadBalancingPolicy: request query plan
LoadBalancingPolicy-->>PrepareHandler: eligible hosts
PrepareHandler->>PrepareHandler: create connection pool for routable host
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
50d7c15 to
107482f
Compare
dkropachev
left a comment
dkropachev
left a comment
There was a problem hiding this comment.
Please check all the places where Host.SetInfo is called and make sure that node pools are updated.
At minimum we should have a test when node goes from 0 token to owning some tokens, because it is expected scenario.
And we need to make sure that Session is going to create node pool for that node in such case.
cc0139f to
e31691d
Compare
When a host gains tokens, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Makefile (2)
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
LoadBalancingPolicyTestsin Scylla filter.
FullyQualifiedName!~LoadBalancingPolicyTestsappears twice in theTEST_INTEGRATION_SCYLLA_FILTERexclusion list. The second occurrence is redundant and can be removed to avoid confusion during future maintenance.🤖 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 `@Makefile` at line 78, Remove the second duplicate FullyQualifiedName!~LoadBalancingPolicyTests entry from TEST_INTEGRATION_SCYLLA_FILTER, preserving the first occurrence and all other exclusions unchanged.
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider breaking the long filter expression across lines.
The
TEST_INTEGRATION_SCYLLA_FILTERis a single ~1.2 KB line, making it difficult to scan for specific exclusions or detect duplicates. Using Make's line-continuation backslash would improve readability without changing behavior.♻️ Suggested formatting
TEST_INTEGRATION_SCYLLA_FILTER ?= \ (FullyQualifiedName!~ClientWarningsTests & \ FullyQualifiedName!~CustomPayloadTests & \ FullyQualifiedName!~Connect_With_Ssl_Test & \ ... \ FullyQualifiedName!~RetryPolicyTests)🤖 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 `@Makefile` at line 78, Reformat the TEST_INTEGRATION_SCYLLA_FILTER assignment across multiple Makefile lines using backslash continuations, placing each exclusion on its own line while preserving the existing expression, exclusions, ordering, and behavior.
🤖 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.
Nitpick comments:
In `@Makefile`:
- Line 78: Remove the second duplicate
FullyQualifiedName!~LoadBalancingPolicyTests entry from
TEST_INTEGRATION_SCYLLA_FILTER, preserving the first occurrence and all other
exclusions unchanged.
- Line 78: Reformat the TEST_INTEGRATION_SCYLLA_FILTER assignment across
multiple Makefile lines using backslash continuations, placing each exclusion on
its own line while preserving the existing expression, exclusions, ordering, and
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 04f72e0e-e765-4434-b475-5a244461e534
📒 Files selected for processing (7)
Makefilesrc/Cassandra.Tests/HostTests.cssrc/Cassandra.Tests/PoliciesUnitTests.cssrc/Cassandra.Tests/Requests/PrepareHandlerTests.cssrc/Cassandra/Host.cssrc/Cassandra/Policies/DCAwareRoundRobinPolicy.cssrc/Cassandra/Policies/RoundRobinPolicy.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Cassandra/Policies/RoundRobinPolicy.cs
- src/Cassandra/Host.cs
1bf577c to
69933b5
Compare
69933b5 to
42ac295
Compare
42ac295 to
8402565
Compare
e17e34b to
a2100c7
Compare
a2100c7 to
1ad5362
Compare
There was a problem hiding this comment.
Pull request overview
Excludes explicit zero-token nodes from built-in query routing while allowing hosts to become routable after gaining tokens.
Changes:
- Marks zero-token hosts ignored and filters query plans.
- Tracks token-state transitions and coordinates connection pools.
- Adds routing, host-transition, and pool-concurrency tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
RoundRobinPolicy.cs |
Filters zero-token hosts. |
DCAwareRoundRobinPolicy.cs |
Filters local and remote plans. |
Host.cs |
Tracks zero-token state. |
HostConnectionPool.cs |
Handles empty-pool draining. |
TestHelper.cs |
Models absent versus empty tokens. |
PrepareHandlerTests.cs |
Tests routing restoration. |
PoliciesUnitTests.cs |
Tests policy filtering and balancing. |
HostTests.cs |
Tests token-state transitions. |
HostConnectionPoolTests.cs |
Tests pool transition races. |
Makefile |
Serializes test builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Nodes must be targetted by driver queries in order to become coordinators, unless we're taking CQL forwarding into consideration. Maybe @piodul or @dawmd can explain this. |
I can remove the |
Introduce the notion of a zero-token node on Host. Such nodes appear in topology metadata with an empty token set, while hosts whose token metadata is absent or unknown must keep their previous behaviour. Expose Host.IsZeroTokenNode, tracked via a dedicated flag that is recomputed whenever a row carries the tokens column and set true only when that token set is empty. Because Scylla returns the empty non-frozen tokens set as NULL in system.local/system.peers, a present-but-NULL tokens column is treated as an empty token set (a real zero-token node); a non-empty set clears the flag, and an absent column leaves the token state unknown, so a host is not misclassified in either direction. When fresh metadata marks an existing host as zero-token, force its distance to Ignored so session pools receive DistanceChanged and drain instead of keeping query connections open. When a host transitions out of the zero-token state (it gains tokens after previously having none), do not call BringUpIfDown(): a token update does not prove liveness, as system.peers can still contain a down peer. The connection pool's successful open will call BringUpIfDown() once a real connection confirms the node is reachable. Add HostTests covering the distance change on becoming zero-token, the no-BringUpIfDown-on-token-gain behaviour, and the NULL/absent tokens column cases.
Update RoundRobinPolicy and DCAwareRoundRobinPolicy to report zero-token hosts as Ignored and omit them from generated query plans, while keeping the cached host lists able to observe a zero-token host that later gains tokens. Enforce the exclusion centrally in Cluster.RetrieveAndSetDistance: it now returns Ignored for a zero-token host before consulting the load balancing policy. This is the single choke point every routing path funnels through (request handler, prepare handler, control connection, session warmup), so the guarantee holds even for custom load balancing policies that do not check Host.IsZeroTokenNode, and keeps the distance consistent with the Ignored value Host.SetInfo forces when the node is first observed as zero-token. The built-in policies still check IsZeroTokenNode in NewQueryPlan because that also drives query-plan filtering, not just the reported distance. For DCAwareRoundRobinPolicy, filter the local hosts down to the routable ones before applying the round-robin modulo. Rotating over the full list and skipping zero-token hosts afterwards biased the first attempt towards the host following a zero-token node; filtering first preserves a fair round-robin across the routable hosts. Add unit coverage for both built-in policies: zero-token exclusion, zero-token-to-token routing, balanced first-host distribution, and the prepare-handler flow that creates a connection pool once a zero-token host gains tokens.
8351a0d to
f1811b9
Compare
|
@wprzytula I removed mentions of coordinator-only nodes |
|
@dkropachev could you rereview this PR please? |
Zero-token nodes are only going to be supported in raft topology mode (which became the only supported mode in 2026.2). This has nothing to do with strong consistency or CQL forwarding. As for the motivation, I am not sure either why we want to outright exclude the nodes. In the cover letter of the PR which brought the implementation to core, Patryk explicitly wrote:
The zero-token nodes do not keep any data, so they do not serve any requests as replicas but it looks like they should work fine as coordinators. I can see why we should avoid these nodes if we are doing token-aware routing, but not sure why they should be outright excluded - but I don't know/remember all the details about our default load balancing policies so maybe there is some sense for it if we disallow failover from token-aware routing. (@patjed41 please correct me if I'm wrong about this, or if there is anything worth adding here) |
| /// </summary> | ||
| private void FinishEmptyDrain(Action afterDrainHandler, int retriesLeft = HostConnectionPool.EmptyDrainMaxRetries) | ||
| { | ||
| if (Volatile.Read(ref _connectionOpenTcs) == null || retriesLeft <= 0) |
There was a problem hiding this comment.
Broken case: when retriesLeft <= 0 while _connectionOpenTcs is still set, CompleteDrain() resets the pool to Init; a late successful open then passes the IsClosing checks and can add a connection while the host is still Ignored.
Yes
Yes, we even have a test for that in Scylla --
I can recall the whole story to give some context. The main motivation for introducing zero-token nodes was to use them in arbiter DC (an additional single node DC in a 2-DC cluster) to prevent majority loss in group0 when one DC dies. At least that was the plan. I don't know how widely this feature is used currently. For arbiter DC, we didn't want drivers to contact zero-token nodes. We concluded (perhaps incorrectly after looking at this PR) that drivers usually do exactly what we wanted, as they ignore all nodes with no tokens in We decided that we can always change our drivers however we want if we ever need them to contact zero-token nodes. One possible use case is coordinator-only/proxy nodes, which was the motivation to introduce zero-token nodes in Cassandra. A video about this idea: https://www.youtube.com/watch?v=K0sQvaxiDH0. From what I remember, DataStax drivers don't support zero-token nodes well, and workarounds like the I don't know how drivers should handle zero-token nodes. It depends on if/how zero-token nodes are/will be used. |
When a host transitions to Ignored (e.g. a zero-token node) and its pool is empty, FinishEmptyDrain polls until any in-flight connection open completes before resetting the pool state to Init, preventing a race where the completing open would add a connection to a now-ignored host. Introduce EmptyDrainMaxRetries (30 attempts, 1 s apart) so the polling loop cannot spin forever. If the open does not complete in time, log a warning and force the drain to completion anyway. Because that forced completion resets the pool state to Init while an open may still be in flight, the post-open acceptance checks in CreateOpenConnection can no longer rely on pool state alone: a late successful open would pass the IsClosing checks and add a connection to a still-Ignored host. Guard both acceptance points with ShouldDiscardNewConnection (IsClosing || _distance == Ignored) so an ignored host never keeps an established connection regardless of how the drain finished. _distance defaults to Local, so pool warmup is unaffected. After a drain the pool might have missed a reconnection trigger fired while it was Closing (e.g. an Ignored->Local distance change): the OnDistanceChanged ScheduleReconnection call returns early because IsClosing is still true. A shared CompleteDrain helper, run from both the empty (FinishEmptyDrain) and non-empty (DrainConnectionsTimer) drain paths, checks _distance after resetting state and calls ScheduleReconnection when the host is now routable. It also resets _canCreateForeground first, mirroring OnDistanceChanged and OnHostUp, so EnsureCreate calls do not fail in the window before the first connection is established. Add HostConnectionPoolTests covering the three new behaviours: - Empty pool drain does not block a later Ignored->Local reconnection. - In-flight open is discarded when the pool becomes Ignored. - Distance changed to Local during FinishEmptyDrain polling triggers reconnection after the drain completes.
f1811b9 to
c2943e6
Compare
| var tokenList = tokens as ICollection<string> ?? tokens.ToArray(); | ||
| Tokens = tokenList; | ||
| _isZeroTokenNode = tokenList.Count == 0; |
@dkropachev Considering all above, should we really pursue this PR and similar ones? There's no clarity in how driver should approach zero-token nodes on the Core side IIUC. |
@wprzytula In different drivers we treat peers that don't have tokens as invalid, so they are excluded without much effort, C# driver doesn't have such mechanism this is why this PR is more complex then its equivalents in different drivers. |
This is not true for Rust Driver. Zero-token nodes are still considered valid for routing, just will never be preferred for token-aware routing. |
I should have been more specific, it is like that in python-driver and gocql. Similarly in java, by default they are considered invalid, but there is a special config option to make them valid only if user sets it. |
Understood. The way I see this matter is that there's no clear conclusion on how drivers should handle zero-token nodes (avoid them from token-unaware routing or not), so I'm against investing our time in doing changes back-and-forth until the Core comes up with a clear guidance. |
Makes sense, @dkropachev WDYT? should we proceed with this PR or wait for the core decision? |
The requireements of this issue: we need to make sure that driver works properly when zero token nodes are present in the cluster. For now we have only one case in mind where these nodes are not participating in query execution, whcih means that no need to create/handle connections to them, which means that we technically can not add them to metadata. You already can see how letting these nodes into metadata creates all sort of problems in drivers with mutable Node state. When/if we decide to let them in, then we will have to address these problems, and most-likely then not we need to make Node state immutable first. |
|
@dkropachev @wprzytula FYI |
Does it(master)have tests for zero token nodes? If not let's have pr to bring tests in |
I doesn't, I added tests in the draft I mentioned |
Then let's close it as superseeded by #261, please update pr title and description |
|
Closing in favor of #261 |
Zero-token nodes can appear in topology metadata with an explicit empty token list. Treat those hosts as non-routing nodes while preserving hosts whose token metadata is absent or unknown.
Update RoundRobinPolicy and DCAwareRoundRobinPolicy to mark zero-token hosts as Ignored and omit them from generated query plans, including cached DC-aware host lists.
Add unit coverage for both built-in round-robin policies to verify zero-token hosts are skipped and token-owning hosts remain routable.
Fixes: https://scylladb.atlassian.net/browse/DRIVER-191