Implement TABLETS_ROUTING_V2#913
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 adds tablets routing v2 negotiation, tablet-version blocks on prepared executions, connection-level routing, and custom-payload caching. Schema parsing tracks strongly consistent keyspaces, while token-aware plans can prioritize tablet leaders. Unit and integration tests cover protocol behavior, metadata refresh, shard routing, payload decoding, and leader selection. Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponseFuture
participant HostConnection
participant ScyllaServer
participant ClusterMetadata
Client->>ResponseFuture: execute prepared query
ResponseFuture->>HostConnection: borrow connection with routing context
HostConnection->>ScyllaServer: send EXECUTE with tablet version block
ScyllaServer-->>ResponseFuture: return tablet routing payload when stale
ResponseFuture->>ClusterMetadata: cache tablet metadata
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 |
There was a problem hiding this comment.
Pull request overview
Implements Scylla TABLETS_ROUTING_V2 negotiation and request/response handling in the driver, including encoding the per-request tablet_version_block, parsing the new tablets-routing-v2 custom payload (with tablet_version), and adding leader-aware routing behavior for strongly-consistent keyspaces.
Changes:
- Add TABLETS_ROUTING_V2 protocol feature negotiation and ensure V2 subsumes V1 in STARTUP options.
- Attach a per-request
tablet_version_blockto EXECUTE messages on V2-negotiated connections; parse and cache V2 tablet routing payloads includingtablet_version. - Introduce strongly-consistent keyspace detection (Scylla-only) and leader-first routing in
TokenAwarePolicy; add unit and integration coverage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_tablets.py | Adds unit tests for tablet_version_block encoding and for storing tablet_version on Tablet. |
| tests/unit/test_protocol_features.py | Adds negotiation tests ensuring V2 is preferred over V1 in STARTUP options. |
| tests/unit/test_policies.py | Adds unit tests for leader-aware routing behavior (leader-first, fallback, and non-SC cases). |
| tests/integration/standard/test_tablets_routing_v2.py | Adds opt-in end-to-end tests validating negotiation, payload decoding, and wire behavior against a V2-capable Scylla cluster. |
| cassandra/tablets.py | Adds tablet_version_block helpers and extends Tablet to store tablet_version. |
| cassandra/query.py | Adds memoization for routing-key token computation via Statement.routing_token(). |
| cassandra/protocol.py | Extends ExecuteMessage to optionally append tablet_version_block byte when present. |
| cassandra/protocol_features.py | Adds TABLETS_ROUTING_V2 constant and negotiation logic; makes V2 mutually exclusive with V1 in STARTUP. |
| cassandra/pool.py | Adds per-pool V2 capability detection and reuses memoized routing token to avoid repeated hashing. |
| cassandra/policies.py | Adds leader-first routing for strongly-consistent keyspaces when tablet replicas are available. |
| cassandra/metadata.py | Adds KeyspaceMetadata.strongly_consistent and populates it from Scylla system_schema.scylla_keyspaces. |
| cassandra/cluster.py | Computes/attaches tablet_version_block per connection and parses V2 routing payloads based on the serving connection’s negotiated features. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/unit/test_policies.py (1)
1043-1082: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: test name/docstring don't match what's actually exercised.
This test asserts the "no tablet_version (V1)" path, but the production code (
make_query_plan) never readstablet.tablet_version; leader-first is gated solely onks_meta.strongly_consistent, which is set toFalsehere. So the test passes because of thestrongly_consistent=Falseflag, not becausetablet_version=None. Consider renaming/reframing it (or settingstrongly_consistent=Truewithtablet_version=None) so the test actually guards the behavior its name implies; otherwise it overlaps withtest_no_leader_routing_for_eventually_consistent_keyspace.🤖 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_policies.py` around lines 1043 - 1082, The test name and docstring in test_no_leader_routing_without_tablet_version do not match the behavior actually being exercised, because make_query_plan only gates leader-first routing on keyspace metadata strong consistency, not Tablet.tablet_version. Update the test so it truly covers the intended case by either renaming/reframing it to reflect strongly_consistent=False, or by setting cluster.metadata.keyspaces['ks'].strongly_consistent to True while keeping tablet_version=None to verify the V1 tablet path; keep the assertions aligned with the TokenAwarePolicy routing behavior.cassandra/policies.py (1)
510-514: 🚀 Performance & Scalability | 🔵 TrivialOptional: Cache the child query plan to prevent double invocation and potential inconsistency.
child.make_query_plan(keyspace, query)is invoked at line 512 to filter replicas and is called again at line 551 to build the final plan. If the child policy is stateful (e.g.,RoundRobinPolicyorDCAwareRoundRobinPolicy), the second invocation may advance internal pointers and return a different host sequence, causing the filtering logic to diverge from the execution order and introducing unnecessary overhead.♻️ Suggested approach
Materialize the child plan once and reuse it:
- if tablet is not None: - replicas_mapped = set(map(lambda r: r[0], tablet.replicas)) - child_plan = child.make_query_plan(keyspace, query) - - replicas = [host for host in child_plan if host.host_id in replicas_mapped] + child_plan = list(child.make_query_plan(keyspace, query)) + if tablet is not None: + replicas_mapped = set(map(lambda r: r[0], tablet.replicas)) + replicas = [host for host in child_plan if host.host_id in replicas_mapped]Update the subsequent loop (line 551) to iterate over the cached
child_planinstead of re-invokingchild.make_query_plan(...).🤖 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/policies.py` around lines 510 - 514, Cache the result of child.make_query_plan(keyspace, query) in the policy logic so it is only evaluated once, then reuse that same child_plan for both the replica filtering in the tablet block and the final plan construction loop. Update the query planning flow in the policy method that handles tablet replicas to iterate over the cached child_plan instead of calling child.make_query_plan(...) a second time, which avoids stateful policy divergence and redundant work.
🤖 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 5055-5075: The ExecuteMessage tablet_version_block is only being
set in the pooled connection path, so control-connection fallback can miss the
mandatory V2 trailing byte. Update _query_control_connection() in
cassandra/cluster.py to use the same tablets_routing_v2 check and
_compute_tablet_version_block(self.query) logic as the ExecuteMessage handling
in the main send path, placing it after self._connection = connection and before
connection.send_msg(...) so V2 control connections always include the block and
non-V2 ones do not.
- Around line 5212-5231: The tablet-routing cache is using only
self.query.keyspace when adding tablets, which can store entries under a None
key and miss cache hits for session-level keyspaces. Update the tablet handling
in ResponseFuture to use the effective keyspace consistently, matching
_compute_tablet_version_block() by falling back to self.keyspace when
self.query.keyspace is unset, and then pass that resolved keyspace into
metadata._tablets.add_tablet().
In `@cassandra/metadata.py`:
- Around line 2658-2660: The fallback in metadata schema refresh is too broad
because the except block in the system_schema.scylla_keyspaces read path catches
every Exception, which masks unexpected failures. Narrow the handler around the
code in metadata.py that logs “Could not read system_schema.scylla_keyspaces” so
it only catches the specific expected driver/server schema-unavailable error(s),
and let other refresh errors propagate. Keep the existing debug log and exc_info
behavior for the expected case, but do not use a blanket Exception catch in this
branch.
In `@cassandra/pool.py`:
- Around line 451-453: The tablets_routing_v2 property in Pool should not
iterate the live _connections view directly, because concurrent
replacement/shutdown can mutate it and closed connections can incorrectly keep
V2 enabled. Snapshot the current connections first, then compute the flag only
from still-open/live connections before checking each connection’s
features.tablets_routing_v2.
In `@cassandra/query.py`:
- Line 278: The cached routing token in Statement is being reused without
considering token_class, which can cause the wrong ring/token to be used across
sessions or clusters. Update the token cache logic in Statement-related routing
code so _routing_token is keyed by token_class as well, and make the
lookup/reuse path in the affected routing methods distinguish tokens by the
partitioner class used. Ensure the changes cover the Statement fields and the
routing/token selection flow that currently reads and writes _routing_token.
---
Nitpick comments:
In `@cassandra/policies.py`:
- Around line 510-514: Cache the result of child.make_query_plan(keyspace,
query) in the policy logic so it is only evaluated once, then reuse that same
child_plan for both the replica filtering in the tablet block and the final plan
construction loop. Update the query planning flow in the policy method that
handles tablet replicas to iterate over the cached child_plan instead of calling
child.make_query_plan(...) a second time, which avoids stateful policy
divergence and redundant work.
In `@tests/unit/test_policies.py`:
- Around line 1043-1082: The test name and docstring in
test_no_leader_routing_without_tablet_version do not match the behavior actually
being exercised, because make_query_plan only gates leader-first routing on
keyspace metadata strong consistency, not Tablet.tablet_version. Update the test
so it truly covers the intended case by either renaming/reframing it to reflect
strongly_consistent=False, or by setting
cluster.metadata.keyspaces['ks'].strongly_consistent to True while keeping
tablet_version=None to verify the V1 tablet path; keep the assertions aligned
with the TokenAwarePolicy routing behavior.
🪄 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: 587d3541-27be-4132-8dda-c90e66cff85c
📒 Files selected for processing (12)
cassandra/cluster.pycassandra/metadata.pycassandra/policies.pycassandra/pool.pycassandra/protocol.pycassandra/protocol_features.pycassandra/query.pycassandra/tablets.pytests/integration/standard/test_tablets_routing_v2.pytests/unit/test_policies.pytests/unit/test_protocol_features.pytests/unit/test_tablets.py
|
v2:
Tested against scylladb/scylladb#30291 (scylladb/scylladb@58ab555). |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/metadata.py`:
- Around line 805-810: Update the documentation for the strongly_consistent
metadata field so it matches the behavior in _is_strongly_consistent() and the
new tests. In the keyspace metadata docstring near strongly_consistent, change
the description to state that only ScyllaDB keyspaces with consistency set to
global are marked True, while local, eventual, and non-ScyllaDB cases remain
False. Keep the public contract aligned with the implementation by tightening
the wording around strongly_consistent.
In `@tests/integration/standard/test_tablets_routing_v2.py`:
- Around line 279-294: The global monkeypatch on
ProtocolFeatures.add_startup_options in the test setup can leak if Cluster
creation or shutdown fails. Move the restore logic for add_startup_options into
a nested finally that always runs after the patch is applied, and keep the
cleanup tied to the cluster/session lifecycle in the same test helper around
Cluster.connect and cluster.shutdown. Use the existing
ProtocolFeatures.add_startup_options, Cluster, and session setup block to ensure
the original value is restored no matter which step raises.
- Line 41: The module-level skip logic in the test setup is too broad because
the `except Exception as exc` block can mask unrelated startup problems. Narrow
the handler around the cluster start path to only skip on the specific expected
unsupported-feature/startup failure, using the same setup flow where the cluster
is initialized, and re-raise any other exception so real regressions still fail
fast.
🪄 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: 712ffcf1-f346-4fd7-931d-303a5bc26920
📒 Files selected for processing (12)
cassandra/cluster.pycassandra/metadata.pycassandra/policies.pycassandra/pool.pycassandra/query.pycassandra/tablets.pydocs/scylla-specific.rsttests/integration/standard/test_tablets_routing_v2.pytests/unit/test_metadata.pytests/unit/test_policies.pytests/unit/test_response_future.pytests/unit/test_tablets.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/test_tablets.py
- cassandra/policies.py
- cassandra/query.py
- tests/unit/test_policies.py
- cassandra/tablets.py
- cassandra/cluster.py
| # advertisement of TABLETS_ROUTING_V2_EXPERIMENTAL. | ||
| 'experimental_features': ['lwt', 'udf', 'strongly-consistent-tables'], | ||
| }) | ||
| except Exception as exc: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find existing integration-test patterns and harness exception types to catch specifically.
rg -n -C3 "def use_cluster|pytest.skip\\(|Could not start a Scylla cluster|except .*Exception" tests cassandraRepository: scylladb/python-driver
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tests/integration/standard/test_tablets_routing_v2.py =="
nl -ba tests/integration/standard/test_tablets_routing_v2.py | sed -n '1,120p'
echo
echo "== tests/integration/__init__.py (use_cluster) =="
nl -ba tests/integration/__init__.py | sed -n '421,560p'
echo
echo "== integration test patterns for skip/start failures =="
rg -n -C2 "pytest\.skip\(|Could not start|except .*Exception|DependencyException|NoHostAvailable|OperationTimedOut|FeatureNotSupported|Unsupported" tests/integration tests | sed -n '1,220p'Repository: scylladb/python-driver
Length of output: 260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tests/integration/standard/test_tablets_routing_v2.py =="
sed -n '1,140p' tests/integration/standard/test_tablets_routing_v2.py | cat -n
echo
echo "== tests/integration/__init__.py (use_cluster) =="
sed -n '421,560p' tests/integration/__init__.py | cat -n
echo
echo "== relevant integration skip / exception patterns =="
rg -n -C2 "pytest\.skip\(|Could not start|DependencyException|Unsupported|OperationTimedOut|NoHostAvailable|except .*Exception" tests/integration tests | sed -n '1,220p'Repository: scylladb/python-driver
Length of output: 32197
Limit the module-level skip to the expected startup failure. Catching Exception here can hide setup bugs, port conflicts, and config regressions behind a skip. Narrow this to the cluster-start/unsupported-feature failure you expect, or re-raise everything else.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 41-41: Do not catch blind exception: Exception
(BLE001)
🤖 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_tablets_routing_v2.py` at line 41, The
module-level skip logic in the test setup is too broad because the `except
Exception as exc` block can mask unrelated startup problems. Narrow the handler
around the cluster start path to only skip on the specific expected
unsupported-feature/startup failure, using the same setup flow where the cluster
is initialized, and re-raise any other exception so real regressions still fail
fast.
Source: Linters/SAST tools
|
v2.1:
Also tested against scylladb/scylladb#30291 (scylladb/scylladb@58ab555). |
| # ScyllaDB-only: per-keyspace consistency option. The column is null for | ||
| # eventually-consistent keyspaces (and the whole table is absent on Cassandra | ||
| # and on Scylla versions without strongly-consistent tablets). | ||
| _SELECT_SCYLLA_KEYSPACES = "SELECT keyspace_name, consistency FROM system_schema.scylla_keyspaces" |
There was a problem hiding this comment.
It's a real pity that we need to perform yet another fetch from system_schema in order to obtain this information - especially when strong consistency is still experimental, people will be paying for support for a feature which they not only do not use, but can't use.
Maybe we should have considered sending some information about whether a table uses strong consistency or not in the prepared statement. I think this could also apply to information like the partitioner and whether a table uses tablets or not. This is, of course, out of scope.
There was a problem hiding this comment.
That's a good point. However, if we want to avoid it, we need to modify the server-code before we can merge this PR since we need some way to learn that a table is strongly consistent. I can start working on it in parallel of course.
There was a problem hiding this comment.
I think that modifications like those I propose would require another round of design review. I don't think we should be implementing this at the moment. The other suggestion about skipping the query to scylla_keyspaces should make the current PR palatable enough, we will only pay the cost on experimental clusters with strong consistency enabled.
There was a problem hiding this comment.
Alright, we can remove this code for the time being. Unfortunately, the consequence will be ditching all leader awareness from the driver, but it shouldn't be difficult to add it back later on. If that's acceptable, I can proceed with the changes.
There was a problem hiding this comment.
Unfortunately, the consequence will be ditching all leader awareness from the driver
That would defeat the whole point of this PR.
What I meant by my "we should not be implementing this at the moment" is that we should not be working on further extending the protocol in the way as I suggested in my first message. I did not mean to drop the code that this conversation is attached to (cassandra/metadata.py, lines 2590-2593), we need it to distinguish whether it's a strongly consistent table or not and whether to do leader awareness routing or not.
In #913 (comment) I suggested that we can skip issuing the query if we know that the cluster does not support strong consistency. If we do this, we will not incur the cost for regular users who are not testing strong consistency at the moment. While not great, I don't think an additional metadata query is tragic; we still have some time before release of strong consistency to address it (if there will be a need to address it at all, given the python-over-rust effort).
There was a problem hiding this comment.
Gated behind the protocol extension in v3. Leaving the discussion as open since this is still something we might want to improve. It should be easier to remember this way until we create an issue.
Add per-connection negotiation of the TABLETS_ROUTING_V2 extension, the successor to TABLETS_ROUTING_V1. When the server advertises it in the SUPPORTED response, the driver echoes it back during STARTUP to opt in; a driver that negotiates v2 does not negotiate v1. While the feature is experimental the wire name carries the `_EXPERIMENTAL` suffix (TABLETS_ROUTING_V2_EXPERIMENTAL), and the server only advertises it when started with the `strongly-consistent-tables` experimental feature enabled. Also add the optional trailing tablet_version_block byte to the EXECUTE message body, written only when set, so later commits can carry the cached tablet version to the server.
Store the server-provided 64-bit tablet_version on each cached Tablet (None until learned) and add helpers to encode it into the one-byte tablet_version_block exchanged on the wire: * Tablet.from_row normalizes the version to an unsigned 64-bit value. The server sends an unsigned hash, but the driver deserializes the payload field as a signed long, so the raw value can come back negative; masking to [0, 2**64) keeps the nibble extraction in choose_tablet_version_block consistent with the server's unsigned layout. * choose_tablet_version_block() packs a randomly chosen block index in the high nibble and that block's value in the low nibble, matching the server's locator::compare_tablet_version_block layout. Blocks are indexed from the least significant bits, so block i covers bits [i*4, i*4 + 4) of the version. A random index avoids any shared mutable counter on the hot path while still probing every nibble often enough to detect a server-side version change quickly. * random_tablet_version_block() returns a random byte for cold start, when no version is cached yet.
Add Statement.routing_token(token_class), which hashes the routing key of the statement once and caches the result, keyed on the token class so a change of partitioner invalidates the cached token. The load balancing policy and the pool's shard selection reuse it instead of each re-hashing the routing key on the hot path; the statement is threaded down through borrow_connection so the pool can reach the memoized token. The tablet_version_block computation added in the following commit reuses the same token as well.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/connection.py`:
- Around line 1224-1226: Preserve backward compatibility in
send_msg()/encode_message handling: the new protocol_features argument passed
from self._protocol_handler.encode_message can break custom protocol handler
subclasses that still implement the older signature. Update the call path in
CassandraConnection.send_msg (and any related encode_message wrapper) to either
accept and ignore extra keyword arguments via **kwargs or add a compatibility
shim that only passes protocol_features when the handler supports it, keeping
existing subclasses working.
In `@tests/integration/standard/test_tablets_routing_v2.py`:
- Around line 155-156: The assertion in the tablet routing test is checking the
bound supports_tablet_routing method object instead of its return value. Update
the loop over self.session._pools.values() to invoke supports_tablet_routing()
on each pool before asserting, so the test validates the negotiated feature
rather than the method reference itself.
- Around line 69-77: The setup in the cluster bootstrap path can leak resources
if Cluster.connect() or _create_schema() fails before teardown_class runs. Wrap
the logic in the class setup flow around cls.cluster, cls.session, and
cls._create_schema so any exception triggers immediate cluster shutdown/cleanup
before re-raising, and make sure the cleanup is tied to the existing setup
method that initializes the Cluster and session.
🪄 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: 359bd0fd-f061-419c-83e3-12b654fcc40e
📒 Files selected for processing (16)
cassandra/cluster.pycassandra/connection.pycassandra/metadata.pycassandra/policies.pycassandra/pool.pycassandra/protocol.pycassandra/protocol_features.pycassandra/query.pycassandra/tablets.pydocs/scylla-specific.rsttests/integration/standard/test_tablets_routing_v2.pytests/unit/test_metadata.pytests/unit/test_policies.pytests/unit/test_protocol_features.pytests/unit/test_response_future.pytests/unit/test_tablets.py
✅ Files skipped from review due to trivial changes (1)
- docs/scylla-specific.rst
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/unit/test_protocol_features.py
- cassandra/tablets.py
- cassandra/protocol_features.py
- tests/unit/test_response_future.py
- tests/unit/test_metadata.py
- cassandra/query.py
- tests/unit/test_policies.py
- cassandra/policies.py
- cassandra/pool.py
- tests/unit/test_tablets.py
|
v3:
|
|
v4:
What remains to be fixed is the violation of a public API pointed out by coderabbit here: #913 (comment). |
|
The new CI failure seems unrelated to these changes, but I didn't find it in the Issues on GitHub or Jira. Summary of it (since the logs are going to be deleted soon. I have them saved, but let's persist at least this here too): @Lorak-mmk can you take a look and confirm it's unrelated? Then I can create an issue for it. Btw. I asked Claude to summarise the full logs. This is what it said: Bottom lineThe single failure is unrelated to your PR. It's a Client Routes / Private Link (NLB) test, and your branch doesn't touch that feature or test at all ( What failedThe test is a 4-stage topology-churn scenario (start 3 nodes → bootstrap 3 → decommission the original 3 → verify session survives). It fails in Stage 4, at test_client_routes.py: right after url = "http://%s:10000/v2/client-routes" % contact_point # Scylla REST API, port 10000
...
response = urllib.request.urlopen(req) # <-- returns 500Port 10000 is Scylla's own REST API, so the 500 originates from the Scylla node (release 2026.1.8), not from the test's Noise to ignore
|
|
Doesn't sound like something you could cause. @sylwiaszunejko is a better person to look at this, she implemented PrivateLink tests IIRC. |
Agree, looks like this test is flaky, probably there is a need to retry mechanism in it, it shouldn't block you |
With TABLETS_ROUTING_V2 the server returns, on a tablet_version mismatch, the tablet's replica set plus the new tablet_version, so the driver can keep its routing cache fresh without the per-response overhead v1 incurs. * Every EXECUTE on a V2 connection carries a tablet_version_block computed from the cached version -- or a random byte on cold start (a token-aware request with no cached version yet), or 0 for a non-token-aware request, which the server never version-checks. * On the response, the routing payload is parsed according to what the serving connection negotiated; the v2 tuple additionally carries the tablet_version, which is stored back on the tablet. The block value is connection-independent, so it is computed once when the ExecuteMessage is built and the serializer decides whether to emit it. The connection's negotiated ProtocolFeatures are threaded through send_msg and ProtocolHandler.encode_message into send_body (mirroring decode_message, which already receives them); ExecuteMessage appends the trailing byte only when TABLETS_ROUTING_V2 was negotiated. On a V2 connection the server always reads exactly one trailing byte per EXECUTE, so a message whose block was never computed coalesces to 0 to keep the frame in sync. The optional protocol_features parameter added to encode_message/send_body is backwards compatible for in-tree callers; third-party message types or protocol handlers overriding these would need the same optional parameter. The memoized Statement.routing_token from the previous commit is reused when computing the tablet_version_block, so a token-aware EXECUTE hashes its routing key only once on this hot path. Refs: SCYLLADB-288 Refs: SCYLLADB-291
|
v5:
We discussed the API problem with @Lorak-mmk and @nikagra and decided to ignore it. It's already been modified before (in the same way!) here: f36ba79#diff-5eba513e7d4e3e68df22b9a5973828907fe6caa4730581bf5c318e890835f163L1163-R1166. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/standard/test_tablets_routing_v2.py (1)
300-321: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMonkeypatch leak on
ProtocolFeatures.add_startup_optionsis still present — previous fix appears lost.This code is identical to the "before" state from the earlier review (marked "✅ Addressed in commits 766898d to 8dedad4"):
Cluster(...)is constructed beforetry, so if construction raises,finallynever runs andProtocolFeatures.add_startup_optionsstays patched globally for the rest of the test run. Also,cluster.shutdown()runs before restoringoriginalinfinally, so ashutdown()failure also skips the restore. This likely regressed during the v2.1 rebase.🔧 Proposed fix
original = ProtocolFeatures.add_startup_options ProtocolFeatures.add_startup_options = _startup_with_both_extensions -cluster = Cluster(contact_points=["127.0.0.1", "127.0.0.2", "127.0.0.3"], - protocol_version=PROTOCOL_VERSION, - execution_profiles={ - EXEC_PROFILE_DEFAULT: ExecutionProfile( - load_balancing_policy=TokenAwarePolicy(RoundRobinPolicy())) - }, - reconnection_policy=ConstantReconnectionPolicy(1)) +cluster = None try: + cluster = Cluster(contact_points=["127.0.0.1", "127.0.0.2", "127.0.0.3"], + protocol_version=PROTOCOL_VERSION, + execution_profiles={ + EXEC_PROFILE_DEFAULT: ExecutionProfile( + load_balancing_policy=TokenAwarePolicy(RoundRobinPolicy())) + }, + reconnection_policy=ConstantReconnectionPolicy(1)) session = cluster.connect('test_v2') self._wait_for_shard_connections(session) yield cluster, session finally: - cluster.shutdown() ProtocolFeatures.add_startup_options = original + if cluster is not None: + cluster.shutdown()A leaked patch silently alters protocol negotiation for every
Clustercreated afterward in the same process, which can cause hard-to-diagnose failures in unrelated 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_tablets_routing_v2.py` around lines 300 - 321, Fix the cleanup ordering in _cluster_with_v1_and_v2: move Cluster construction inside the protected block so construction failures cannot leak the ProtocolFeatures.add_startup_options monkeypatch, and restore the original method before calling cluster.shutdown(). Preserve the existing shutdown and session-yield behavior while ensuring restoration runs even when construction or shutdown raises.
🧹 Nitpick comments (1)
tests/unit/test_policies.py (1)
1332-1333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefix unused unpacked variable with an underscore.
The
other_replicavariable is unpacked from the_make_leader_routing_setuptest fixture but is never used in these test methods. Replacing it with_clarifies intent and resolves static analysis warnings.
tests/unit/test_policies.py#L1332-L1333: replaceother_replicawith_.tests/unit/test_policies.py#L1348-L1348: replaceother_replicawith_.🧹 Proposed fixes
For
tests/unit/test_policies.py(lines 1332-1333):- policy, leader, other_replica = self._make_leader_routing_setup( + policy, leader, _ = self._make_leader_routing_setup( default_consistency_level=ConsistencyLevel.LOCAL_QUORUM)For
tests/unit/test_policies.py(line 1348):- policy, leader, other_replica = self._make_leader_routing_setup() + policy, leader, _ = self._make_leader_routing_setup()🤖 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_policies.py` around lines 1332 - 1333, In tests/unit/test_policies.py at lines 1332-1333 and 1348, update the unpacking from _make_leader_routing_setup so the unused other_replica value is assigned to _ instead. No other behavior changes are needed.Source: Linters/SAST tools
🤖 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 `@tests/integration/standard/test_tablets_routing_v2.py`:
- Around line 300-321: Fix the cleanup ordering in _cluster_with_v1_and_v2: move
Cluster construction inside the protected block so construction failures cannot
leak the ProtocolFeatures.add_startup_options monkeypatch, and restore the
original method before calling cluster.shutdown(). Preserve the existing
shutdown and session-yield behavior while ensuring restoration runs even when
construction or shutdown raises.
---
Nitpick comments:
In `@tests/unit/test_policies.py`:
- Around line 1332-1333: In tests/unit/test_policies.py at lines 1332-1333 and
1348, update the unpacking from _make_leader_routing_setup so the unused
other_replica value is assigned to _ instead. No other behavior changes are
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc91ecbe-13de-4eb0-9fb1-74f53c0f1c73
📒 Files selected for processing (12)
cassandra/cluster.pycassandra/connection.pycassandra/metadata.pycassandra/policies.pycassandra/pool.pycassandra/protocol.pydocs/scylla-specific.rsttests/integration/standard/test_tablets_routing_v2.pytests/unit/test_metadata.pytests/unit/test_policies.pytests/unit/test_response_future.pytests/unit/test_tablets.py
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/scylla-specific.rst
- cassandra/connection.py
- tests/unit/test_metadata.py
- tests/unit/test_response_future.py
- cassandra/pool.py
- cassandra/policies.py
- cassandra/protocol.py
- cassandra/metadata.py
- cassandra/cluster.py
Cover the end-to-end behaviour against a live ScyllaDB started with the `strongly-consistent-tables` experimental feature: v2 negotiation, payload-driven cache population, and the tablet_version_block matching rules (no payload on a matching block, exactly one matching value per index, and v2 taking precedence over v1 on a wrong-shard request).
Extend the "Tablet Awareness" section of the Scylla-specific guide to cover the V2 protocol extension: the per-connection negotiation and the tablet_version_block byte that lets the server skip re-sending routing information the driver already has.
Add KeyspaceMetadata._strongly_consistent, derived from the per-keyspace `consistency` option in system_schema.scylla_keyspaces. A keyspace is treated as strongly consistent when its consistency is `global`; Scylla only supports `global` for now, so `local`, `null`, and `eventual` are all treated as eventually consistent for routing purposes. The lookup is cached per schema refresh and degrades to "eventually consistent" on non-Scylla clusters or older Scylla versions that lack the table or column. The flag is underscore-prefixed to keep it private: the name is not yet stable and we do not want to commit to a public API for it. It lets the leader-aware routing logic tell which tablet tables actually have a Raft leader.
For a strongly-consistent tablet the TABLETS_ROUTING_V2 server orders the replica set with the Raft leader first (replicas[0]) and keeps it fresh via the tablet_version already tracked in the previous commits. TokenAwarePolicy uses this to send reads and writes for such tables straight to the leader, saving the extra coordinator->leader hop. The leader is yielded first only when the keyspace's _strongly_consistent flag is set and the tablet carries a tablet_version: eventually-consistent tablet tables are assigned a tablet_version too, and a versionless (v1-sourced or stale) tablet must not be mistaken for a leader hint. The hint stays bounded by the wrapped policy -- the leader is front-run only if the child policy would consider it at all (never a host it reports as IGNORED, nor a cross-datacenter leader under a DCAwareRoundRobinPolicy with no remote hosts); otherwise the usual token-aware (optionally shuffled) ordering applies and the server forwards to the leader as it would without v2. Refs: SCYLLADB-288 Fixes: SCYLLADB-291
Extend the TABLETS_ROUTING_V2 integration suite with a strongly-consistent (consistency='global', Raft-backed) keyspace and cover, against a live ScyllaDB: that the driver learns _strongly_consistent from system_schema.scylla_keyspaces (statically and as keyspaces are created and dropped), and that TokenAwarePolicy routes every request for such a table to the Raft leader (replicas[0]).
Extend the Scylla-specific guide's TABLETS_ROUTING_V2 section, which the previous docs commit introduced for tablet-version tracking, to cover leader-aware routing: strongly-consistent (Raft-backed) tablet tables have a leader that the driver targets directly to save the coordinator->leader hop, the behaviour is best-effort and bounded by the load-balancing policy, and eventually-consistent tables keep their usual token-aware ordering.
|
The failed test is still the same -- unrelated. v6:
|
Connection.send_msg now passes the connection's negotiated ProtocolFeatures to the encoder; _ProtocolHandler.encode_message accepts it as a new required protocol_features argument (passed by keyword from send_msg) and forwards it to every message's send_body, which gains the same parameter. Messages carry connection-independent request data; send_body decides the wire format from (protocol_version, protocol_features), so fields belonging to a negotiated protocol extension are emitted exactly on the connections that negotiated it — on every send path, including the control-connection fallback, and without mutating shared message objects per attempt. This is pure plumbing: no message consumes the parameter yet, so no bytes on the wire change. It is groundwork for the SCYLLA_USE_METADATA_ID (scylladb#770) and TABLETS_ROUTING_V2 (scylladb#913) extensions, which must serialize extension fields based on what the serving connection negotiated. The encode side becomes symmetric with decode_message, which already receives protocol_features. This changes the contracted signature of encode_message: custom protocol handlers overriding it must accept the protocol_features keyword argument. The argument is deliberately required, with no default and no fallback for old-style encoders: extensions are negotiated per connection at STARTUP before the per-request handler is known, so an encoder unaware of protocol_features could silently omit fields a negotiated extension requires; omitting it fails fast with TypeError instead. Tests: send_msg hands the connection's features to the encoder; encode_message forwards them into send_body (plain and compressed paths) and raises TypeError when the argument is omitted; a byte-identity suite pins frames for representative messages (v3/v4/v5) to the exact bytes produced before this change, both without features and with all-default features. Co-authored-by: Dawid Mędrek <dawid.medrek@scylladb.com>
Connection.send_msg now passes the connection's negotiated ProtocolFeatures to the encoder; _ProtocolHandler.encode_message accepts it as a new required protocol_features argument (passed by keyword from send_msg) and forwards it to every message's send_body, which gains the same parameter. Messages carry connection-independent request data; send_body decides the wire format from (protocol_version, protocol_features), so fields belonging to a negotiated protocol extension are emitted exactly on the connections that negotiated it — on every send path, including the control-connection fallback, and without mutating shared message objects per attempt. This is pure plumbing: no message consumes the parameter yet, so no bytes on the wire change. It is groundwork for the SCYLLA_USE_METADATA_ID (scylladb#770) and TABLETS_ROUTING_V2 (scylladb#913) extensions, which must serialize extension fields based on what the serving connection negotiated. The encode side becomes symmetric with decode_message, which already receives protocol_features. This changes the contracted signature of encode_message: custom protocol handlers overriding it must accept the protocol_features keyword argument. The argument is deliberately required, with no default and no fallback for old-style encoders: extensions are negotiated per connection at STARTUP before the per-request handler is known, so an encoder unaware of protocol_features could silently omit fields a negotiated extension requires; omitting it fails fast with TypeError instead. Tests: send_msg hands the connection's features to the encoder; encode_message forwards them into send_body (plain and compressed paths) and raises TypeError when the argument is omitted; a byte-identity suite pins frames for representative messages (v3/v4/v5) to the exact bytes produced before this change, both without features and with all-default features. Co-authored-by: Dawid Mędrek <dawid.medrek@scylladb.com>
Connection.send_msg now passes the connection's negotiated ProtocolFeatures to the encoder; _ProtocolHandler.encode_message accepts it as a new required protocol_features argument (passed by keyword from send_msg) and forwards it to every message's send_body, which gains the same parameter. Messages carry connection-independent request data; send_body decides the wire format from (protocol_version, protocol_features), so fields belonging to a negotiated protocol extension are emitted exactly on the connections that negotiated it — on every send path, including the control-connection fallback, and without mutating shared message objects per attempt. This is pure plumbing: no message consumes the parameter yet, so no bytes on the wire change. It is groundwork for the SCYLLA_USE_METADATA_ID (#770) and TABLETS_ROUTING_V2 (#913) extensions, which must serialize extension fields based on what the serving connection negotiated. The encode side becomes symmetric with decode_message, which already receives protocol_features. This changes the contracted signature of encode_message: custom protocol handlers overriding it must accept the protocol_features keyword argument. The argument is deliberately required, with no default and no fallback for old-style encoders: extensions are negotiated per connection at STARTUP before the per-request handler is known, so an encoder unaware of protocol_features could silently omit fields a negotiated extension requires; omitting it fails fast with TypeError instead. Tests: send_msg hands the connection's features to the encoder; encode_message forwards them into send_body (plain and compressed paths) and raises TypeError when the argument is omitted; a byte-identity suite pins frames for representative messages (v3/v4/v5) to the exact bytes produced before this change, both without features and with all-default features. Co-authored-by: Dawid Mędrek <dawid.medrek@scylladb.com>
In dfccfff, we introduced tablet awareness.
Thanks to it, the driver was finally able to learn about the replicas
of a given tablet and successfully route requests to the right nodes
and shards. Unfortunately, the feature isn't sufficient to handle
strongly consistent tables.
Just like an eventually consistent tablet, a strongly consistent one
has a set of replicas that own it. The difference is that the latter
also has a distinguished replica called the leader that coordinates all
of the writes and (almost) all reads to the tablet. That creates a need
for the driver to route its requests to the leader instead of an
arbitrary replica. However, the current tablet awareness doesn't allow
the driver to learn about the identity of the leader.
To solve that problem, we devise a new protocol extension that will
effectively function as a new version of tablet awareness. It affects
both eventually and strongly consistent tablets. This PR implements it
in the driver.
We introduce the notion of a tablet version -- a 64-bit hash that
corresponds to the replicas of a tablet and (in the case of strongly
consistent tablets) its leader.
With every EXECUTE request, the driver provides information about the
tablet version it knows. If the server detects a mismatch, the response
to the request will contain full routing information: the boundary
tokens of the tablet, the list of its replicas, and the tablet version.
The driver then updates its cache and routes subsequent requests to the
right replicas.
Tests have been provided to verify that the implementation is correct.
Fixes: SCYLLADB-291
Refs: SCYLLADB-288
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.