improvement: maintain token-aware routing even when schema metadata refresh is disabled - #977
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:
📝 WalkthroughWalkthroughThe driver preserves token-aware routing when Sequence Diagram(s)sequenceDiagram
participant Driver
participant SystemSchema
participant Metadata
participant TokenMap
Driver->>SystemSchema: Query keyspace replication strategies
SystemSchema-->>Driver: Return strategy rows
Driver->>Metadata: Apply strategy updates
Metadata->>TokenMap: Rebuild affected mappings
TokenMap-->>Driver: Provide replicas for token-aware routing
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
mykaul
changed the title
|
@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.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
tests/unit/test_metadata.py-474-474 (1)
474-474: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove unused unpacked values.
Line 474 triggers RUF059. Replace
tokensandhostswith_.🤖 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_metadata.py` at line 474, Update the unpacking assignment in the test method around _build_metadata so the unused tokens and hosts values are assigned to _ while retaining metadata for subsequent use.Sources: Coding guidelines, Linters/SAST tools
tests/integration/standard/test_metadata.py-182-197 (1)
182-197: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease the cluster when an assertion fails.
Wrap the test body in
try/finally. A failed assertion currently skipsno_schema.shutdown()and can affect later tests.🤖 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/integration/standard/test_metadata.py` around lines 182 - 197, Wrap the no-schema cluster setup and assertions in the test body with a try/finally block, keeping no_schema.shutdown() in the finally clause so it executes even when an assertion fails. Preserve the existing metadata, token-aware routing, and query assertions unchanged.
🤖 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.
Other comments:
In `@tests/integration/standard/test_metadata.py`:
- Around line 182-197: Wrap the no-schema cluster setup and assertions in the
test body with a try/finally block, keeping no_schema.shutdown() in the finally
clause so it executes even when an assertion fails. Preserve the existing
metadata, token-aware routing, and query assertions unchanged.
In `@tests/unit/test_metadata.py`:
- Line 474: Update the unpacking assignment in the test method around
_build_metadata so the unused tokens and hosts values are assigned to _ while
retaining metadata for subsequent use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: a92165a5-a7d9-41fc-8c1d-528c5710a44f
📒 Files selected for processing (6)
CHANGELOG.rstcassandra/cluster.pycassandra/metadata.pytests/integration/standard/test_metadata.pytests/unit/test_control_connection.pytests/unit/test_metadata.py
601d0e0 to
b8d6342
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
tests/unit/test_metadata.py-545-552 (1)
545-552: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify the no-rebuild contract.
Assert that
TokenMap.rebuild_keyspaceis not called. The copied mapping can compare equal after a rebuild, so Line 552 cannot detect the claimed no-rebuild behavior.🤖 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_metadata.py` around lines 545 - 552, Update the test around _update_replication_strategies to mock or spy on TokenMap.rebuild_keyspace before invoking it, then assert that the method was not called. Retain the existing tablet and token mapping assertions, but do not rely on mapping equality alone to verify the no-rebuild contract.tests/integration/standard/test_metadata.py-190-197 (1)
190-197: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise token-aware host selection.
Execute a statement for
self.ks_namewith a routing key. Assert that its query plan or coordinator is a resolved replica. Thesystem.localquery does not useself.ks_nameor a routing key, so it does not test token-aware routing.🤖 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/integration/standard/test_metadata.py` around lines 190 - 197, Replace the `system.local` query in the token-aware routing test with a statement executed against `self.ks_name` using a routing key. Assert that the resulting query plan or coordinator resolves to one of the replicas returned by `token_map.get_replicas`, preserving the existing replica lookup and validation.
🤖 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.
Other comments:
In `@tests/integration/standard/test_metadata.py`:
- Around line 190-197: Replace the `system.local` query in the token-aware
routing test with a statement executed against `self.ks_name` using a routing
key. Assert that the resulting query plan or coordinator resolves to one of the
replicas returned by `token_map.get_replicas`, preserving the existing replica
lookup and validation.
In `@tests/unit/test_metadata.py`:
- Around line 545-552: Update the test around _update_replication_strategies to
mock or spy on TokenMap.rebuild_keyspace before invoking it, then assert that
the method was not called. Retain the existing tablet and token mapping
assertions, but do not rely on mapping equality alone to verify the no-rebuild
contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: ab2b0a17-e305-4a14-bbc5-ab7a6214aeef
📒 Files selected for processing (2)
tests/integration/standard/test_metadata.pytests/unit/test_metadata.py
2acb9bf to
8d6940c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
tests/integration/standard/test_metadata.py-202-203 (1)
202-203: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_current_hostcan be a non-replica after a retry.
_current_hostholds the last host attempted, not the first host the query plan produced. If the first replica fails and the retry policy moves to the next host, the assertion fails on a healthy cluster. Assert onrs.response_future.attempted_hosts[0]instead.💚 Proposed fix
replicas = set(no_schema.metadata.get_replicas(self.ks_name, statement.routing_key)) - assert rs.response_future._current_host in replicas + assert rs.response_future.attempted_hosts[0] in replicas🤖 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/integration/standard/test_metadata.py` around lines 202 - 203, Update the assertion in the metadata replica test to check rs.response_future.attempted_hosts[0] against replicas instead of _current_host, preserving the existing replica set calculation.
🧹 Nitpick comments (3)
tests/unit/test_metadata.py (1)
789-812: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese two tests do not exercise driver code.
test_consistency_modes_are_mapped_from_rowsrebuilds the dict comprehension from_aggregate_resultsinside the test.test_keyspace_absent_from_the_map_is_eventualonly assertsdict.getdefault behavior. Both pass even ifSchemaParserV3breaks. Callparser._aggregate_results()and assert onparser.keyspace_consistency_modesinstead.🤖 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_metadata.py` around lines 789 - 812, Update test_consistency_modes_are_mapped_from_rows to invoke parser._aggregate_results() and assert the resulting parser.keyspace_consistency_modes values, rather than recreating the mapping with _consistency_mode_from_string. Update test_keyspace_absent_from_the_map_is_eventual to exercise _aggregate_results() with the relevant empty or missing-row setup, then assert the parser’s resulting map fallback behavior so both tests fail if SchemaParserV3 aggregation breaks.cassandra/cluster.py (1)
4168-4190: 🚀 Performance & Scalability | 🔵 TrivialA keyspace event triggers a full-table read.
_refresh_replication_strategiesreads all ofsystem_schema.keyspacesfor every keyspace event, even whenkwargs["keyspace"]names one keyspace. The full read is required to detect dropped keyspaces, so the cost is justified. Confirm that the event rate on clusters with frequent DDL stays acceptable, and consider a filtered read forCREATED/UPDATEDwith the full read reserved forDROPPED.🤖 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 `@cassandra/cluster.py` around lines 4168 - 4190, Update _refresh_schema and _refresh_replication_strategies so CREATED or UPDATED keyspace events use a keyspace-filtered metadata read based on kwargs["keyspace"], while DROPPED events retain the full system_schema.keyspaces read needed to detect removals. Preserve token-aware routing updates and verify the filtered path is used for frequent keyspace DDL events without changing schema-disabled behavior.cassandra/metadata.py (1)
1887-1897: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or deprecate
TokenMap.replica_map_for_keyspace.No in-repository callers remain. Remove it if it is not part of the supported public API; otherwise mark it deprecated.
🤖 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 `@cassandra/metadata.py` around lines 1887 - 1897, The TokenMap replica_map_for_keyspace API is unused in the repository; remove it if unsupported, or mark it deprecated while preserving behavior if it is public. Update the TokenMap definition and any related documentation or exports consistently.
🤖 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 `@cassandra/cluster.py`:
- Around line 4151-4166: Update the strategy-fetching flow around
ReplicationStrategy.create so failures parsing an individual row are handled per
row, logged, and do not prevent valid rows from being added or
_update_replication_strategies from being called. Keep the connection wait/query
failure as a separate guard that returns, but log that fetch failure at warning
instead of debug.
---
Other comments:
In `@tests/integration/standard/test_metadata.py`:
- Around line 202-203: Update the assertion in the metadata replica test to
check rs.response_future.attempted_hosts[0] against replicas instead of
_current_host, preserving the existing replica set calculation.
---
Nitpick comments:
In `@cassandra/cluster.py`:
- Around line 4168-4190: Update _refresh_schema and
_refresh_replication_strategies so CREATED or UPDATED keyspace events use a
keyspace-filtered metadata read based on kwargs["keyspace"], while DROPPED
events retain the full system_schema.keyspaces read needed to detect removals.
Preserve token-aware routing updates and verify the filtered path is used for
frequent keyspace DDL events without changing schema-disabled behavior.
In `@cassandra/metadata.py`:
- Around line 1887-1897: The TokenMap replica_map_for_keyspace API is unused in
the repository; remove it if unsupported, or mark it deprecated while preserving
behavior if it is public. Update the TokenMap definition and any related
documentation or exports consistently.
In `@tests/unit/test_metadata.py`:
- Around line 789-812: Update test_consistency_modes_are_mapped_from_rows to
invoke parser._aggregate_results() and assert the resulting
parser.keyspace_consistency_modes values, rather than recreating the mapping
with _consistency_mode_from_string. Update
test_keyspace_absent_from_the_map_is_eventual to exercise _aggregate_results()
with the relevant empty or missing-row setup, then assert the parser’s resulting
map fallback behavior so both tests fail if SchemaParserV3 aggregation breaks.
🪄 Autofix
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: QUIET
Plan: Pro Plus
Run ID: 1d512260-c276-4bea-8dc0-e6fc118bc42c
📒 Files selected for processing (4)
cassandra/cluster.pycassandra/metadata.pytests/integration/standard/test_metadata.pytests/unit/test_metadata.py
With schema_metadata_enabled=False the driver skipped all schema refresh, so the token map never had keyspace replica information and token-aware routing was silently given away. Fetch keyspace replication strategies from the lightweight system_schema.keyspaces table on connect and on keyspace schema change events, keep them in Metadata._keyspace_replication_strategies, and have TokenMap fall back to them when keyspace metadata is unavailable. Dropped table events are also processed so tablet metadata stays up to date. Schema keyspace events are still ignored as before.
8d6940c to
e393697
Compare
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 `@cassandra/cluster.py`:
- Around line 4178-4195: Move the _wait_for_schema_agreement call ahead of the
not self._schema_meta_enabled and not force branch so
_refresh_replication_strategies uses post-agreement schema data. Preserve
schema_agreement_wait=-1 for the initial connection path, and add a regression
test covering stale pre-agreement replication results to verify token-aware
routing refreshes correctly.
🪄 Autofix
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: QUIET
Plan: Pro Plus
Run ID: 63c83f51-c10c-4037-8195-e1e886f53429
📒 Files selected for processing (2)
cassandra/cluster.pytests/unit/test_control_connection.py
- Wait for schema agreement before the disabled-metadata branch so the CL.ONE replication read cannot return pre-agreement data - Filter the keyspace read to the changed keyspace on keyspace events, merging the result into the known strategies (a missing row drops it) - Deprecate the now-unused TokenMap.replica_map_for_keyspace - Assert on the first attempted host in the integration test, exercise the real _aggregate_results in the consistency-mode unit tests, and add regression tests for the agreement-first behavior Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cassandra/cluster.py (1)
4163-4174: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle strategy-construction failures per row.
ReplicationStrategy.create()is outside the guard. ASimpleStrategyrow withoutreplication_factorcan raiseKeyErrorand abort the refresh. Keep the existing strategy for that keyspace and continue processing other rows. Add a regression case for this malformed options map.🤖 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 `@cassandra/cluster.py` around lines 4163 - 4174, Update the per-row processing around ReplicationStrategy.create so strategy-construction exceptions, including missing SimpleStrategy replication_factor, are caught without aborting the refresh. Preserve the existing strategy for the affected keyspace and continue processing subsequent rows, and add a regression test covering this malformed options map.
🤖 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.
Duplicate comments:
In `@cassandra/cluster.py`:
- Around line 4163-4174: Update the per-row processing around
ReplicationStrategy.create so strategy-construction exceptions, including
missing SimpleStrategy replication_factor, are caught without aborting the
refresh. Preserve the existing strategy for the affected keyspace and continue
processing subsequent rows, and add a regression test covering this malformed
options map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: f1015cda-1df5-4aa1-a56d-0c7b83107ceb
📒 Files selected for processing (5)
cassandra/cluster.pycassandra/metadata.pytests/integration/standard/test_metadata.pytests/unit/test_control_connection.pytests/unit/test_metadata.py
1 participant
Motivation
With
schema_metadata_enabled=False, the driver skipped all schema refresh, so the token map never had keyspace replica information and token-aware routing was silently given away — even though it was the stated cost of disabling schema metadata.Change
Fetch keyspace replication strategies from the lightweight
system_schema.keyspacestable on connect and on keyspace schema change events, and use them for token-aware routing when full keyspace metadata is unavailable. Tablet metadata is invalidated for dropped tables and keyspaces, and for keyspace replication changes. No change to token-aware routing behavior when schema metadata is enabled.Tests
Added unit tests covering replica lookup, token map rebuild, strategy add/remove/change, keyspace metadata sync, control connection event handling, and graceful degradation on fetch failure; added an integration test verifying token-aware routing with schema metadata disabled. Full unit suite passes (783 tests).