Ensure zero token nodes are handled properly - #261
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds regression coverage and a manual Scylla harness for zero-token coordinator nodes.
Changes:
- Adds unit tests for topology, routing, and logging behavior.
- Adds a CCM-based manual verification workflow.
- Adds a Make target for running the workflow.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Adds the manual-test target. |
ci/run-zero-token-manual-test.sh |
Creates and exercises a zero-token Scylla node. |
tests/ZeroTokenManualTest/ZeroTokenManualTest.csproj |
Defines the manual harness project. |
tests/ZeroTokenManualTest/Program.cs |
Implements routing and topology checks. |
src/Cassandra.Tests/ZeroTokenNodeTests.cs |
Adds zero-token routing tests. |
src/Cassandra.Tests/Connections/Control/TopologyRefresherTests.cs |
Tests zero-token topology discovery. |
Comments suppressed due to low confidence (2)
ci/run-zero-token-manual-test.sh:90
- With
set -e, a nonzero watch result exits the script atwait, soWATCH_RCis never captured, the watch log is never printed, and the contact-point phase is skipped. Runwaitas anifcondition so the script can preserve the status and finish the remaining diagnostics.
wait "${WATCH_PID}"
WATCH_RC=$?
tests/ZeroTokenManualTest/Program.cs:277
- The watch phase passes whenever queries do not throw, even if no zero-token DOWN/UP transition was observed and warnings/errors were captured. That lets the topology scenario pass without exercising its stated behavior; include both expected transitions and the listener result in the success condition.
var queryErrors = events.Count(e => e.Contains("QUERY-ERROR") || e.Contains("EXCEPTION"));
var ok = queryErrors == 0;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dkropachev
left a comment
dkropachev
left a comment
There was a problem hiding this comment.
Also make sure all the cases covered - including node being loaded with zero-token neds and then upgraded to the regular node.
0966476 to
2f10a56
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/Cassandra.Tests/ZeroTokenNodeTests.cs:87
DCAwareRoundRobinPolicymust also exclude a discovered zero-token peer from query plans. Keeping it local and routable conflicts with the zero-token driver requirements and with the Rust-driver behavior this fixture claims to follow. Update the policy implementation and make these assertions expectIgnored/absence from the plan while retaining the host in metadata.
Assert.AreEqual(HostDistance.Local, policy.Distance(zeroToken));
var plan = policy.NewQueryPlan(null, new SimpleStatement()).Select(h => h.Host).ToList();
CollectionAssert.Contains(plan, zeroToken);
2f10a56 to
5f45d3f
Compare
5f45d3f to
b77cb04
Compare
b77cb04 to
c04050d
Compare
![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 (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer category-based exclusion over appending test names to this filter.
Should_KeepServingQueries_When_ZeroTokenNodeGoesDownAndUpandZeroTokenNodeReplacementTestsare both already taggedRealClusterLong. Excluding them by name means every future long-running test has to be added to this line manually and silently runs if forgotten.TestCategory!=RealClusterLongwould cover them declaratively.♻️ Suggested direction
-... & FullyQualifiedName!~RetryPolicyTests & FullyQualifiedName!~ZeroTokenNodeReplacementTests & FullyQualifiedName!~Should_KeepServingQueries_When_ZeroTokenNodeGoesDownAndUp) +... & FullyQualifiedName!~RetryPolicyTests) & TestCategory!=realclusterlongVerify the exact category string emitted by
TestCategory.RealClusterLongbefore applying.🤖 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 60, Update TEST_INTEGRATION_SCYLLA_FILTER to exclude the RealClusterLong category using the exact emitted TestCategory.RealClusterLong string, rather than listing ZeroTokenNodeReplacementTests and Should_KeepServingQueries_When_ZeroTokenNodeGoesDownAndUp by name. Remove those individual name filters while preserving the remaining exclusions.src/Cassandra.Tests/ZeroTokenNodeUnitTests.cs (1)
183-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match the exercised helper path.
TestHelper.CreateHostalready callsHost.SetInfo, so this does exercise the production token normalization path. RenameSetInfo_Should_YieldEmptyTokens_When_TokensAreNullOrEmptyto avoid implying the test directly invokesHost.SetInfo.🤖 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 `@src/Cassandra.Tests/ZeroTokenNodeUnitTests.cs` around lines 183 - 194, Rename the test method SetInfo_Should_YieldEmptyTokens_When_TokensAreNullOrEmpty to reflect that it exercises the path through TestHelper.CreateHost rather than directly invoking Host.SetInfo, while preserving the test logic and assertions.
🤖 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 60: Update TEST_INTEGRATION_SCYLLA_FILTER to exclude the RealClusterLong
category using the exact emitted TestCategory.RealClusterLong string, rather
than listing ZeroTokenNodeReplacementTests and
Should_KeepServingQueries_When_ZeroTokenNodeGoesDownAndUp by name. Remove those
individual name filters while preserving the remaining exclusions.
In `@src/Cassandra.Tests/ZeroTokenNodeUnitTests.cs`:
- Around line 183-194: Rename the test method
SetInfo_Should_YieldEmptyTokens_When_TokensAreNullOrEmpty to reflect that it
exercises the path through TestHelper.CreateHost rather than directly invoking
Host.SetInfo, while preserving the test logic and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fdc71c6d-a6fa-4aa5-bd20-f57e9e9621e2
📒 Files selected for processing (5)
Makefilesrc/Cassandra.IntegrationTests/Core/ZeroTokenNodeTests.cssrc/Cassandra.Tests/Connections/Control/TopologyRefresherTests.cssrc/Cassandra.Tests/ZeroTokenNodeUnitTests.cssrc/Cassandra/TokenMap.cs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/Cassandra.IntegrationTests/Core/ZeroTokenNodeTests.cs:148
- The
NeverUsename promises behavior this test andTokenAwarePolicydo not provide: the test only verifies that healthy token owners are prioritized, while the policy can later fall back to the zero-token host through its child plan. Rename the test to describe the behavior it actually checks.
public void Should_NeverUseZeroTokenNode_As_Coordinator_When_TokenAware()
src/Cassandra.IntegrationTests/Core/ZeroTokenNodeTests.cs:95
- This overstates the token-aware routing contract.
TokenAwarePolicyyields replicas first, then appends every remaining host from its child policy (src/Cassandra/Policies/TokenAwarePolicy.cs:141-151), so a zero-token host can still coordinate after replica failures. Document that it is not returned or prioritized as a replica rather than saying token-aware routing can never target it.
This issue also appears on line 148 of the same file.
/// <item>a zero-token node never owns data, so it must never be returned as a replica by the
/// token map, and consequently token aware routing must never target it.</item>
c04050d to
4790a4b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Cassandra/TokenMap.cs:130
hasTokensonly checks whether token strings were advertised, not whether any token parsed successfully. Since parse failures are caught below, a host whose tokens are all malformed still inflatesHostLengthandRacks; on a distinct rack this reproduces the unsatisfied-RF bug this change fixes. Register datacenters only after a host has produced at least one parsed token, or derive the datacenter metadata from the distinct hosts inprimaryReplicas.
var hasTokens = host.Tokens.Any();
if (hasTokens && host.Datacenter != null)
|
CI failures unrelated to the changes made, most likely a flaky test |
Support zero-token nodes
Adds coverage for zero-token nodes (ScyllaDB nodes started with
join_ring: false, which reporttokens = nullinsystem.peers) and fixes a replica-placement bug found while writing it.Expected semantics, aligned with the Rust driver: a zero-token node is an ordinary routable host —
discovered, kept in load balancing plans, usable as a coordinator and as a contact point — but it
never owns data, so it must never be returned as a replica. Processing one must not warn or error.
Bug fix
TokenMap.Buildregistered every host in its per-datacenter host count and rack set, includinghosts that own no tokens.
NetworkTopologyStrategyuses that rack set to decide whether racks arestill missing, so a zero-token node on its own rack inflated
Racks.Count, keptracksMissingtrue forever and caused real token owners to be skipped — leaving the replication factor
unsatisfied. The inflated host count also made
dcRftoo large, forcing a full ring scan per token.The fix registers a datacenter only for hosts that actually own tokens. A datacenter made up
exclusively of zero-token nodes is therefore absent from the map and its replication factor is
treated as trivially satisfied.
HostLength/Racksare consumed only byNetworkTopologyStrategy, so the blast radius is limited to it.Fixes: https://scylladb.atlassian.net/browse/DRIVER-191