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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Cassandra/Connections/HostConnectionPool.cs:571
- When retries expire, this branch resets the pool from
Closingeven though_connectionOpenTcsstill represents an active open. If the host remainsIgnored, that open can later pass theIsClosingchecks inCreateOpenConnectionand add a connection to the ignored host—the race this method is intended to prevent. Keep the pool closing until the open is cancelled/completes, or also reject the completed connection based on the current distance.
if (Volatile.Read(ref _connectionOpenTcs) == null || retriesLeft <= 0)
{
if (retriesLeft <= 0)
{
HostConnectionPool.Logger.Warning(
02194ca to
d53f8a1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Cassandra/Connections/HostConnectionPool.cs:570
- When the retry budget expires,
_connectionOpenTcsis explicitly still non-null, yet this branch resets the pool fromClosingtoInit. The delayed open then passes bothIsClosingchecks inCreateOpenConnectionand can add a connection that is never drained from an Ignored host. Keep the pool closing until that open settles, or add cancellation/generation tracking so a timed-out open can never publish its connection before resetting the state.
if (Volatile.Read(ref _connectionOpenTcs) == null || retriesLeft <= 0)
{
if (retriesLeft <= 0)
{
d53f8a1 to
fb5ccfe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Cassandra/Connections/HostConnectionPool.cs:569
- When the retry count reaches zero while
_connectionOpenTcsis still non-null, this branch invokes the ignored-host drain handler and changes the pool fromClosingback toInit. A connection open taking longer than 30 seconds can then pass theIsClosingchecks when it eventually completes and be added to the ignored host's pool, defeating the protection this method adds. Drain completion must wait for or cancel/observe that open rather than resetting the state while it remains in flight.
if (Volatile.Read(ref _connectionOpenTcs) == null || retriesLeft <= 0)
{
if (retriesLeft <= 0)
fb5ccfe to
a5de828
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/Cassandra/Connections/HostConnectionPool.cs:569
- After 30 retries this resets the pool to
Initeven though_connectionOpenTcsstill represents an in-flight open. If the host is still Ignored and that open later completes, bothIsClosingchecks inCreateOpenConnectionare now false, so the connection is added to the ignored host—the exact race this method is meant to prevent. Keep a generation/ignored-state guard through completion (or otherwise ensure a late connection is discarded) before forcing the state reset.
if (Volatile.Read(ref _connectionOpenTcs) == null || retriesLeft <= 0)
{
if (retriesLeft <= 0)
a5de828 to
478b60c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Cassandra/Connections/HostConnectionPool.cs:571
- When the retry budget expires,
_connectionOpenTcscan still represent an active socket open, yet this branch resets the pool toInit. When that open eventually returns,CreateOpenConnectionseesIsClosing == falseand adds the connection to the zero-token/ignored host. A slow or stalled open lasting over 30 seconds therefore defeats the drain's safety guarantee. Keep the pool closing until the in-flight task actually clears (or cancel/version that attempt) rather than forcing completion.
if (Volatile.Read(ref _connectionOpenTcs) == null || retriesLeft <= 0)
{
if (retriesLeft <= 0)
{
HostConnectionPool.Logger.Warning(
478b60c to
22c67ab
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Cassandra/Connections/HostConnectionPool.cs:574
- The 30-second fallback reopens the race this method is meant to prevent. If a connection attempt remains in
DoCreateAndOpenpast the retry limit, this resets the pool toInit; when that attempt later completes, itsIsClosingchecks pass and it can add a connection to an ignored host. Do not complete the drain while_connectionOpenTcsis still non-null (or await that task directly) rather than forcing completion on elapsed time.
if (Volatile.Read(ref _connectionOpenTcs) == null || retriesLeft <= 0)
{
if (retriesLeft <= 0)
{
HostConnectionPool.Logger.Warning(
"Pool #{0} to {1}: in-flight connection open did not complete within {2} s; " +
"forcing drain completion.",
GetHashCode(), _host.Address, HostConnectionPool.EmptyDrainMaxRetries);
Introduce the notion of a zero-token, coordinator-only 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 set whenever a row carries the tokens column. 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); only 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.
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. 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.
22c67ab to
93f89d0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Cassandra/Connections/HostConnectionPool.cs:571
- A fixed 30-second cutoff reopens the pool while the connection open is still in flight.
ConnectTimeoutMillisis configurable above 30 seconds (and opening can include multiple timed phases), so the open can later observeInitand add a connection to a host whose distance is stillIgnored; that connection is then never drained because there is no new distance transition. Keep the pool closing until the open completes, or make the connection-add path reject connections whenever_distance == HostDistance.Ignoredinstead of forcing completion here.
if (Volatile.Read(ref _connectionOpenTcs) == null || retriesLeft <= 0)
{
if (retriesLeft <= 0)
{
HostConnectionPool.Logger.Warning(
wprzytula
left a comment
wprzytula
left a comment
There was a problem hiding this comment.
Basic question to the very approach taken by this PR. Cover letter says:
Zero-token coordinator-only 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.
The bolded parts seem contradictory. If those nodes are intended to be coordinator-only (so their purpose is to serve as request coordinators when drivers contact them to execute statements), then why exclude those nodes from routing? This defeats their purpose in the cluster IMO.
| @@ -53,15 +53,114 @@ public void Should_UseHostIdEmpty_When_HostIdIsNull() | |||
| Assert.AreEqual(Guid.Empty, host.HostId); | |||
There was a problem hiding this comment.
Expose Host.IsZeroTokenNode, tracked via a dedicated flag that is set whenever a row carries the tokens column.
🤔 I don't understand. Why would the zero-token flag be set always if the tokens column is set, even if nonempty?
| @@ -53,15 +53,114 @@ public void Should_UseHostIdEmpty_When_HostIdIsNull() | |||
| Assert.AreEqual(Guid.Empty, host.HostId); | |||
There was a problem hiding this comment.
only an absent column leaves the token state unknown, so a host is not misclassified in either direction.
❓ Do you know if and when such situation (lack of tokens column) may happen?
Zero-token coordinator-only 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