fix: detect ScyllaDB via SUPPORTED protocol extensions, not shard count - #910
fix: detect ScyllaDB via SUPPORTED protocol extensions, not shard count#910mykaul wants to merge 3 commits into
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
participant Cluster
participant ControlConnection
participant ProtocolFeatures
participant SchemaParserV3
Cluster->>ControlConnection: connect
ControlConnection->>ProtocolFeatures: read is_scylla
ProtocolFeatures-->>ControlConnection: Scylla capability flag
ControlConnection->>ControlConnection: configure peers-v2 and metadata timeout
SchemaParserV3->>ProtocolFeatures: read is_scylla
ProtocolFeatures-->>SchemaParserV3: Scylla capability flag
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Comment |
a64d05e to
bea45ac
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes ScyllaDB detection by using Scylla-specific SUPPORTED protocol extension keys (and sharding info when present) instead of relying on shard-related fields alone, preventing misclassification as Cassandra when shard-awareness is disabled and avoiding peers_v2-related connection failures.
Changes:
- Add
ProtocolFeatures.is_scyllaand implement Scylla detection via known Scylla-specificSUPPORTEDextension keys. - Harden sharding parsing/initialization against missing shard fields to prevent
int(None)crashes. - Gate peers_v2 usage,
USING TIMEOUT, and trigger-metadata behavior onis_scyllainstead of shard-awareness heuristics; add unit tests for detection/regressions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
cassandra/protocol_features.py |
Introduces is_scylla detection and adjusts sharding parsing to avoid crashes. |
cassandra/shard_info.py |
Guards integer conversion for missing shard-related fields. |
cassandra/c_shard_info.pyx |
Mirrors shard-info conversion guards in the Cython implementation. |
cassandra/cluster.py |
Switches peers_v2 disabling and metadata timeout gating to is_scylla. |
cassandra/metadata.py |
Updates Scylla-vs-not logic to use features.is_scylla. |
tests/unit/test_protocol_features.py |
Adds unit tests for Scylla detection and crash regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_protocol_features.py (1)
79-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the normalized
pf.shard_idin these regressions.These cases exercise the missing-
SCYLLA_SHARDpath, but they never check the new top-level normalization to0. Addingassert pf.shard_id == 0would pin the actual fix and catch a regression back toNone.Suggested assertions
def test_scylla_without_sharding_no_crash(self): ... assert pf.is_scylla is True + assert pf.shard_id == 0 assert pf.sharding_info is not None assert pf.sharding_info.shards_count == 0 assert pf.sharding_info.sharding_ignore_msb == 0 def test_scylla_sharding_algorithm_only_no_crash(self): ... assert pf.is_scylla is True + assert pf.shard_id == 0 assert pf.sharding_info is not None assert pf.sharding_info.shards_count == 0🤖 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 `@tests/unit/test_protocol_features.py` around lines 79 - 108, The regression tests in test_scylla_without_sharding_no_crash and test_scylla_sharding_algorithm_only_no_crash only verify sharding_info, but they do not assert the new normalized top-level shard_id value. Update both ProtocolFeatures.parse_from_supported regression cases to also check that pf.shard_id is 0, so the tests pin the missing-SCYLLA_SHARD normalization behavior and catch any return to None.
🤖 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 `@tests/unit/test_protocol_features.py`:
- Around line 79-108: The regression tests in
test_scylla_without_sharding_no_crash and
test_scylla_sharding_algorithm_only_no_crash only verify sharding_info, but they
do not assert the new normalized top-level shard_id value. Update both
ProtocolFeatures.parse_from_supported regression cases to also check that
pf.shard_id is 0, so the tests pin the missing-SCYLLA_SHARD normalization
behavior and catch any return to None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e195da2-f03f-4a94-a124-f7657f65a989
📒 Files selected for processing (6)
cassandra/c_shard_info.pyxcassandra/cluster.pycassandra/metadata.pycassandra/protocol_features.pycassandra/shard_info.pytests/unit/test_protocol_features.py
bea45ac to
b223706
Compare
The driver previously identified ScyllaDB solely by the presence of shard-related fields (SCYLLA_NR_SHARDS, SCYLLA_SHARD, etc.) in the SUPPORTED response. When shard-awareness is disabled on the server side (allow_shard_aware_drivers: false), those fields are absent, causing the driver to misidentify a ScyllaDB cluster as Cassandra. Changes: - Add ProtocolFeatures.detect_scylla() and is_scylla flag, set from known Scylla-specific extension keys (LWT, rate-limit, tablets routing) or sharding_info presence - Replace sharding_info check with is_scylla for peers_v2 and metadata_request_timeout in cluster.py and metadata.py - Guard int() calls against None in _ShardingInfo construction (c_shard_info.pyx, shard_info.py) - Add tests for is_scylla detection and regression tests for missing shard fields - Assert pf.shard_id normalization in test regressions (response to CodeRabbit review)
…plete ScyllaDB gates all SCYLLA_SHARD*/SCYLLA_NR_SHARDS/SCYLLA_PARTITIONER fields behind the same allow_shard_aware_drivers server-side config, so partial sharding info cannot occur in practice. Constructing a ShardingInfo with a zeroed-out shards_count would still violate the invariant, relied on elsewhere (e.g. pool.py's shard-aware pooling), that a non-None sharding_info implies a real, positive shard count. Revert the int(x) if x else 0 defensive guards in _ShardingInfo / ShardingInfo back to plain int(x) (cassandra/shard_info.py, cassandra/c_shard_info.pyx now match master exactly again), and instead tighten ProtocolFeatures.parse_sharding_info to detect an incomplete field set up front and return (0, None) rather than ever constructing a partial ShardingInfo. Also simplify SchemaParserV3._is_not_scylla(): ProtocolFeatures.is_scylla is always present once features is set, so the only thing worth guarding against is features itself being None/absent. Update the two crash-regression tests to assert the corrected behavior (is_scylla True, sharding_info None) instead of asserting a fabricated zero-shard ShardingInfo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
b223706 to
e9f95b6
Compare
|
@Lorak-mmk Thanks for the review and for digging into the server source — you were right, and I've pushed a fix that addresses both of your comments plus the same concern independently flagged by the copilot reviewer. On Instead, I tightened I also rewrote the two crash-regression tests ( On the def _is_not_scylla(self):
features = getattr(self.connection, 'features', None)
return not (features is not None and features.is_scylla)Rebased onto current master (picked up the keyword-only |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/unit/test_protocol_features.py`:
- Around line 59-72: Update test_is_scylla_detected_via_sharding to remove
SCYLLA_LWT_ADD_METADATA_MARK from the supported-feature fixture, ensuring
is_scylla is derived solely from the populated sharding_info while preserving
the existing shard and sharding assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6195dda0-7822-45e0-91f2-6f11a1309125
📒 Files selected for processing (4)
cassandra/cluster.pycassandra/metadata.pycassandra/protocol_features.pytests/unit/test_protocol_features.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cassandra/metadata.py
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 (3)
cassandra/protocol_features.py:101
- This still allows incomplete sharding blocks through. In particular, if
SCYLLA_SHARD,SCYLLA_NR_SHARDS, the expected partitioner, and algorithm are present butSCYLLA_SHARDING_IGNORE_MSBis absent,_ShardingInfo(..., None, ...)still raisesTypeErroratint(sharding_ignore_msb). If the ignore value is present while the partitioner or algorithm is missing, the code instead fabricates routing info for an unsupported configuration. Validate every field required by the shard calculation before constructing_ShardingInfo(and add the missing-ignore regression case).
if shard_id is None or shards_count is None:
cassandra/cluster.py:3901
- The new tests stop at
ProtocolFeatures.is_scylla; none exercises this control-connection wiring, although avoidingpeers_v2is the regression this PR is intended to fix. Add a control-connection test with extension-only features that verifies the first peer query usessystem.peersand that metadata timeout is enabled; this protects both changed consumers from drifting back to shard-count detection.
if connection.features.is_scylla:
self._uses_peers_v2 = False
# Only ScyllaDB supports "USING TIMEOUT". Use is_scylla for the same reason.
self._metadata_request_timeout = None if not connection.features.is_scylla or not self._cluster.metadata_request_timeout \
cassandra/metadata.py:2588
- This changes Cassandra/DSE schema behavior by adding trigger queries, but there is no regression test distinguishing a Cassandra connection (
is_scylla=False) from an extension-only ScyllaDB connection (is_scylla=True). Add schema-parser tests asserting that the trigger query is included only for the former so this compatibility-sensitive branch is covered.
features = getattr(self.connection, 'features', None)
return not (features is not None and features.is_scylla)
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 (3)
cassandra/protocol_features.py:101
- The incomplete-field guard still allows
SCYLLA_SHARDandSCYLLA_NR_SHARDSwith a missingSCYLLA_SHARDING_IGNORE_MSBthrough to_ShardingInfo. Both the Python and Cython constructors callint(sharding_ignore_msb), so that SUPPORTED response still raisesTypeErrorduring connection setup. Treat the missing ignore-MSB value as incomplete as well.
if shard_id is None or shards_count is None:
cassandra/cluster.py:3901
- The new protocol-feature tests only verify that
is_scyllais parsed; they do not exercise the connection behavior this regression is intended to fix. Add a control-connection test with extension-only Scylla features that asserts the initial query usessystem.peersrather thansystem.peers_v2and thatmetadata_request_timeoutis applied. Otherwise a wiring regression at either of these lines can still pass the added suite while reproducing the connection failure.
if connection.features.is_scylla:
self._uses_peers_v2 = False
# Only ScyllaDB supports "USING TIMEOUT". Use is_scylla for the same reason.
self._metadata_request_timeout = None if not connection.features.is_scylla or not self._cluster.metadata_request_timeout \
cassandra/metadata.py:2588
- Add coverage for both values of
ProtocolFeatures.is_scyllaat the schema-parser level, asserting that Cassandra includes and processes the triggers query while Scylla omits it. The added parser-only tests do not exercise this changed response list/unpacking behavior, so the stated Cassandra trigger-metadata fix is currently unverified.
features = getattr(self.connection, 'features', None)
return not (features is not None and features.is_scylla)
Summary
The driver previously identified ScyllaDB solely by the presence of shard-related fields (
SCYLLA_NR_SHARDS,SCYLLA_SHARD, etc.) in theSUPPORTEDresponse.When shard-awareness is disabled on the server side (
allow_shard_aware_drivers: false), those fields are absent, causing the driver to misidentify a ScyllaDB cluster as Cassandra.The immediate consequence is that the driver leaves
peers_v2enabled (which ScyllaDB does not support) and fails to connect — the same regression described in scylladb/gocql#902.This fix mirrors scylladb/gocql#903.
Changes
cassandra/protocol_features.pyProtocolFeatures.is_scyllabool (defaultFalse), set by the newdetect_scylla()static method.detect_scylla()returnsTruewhen any known Scylla-specific extension key (SCYLLA_LWT_ADD_METADATA_MARK,SCYLLA_RATE_LIMIT_ERROR,TABLETS_ROUTING_V1) is present in theSUPPORTEDresponse, or whensharding_infois populated.These keys are advertised by ScyllaDB regardless of whether shard-awareness is enabled, so
is_scyllastaysTrueeven with sharding disabled.parse_sharding_info:int(shard_id)raisedTypeErrorwhenSCYLLA_PARTITIONER/SCYLLA_SHARDING_ALGORITHMwas present withoutSCYLLA_SHARD. Default to0.cassandra/shard_info.py/cassandra/c_shard_info.pyx_ShardingInfo.__init__/ShardingInfo.__init__: guardint(shards_count)andint(sharding_ignore_msb)againstNone(default to0), matching the existing pattern used forshard_aware_port. Fixes the remaining crash path where a shard-defining field was present withoutSCYLLA_NR_SHARDS.cassandra/cluster.py_uses_peers_v2disable and_metadata_request_timeoutnow gate onis_scyllainstead ofsharding_info is not None, so both work correctly when sharding is disabled.cassandra/metadata.py_is_not_scylla()now usesnot is_scyllainstead ofshard_id is None.The old check was always
Falseafter the OPTIONS exchange (Cassandra getsshard_id=0, and0 is NoneisFalse), meaning trigger-metadata queries were silently skipped even for Cassandra/DSE connections.tests/unit/test_protocol_features.pyint(None)paths.Shard-aware pooling is unaffected
pool.pycontinues to gate per-shard connection opening onsharding_info is not None.A ScyllaDB node with sharding disabled correctly gets
is_scylla=True(so peers_v2 is disabled and USING TIMEOUT works) butsharding_info=None(so no per-shard connections are opened). This matches the gocql#903 fix'sisScyllaConn() && nrShards != 0guard.Note on Cassandra behavioral change
_is_not_scylla()was previously always returningFalsepost-OPTIONS for Cassandra (shard_id=0,0 is None = False), sosystem_schema.triggersqueries were silently skipped even for Cassandra/DSE. With this fix, Cassandra/DSE connectionswill correctly issue the trigger-metadata query and populate
TableMetadata.triggers.See also