perf: optimize was_applied fast path for known LWT statements - #13
Open
mykaul wants to merge 133 commits into
Open
perf: optimize was_applied fast path for known LWT statements#13mykaul wants to merge 133 commits into
mykaul wants to merge 133 commits into
Conversation
mykaul
force-pushed
the
perf/optimize-was-applied
branch
from
March 16, 2026 15:49
65e2db1 to
d3ee068
Compare
Introduce the data layer for Private Link client routes support: - ClientRoutesChangeType enum for CLIENT_ROUTES_CHANGE event types - ClientRouteProxy dataclass and ClientRoutesConfig for user-facing configuration - _Route frozen dataclass for immutable route records - _RouteStore for thread-safe route storage with atomic update/merge and preferred route selection that avoids unnecessary connection_id migration when multiple routes exist for the same host
Add _ClientRoutesHandler which manages the full lifecycle of dynamic address translation via system.client_routes: - initialize(): loads all routes at startup and on control connection reconnect - handle_client_routes_change(): processes CLIENT_ROUTES_CHANGE events with targeted merge or full refresh depending on event data - _query_all_routes_for_connections(): complete refresh query using connection_id IN (...) - _query_routes_for_change_event(): targeted query grouping by connection_id with host_id IN (...) per group - _execute_routes_query(): common query execution and result parsing with proxy address override support - resolve_host(): host_id to (address, port) resolution with DNS lookup
- ClientRoutesEndPointFactory: creates endpoints from system.peers rows by extracting host_id, deferring address translation and DNS resolution until connection time - ClientRoutesEndPoint: endpoint that resolves via _ClientRoutesHandler on each connection attempt, ensuring immediate reaction to route changes and CLIENT_ROUTES_CHANGE events
Cluster: - Add client_routes_config parameter with mutual exclusivity check against endpoint_factory - Create _ClientRoutesHandler and ClientRoutesEndPointFactory when client_routes_config is provided ControlConnection: - Register CLIENT_ROUTES_CHANGE event watcher when handler is present - Forward events to handler via _handle_client_routes_change - Trigger full route re-read on control connection reconnection
Cover ClientRouteEntry/ClientRoutesConfig validation, _RouteStore get/merge operations, _ClientRoutesHandler initialization, ClientRoutesEndPoint resolution with and without route mappings, and SSL check_hostname rejection with client_routes_config.
Add comprehensive integration tests covering: - TCP proxy and NLB emulator infrastructure for simulating private link connectivity - query_routes filtering with different connection/host ID combinations - Full private-link connectivity verifying all driver connections go exclusively through the NLB proxy - Dynamic route updates via REST API with driver reconnection through new proxy ports
Recently scylladb started to rely on the options "--auth-superuser-name" and "--auth-superuser-salted-password" to ensure that a cassandra/cassandra user exists for tests - without those options a default superuser no longer exists.
…ames Skip the regex scanner and stack-based parser in parse_casstype_args() when the type string has no parentheses. For simple types like 'AsciiType' or 'org.apache.cassandra.db.marshal.FloatType', go directly to lookup_casstype_simple() which is just a prefix strip + dict lookup. This avoids re.Scanner, re.split on ':' / '=>', int() try/except, and list-of-lists stack manipulation for the common case of non-parameterized types. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
The time.sleep(10) in setup_keyspace() is redundant because callers already ensure the cluster is fully ready before calling it: - use_cluster() calls start_cluster_wait_for_up() which uses wait_for_binary_proto=True + wait_other_notice=True, then wait_for_node_socket() per node - External cluster path (wait=False) had no sleep anyway Remove the wait parameter entirely and its associated sleep, saving 10s per cluster startup.
Replace fixed sleeps with condition-based polling to speed up tests: - simulacron/utils.py: replace 5s sleep with HTTP endpoint polling (max 15s timeout, typically <1s) - test_authentication.py: replace 10s sleep with auth readiness poll that tries connecting with default credentials - upgrade/__init__.py: replace 10s auth sleep with same polling pattern - upgrade/test_upgrade.py: replace 3x 20s sleeps (60s total) with control connection readiness polling Total potential saving: ~95s of unconditional waiting per test run.
Replace fixed sleeps with condition-based polling in four test files: - test_shard_aware.py: replace 25s of sleeps (5+10+5+5) with wait_until_not_raised polling for reconnection after shard connection close and iptables blocking - test_metrics.py: replace 15s of sleeps (5+5+5) with polling for cluster recovery and node-down detection - test_tablets.py: replace 13s of sleeps (3+10) with polling for metadata refresh and decommission completion - simulacron/test_connection.py: replace 20s of sleeps (10+10) with polling for quiescent pool state Total potential saving: ~73s of unconditional waiting.
… for invalidation The tablet tests were intermittently failing because: 1. get_query_trace() used the default 2s max_wait, which is too short under resource pressure (--smp 2). Increased to 10s. 2. test_tablets_invalidation_decommission_non_cc_node used a fixed time.sleep(2) hoping tablet metadata invalidation would complete. Replaced with wait_until polling for the tablet record to be purged (0.5s delay, 20 attempts = 10s budget).
- test_cluster.py: replace sleep(1) x10 iterations with connect(wait_for_all_pools=True) for deterministic pool readiness - test_query.py: replace sleep(5) with wait_until polling for 'Preparing all known prepared statements' log message - test_connection.py: replace sleep(2) with wait_until polling for host_down listener notification
…superuser config Use set_configuration_options() (the Python API behind `ccm updateconf`) to set auth_superuser_name and auth_superuser_salted_password directly in the YAML config instead of passing them via the SCYLLA_EXT_OPTS environment variable.
…ema_agreement min(self._timeout, total_timeout - elapsed) raises TypeError when control_connection_timeout is set to None, which is explicitly documented as a supported value (meaning no timeout). Guard the min() call so that when self._timeout is None, we use only the remaining schema agreement wait time.
Patch reactor.running to False in setUp() so that maybe_start() always enters the branch that spawns the reactor thread. Without this, leaked global reactor state from prior tests can leave reactor.running as True, causing maybe_start() to skip thread creation and the reactor.run mock to never be called — making the assertion in test_connection_initialization fail intermittently. Observed in CI on PyPy 3.11 + macOS x86 (Rosetta 2), where timing differences make the reactor state leak more likely.
The column kind filter at line 2744 used 'clustering_key' but system_schema.columns uses 'clustering' as the kind value. This caused clustering columns to not be excluded from the 'other columns' loop, resulting in them being processed twice (once as clustering key, once as regular column). The correct value 'clustering' was already used 6 lines above in the clustering key extraction loop.
ScyllaDB doesn't support triggers, so skip the triggers query when connected to ScyllaDB. This is detected by checking if the connection has shard awareness (using the existing _is_not_scylla() method). Changes to both SchemaParserV3 and SchemaParserV4: - Modified _query_all() to conditionally append triggers query only for non-ScyllaDB - Modified _query_all() response unpacking to use array slicing for cleaner code - Modified get_table() in V3 to conditionally query triggers This eliminates unnecessary failed queries to system_schema.triggers on ScyllaDB. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
- Fix spelling: 'tring' → 'string' in docstring - Remove extra 't' at end of comment - Refactor complex list comprehension for clarity - Use 'is None' instead of '== None' for None comparison Co-authored-by: mykaul <4655593+mykaul@users.noreply.github.com>
Co-authored-by: mykaul <4655593+mykaul@users.noreply.github.com>
mykaul
force-pushed
the
perf/optimize-was-applied
branch
2 times, most recently
from
April 5, 2026 17:35
8a3b2ed to
f4ec874
Compare
The pool module emits a DEBUG log message when selecting a connection for a query. Emitting a log message for every query is too noisy. Since Python logging lacks a TRACE level, just remove the log.
Implement the SCYLLA_USE_METADATA_ID protocol extension, which backports the CQL v5 prepared-statement metadata-id mechanism to earlier protocol versions. When negotiated, the server includes a hash of the result metadata in the PREPARE response; the driver sends it back with every EXECUTE, allowing the server to omit result metadata from responses (skip_meta) and to report schema changes with METADATA_CHANGED plus fresh metadata, which the driver adopts automatically. protocol_features.py: parse the extension from SUPPORTED, echo it in STARTUP, expose it as ProtocolFeatures.use_metadata_id. protocol.py: ExecuteMessage carries connection-independent request data (skip_meta, result_metadata_id) fixed at construction; serialization decides the wire format from the (protocol_version, protocol_features) that Connection.send_msg supplies for the serving connection: - The metadata-id field is written iff the connection speaks CQL v5+ or negotiated the extension - always, on such connections. An empty sentinel (b'') is written when the statement has no id (prepared before the extension was active, e.g. during a rolling upgrade, or an LWT statement): the sentinel mismatch makes the server respond with METADATA_CHANGED plus the current id and metadata, so such statements acquire an id on their first execution. This also fixes a TypeError on v5 when result_metadata_id was None. - _SKIP_METADATA_FLAG is written only when the SCYLLA_USE_METADATA_ID extension is negotiated on the connection; without the metadata-id mechanism a schema change after PREPARE would leave the driver decoding rows with stale cached metadata. This is deliberately narrower than the metadata-id field above: on native CQL v5 the field is part of the frame layout, but the driver does not request skip there. Upstream never emitted _SKIP_METADATA_FLAG on any version (_write_query_params never wrote it), and enabling the skip optimization for native v5 is a separate change kept out of scope for this Scylla extension. Because messages are immutable after construction, every send path is correct without per-path setup - including the control-connection fallback - and concurrent sends of the same message (speculative executions) cannot race on per-connection state. query.py: PreparedStatement stores (result_metadata, result_metadata_id) as one tuple replaced in a single attribute assignment, read through compatibility properties and updated via update_result_metadata(). Response callbacks update statements while request threads read them; a torn pair (fresh id + stale metadata) would make the server skip sending metadata while rows are decoded against the wrong columns, with no recovery. The compatibility setters are documented as non-atomic relative to each other - update_result_metadata() is the atomic path; the setters exist only for callers assigning the old individual attributes. cluster.py: _create_response_future snapshots the pair once and requests skip_meta only when the statement has both an id and usable cached metadata (result_metadata is None for NO_METADATA/LWT statements and [] for zero-column statements; neither can nor needs to skip metadata). The same snapshot is handed to the ResponseFuture, so a skip_meta response is decoded against the metadata that pairs with the id the message sent - not a later re-read of the statement cache, which a concurrent METADATA_CHANGED could have replaced between construction and send (and which also keeps speculative sends of one message internally consistent). _set_result adopts a METADATA_CHANGED response by replacing the pair atomically; a response carrying a new id without column metadata is ignored with a warning, since adopting the id alone would create the unrecoverable stale-decode state. skip_meta additionally stays off for continuous paging (@dkropachev): Connection.process_msg hardcodes result_metadata=None for every page after the first, since it isn't threaded through the paging session - a skip_meta response has nothing to decode page 2+ against, and would crash on it. _execute_after_prepare refreshes the pair from exactly what the reprepare response carries, including the id (@dkropachev): falling back to the previously cached id when the response has none risks pairing it with metadata from a different schema version than the one that id was computed for - e.g. if the schema changed and then reverted between the two PREPAREs, the old id can become valid again for the current schema while paired locally with an intermediate version's metadata, with no server-side mismatch to catch it. Dropping it instead lets the next id-aware execute re-acquire a correctly paired id through the same b'' sentinel self-healing path a never-prepared statement uses. docs/scylla-specific.rst: documents the extension and its behaviour, worded so the skip_meta optimization reads as conditional on the extension being negotiated rather than pre-existing default behaviour. CHANGELOG.rst: add a Features entry for the extension.
Unit tests for the extension across its layers: test_protocol_features.py: SCYLLA_USE_METADATA_ID parsed from SUPPORTED and echoed in STARTUP options; absent by default. test_protocol.py (wire format): - metadata-id field written on v4 iff the connection negotiated the extension, with the exact bytes asserted; empty sentinel (b'') when the statement has no id, on both the extension path (v4) and the v5 native path (previously a TypeError); - _SKIP_METADATA_FLAG written when skip_meta is requested and the SCYLLA_USE_METADATA_ID extension is negotiated (v4 or v5), and NOT set on a native v5 connection without the extension (the id field is still written there, but the driver does not request skip); also suppressed - together with the id field - on a v4 connection without the extension, even when the statement carries an id; - PREPARED response decoding reads result_metadata_id iff the extension was negotiated (or v5); METADATA_CHANGED/NO_METADATA flag handling. test_query.py: PreparedStatement stores the (result_metadata, result_metadata_id) pair atomically - constructor, update_result_metadata, and the backwards-compatible single-attribute setters all replace the pair as one unit, and previously-taken snapshots stay internally consistent. test_response_future.py: - _create_response_future builds ExecuteMessage from a single pair snapshot: skip_meta only with both an id and usable cached metadata; disabled for id-less statements, NO_METADATA/LWT statements (result_metadata None) and zero-column statements (result_metadata []), while the id still rides on the message; - _query sends the message exactly as constructed (no per-connection mutation - regression test for the speculative-execution race) and decodes a skip_meta response against the metadata snapshotted when the message was built, not a later read of the statement cache (regression for a concurrent METADATA_CHANGED racing the send); - _set_result METADATA_CHANGED path replaces the cached pair atomically; a response with a new id but no column metadata (empty or absent) is ignored with a warning, leaving the cached pair unchanged - adopting the id alone would poison the cache with a stale-metadata/current-id pair the server would never refresh; - _execute_after_prepare refreshes the pair from exactly what the reprepare response carries, including the id, and no longer keeps the previous id when the response has none (@dkropachev: doing so risked pairing a stale id with metadata from a different schema version - test_execute_after_prepare_no_metadata_id_in_response_clears_id); - a statement with valid cached metadata+id must still get skip_meta=False when continuous_paging_options is set (@dkropachev: Connection.process_msg hardcodes result_metadata=None for paging-session pages after the first, so a skip_meta response would crash decoding them - test_create_execute_message_continuous_paging_disables_skip_meta). tests/integration/standard/test_scylla_metadata_id.py: live-server coverage against a real Scylla node via CCM, closing the one gap unit tests can't - whether Scylla actually treats the empty result_metadata_id sentinel as a mismatch rather than a protocol error. Confirms extension negotiation, the normal METADATA_CHANGED-after-ALTER-TABLE path, and the sentinel round trip: a statement forced back to result_metadata_id=None (simulating one prepared before the extension was known, e.g. mid rolling-upgrade) executes without error and comes back with a fresh id. Mirrors the equivalent live test already merged in the Java driver (scylladb/java-driver#758, should_handle_empty_metadata_id_when_executing_statement_when_supported). Run locally against Scylla 2026.1.9 via CCM; see PR description for setup and log excerpt.
mykaul
force-pushed
the
perf/optimize-was-applied
branch
from
July 29, 2026 20:34
f4ec874 to
c7fa63a
Compare
Updates docs theme to 1.9.3.
mykaul
force-pushed
the
perf/optimize-was-applied
branch
from
July 30, 2026 21:12
c7fa63a to
72aeb59
Compare
This was kept this way to preserve legacy behavior, but I think changing the behavior will be less of a problem than what the current behavior causes. The policy is used for reconnections (for example, reconnecting control connection). If reconnect policy finishes generation (it will do so after 64 attempts before my change), then the reconnector finish and the driver won't attempt reconnection anymore. This would be a terrible situation.
This better conveys what this is: not a timeut duration from config, but how much of this timeout is left right now.
The timeout argument in `wait` tells how much we need to wait taking into consideration that we already waited for some other futures. The total wait time that this future had available to complete is different: it includes time we spent waiting for other futures. This created confusing hearbeat messages, that could even show negative wait times. I fixed it by putting both timeouts in the error message. The `timeout` parameter of `OperationTimedOut` I changed to the original timeout because I think it is more useful and relevant here.
TcpProxy.stop()/drop_connections() used to close a connection's sockets directly, from the manager thread, while that connection's own forwarder thread could still be blocked in select()/recv() on those exact file descriptors -- a classic close-under-concurrent-user race. Since fds are process-global, closing them could let the OS silently recycle the fd number into a brand new connection before the stale forwarder thread's blocked call unwound, causing it to read/write/close a socket that no longer belonged to it (observed here as unhandled "ValueError: file descriptor cannot be a negative integer (-1)" crashes in _forward_loop once a socket was closed out from under it). stop() also only ever joined the accept-loop thread, never the per-connection forwarder threads it had just closed sockets out from under. Fix, scoped entirely to this test helper (no driver code touched): - TcpProxy._connections now maps (client_sock, target_sock) -> the forwarder thread serving that pair. - stop()/drop_connections() now shut down (SHUT_RDWR) both sockets -- safe to do concurrently with a blocked select()/recv(), unlike close() -- and then join() every forwarder thread before returning. Only the forwarder thread itself ever closes its own sockets now, and only after it has fully stopped using them. - _handle_new_connection starts the forwarder thread before publishing it into _connections, so a concurrent stop()/drop_connections() can never observe (and try to join) a thread that hasn't started yet. - NLBEmulator.add_node()/remove_node() now serialize against each other via an RLock, so a new proxy/connection can never be created while another thread's remove_node() is still tearing one down. - NLBEmulator._live_addresses() (read by rr_handler(), the discovery port's round-robin accept handler, from the discovery TcpProxy's own accept-loop thread) now also snapshots self._node_proxies under the same _lock that add_node()/remove_node() mutate it under. Previously it iterated the dict unlocked, so a concurrent add_node()/remove_node() could raise "RuntimeError: dictionary changed size during iteration" on that thread. A follow-up pass over the same synchronization path (Copilot automated review on the PR, flagged low-confidence so not posted as formal review threads, but both genuine) found one more real gap and a missing test: - _shutdown_and_join_connections() joined every forwarder thread with a 5s timeout, then unconditionally popped *every* connection out of self._connections regardless of whether its thread had actually exited. A thread that didn't finish in time was dropped from tracking anyway, so active_connections under-reported live connections, and a later stop()/drop_connections() could never retry shutting it down -- permanently leaking that thread and its fds. Fixed by only popping entries whose thread is confirmed dead (`not thread.is_alive()`) after the join; still-alive entries stay tracked until _forward_loop's own self-removal (already lock-protected and idempotent) reaps them, so a subsequent shutdown call can retry. - Added tests/unit/test_tcp_proxy.py, a checked-in deterministic regression test (TcpProxy has no CCM/cluster dependency, only sockets, so it runs as a plain fast unit test against a local dummy TCP echo backend). It covers: (a) the exact regression above -- neutering _shutdown_pair and shrinking one thread's join wait to deterministically force the "still alive after the timeout" path, and asserting the connection stays tracked until a retried drop_connections() actually reaps it -- and (b) a concurrent stress test that hammers drop_connections() from multiple threads while other threads continuously open/close real connections, asserting no unhandled exceptions and no forwarder threads left alive once stop() returns. Validation: - Standalone stress harness (no CCM needed) driving concurrent clients through TcpProxy while repeatedly calling drop_connections()/stop()+restart from another thread: pre-fix, 40 iterations produced 162 unhandled ValueError crashes; post-fix, 40 iterations (same parameters) and a follow-up 150-iteration run produced zero corruptions/exceptions/leaked threads. - Standalone stress harness driving concurrent readers directly exercising _live_addresses() against concurrent add_node()/ remove_node() churn: pre-fix, 313 "dictionary changed size during iteration" RuntimeErrors over 447k calls in 5s; post-fix, zero errors over 1.9M+ calls across three separate runs. - Full end-to-end runs of TestFullNodeReplacementThroughNlb::test_should_survive_full_node_replacement_through_nlb against a real CCM cluster, both before and after the fix, to check for behavioral regressions and reproduce the reported flakiness. - New tests/unit/test_tcp_proxy.py: 30/30 clean runs with the active_connections fix in place; with the fix reverted, the targeted regression test failed deterministically 15/15 runs (active_connections incorrectly reported 0 instead of 1 for a still-alive forwarder thread), confirming the test actually catches the bug it targets. - Full tests/unit/ suite: 722 passed, 88 skipped, 0 failed. Fixes scylladb#948. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tests/unit/test_tcp_proxy.py, added in d99dc46, imported its subject (TcpProxy) from tests/integration/standard/test_client_routes.py, which transitively imports tests/integration/__init__.py. That module guards its ccmlib imports with try/except ImportError, but then unconditionally declares `class Cassandra41CCMCluster(CCMCluster)` at module level, so on any environment without ccmlib installed the import fails with: NameError: name 'CCMCluster' is not defined This broke test collection consistently on the windows-2022 job, where ccmlib is absent. The latent defect in tests/integration/__init__.py predates d99dc46; that commit merely became the first unit test to import tests.integration and thus the first to expose it. TcpProxy is a plain socket-based helper -- it depends only on socket, select and threading, and needs neither CCM nor a running Cassandra/Scylla cluster -- so it does not belong behind that import. Move it verbatim into a new tests/tcp_proxy.py and import it from both call sites: - tests/integration/standard/test_client_routes.py now imports TcpProxy from tests.tcp_proxy; its `select` and `socket` imports, used only by the moved class, are dropped. - tests/unit/test_tcp_proxy.py imports from tests.tcp_proxy and no longer needs its os.environ.setdefault("CASSANDRA_VERSION", ...) shim, which existed solely to get tests.integration's module-level version parsing to succeed. The shim and the docstring paragraph explaining it are removed. The class body is byte-identical to the original; only the new module's license header, docstring and imports are new. No driver code is touched and no test behavior changes. Validation: - pytest tests/unit/test_tcp_proxy.py: 2 passed with neither CASSANDRA_VERSION nor SCYLLA_VERSION set, i.e. the unit test no longer imports tests.integration at all. - tests/integration/standard/test_client_routes.py compiles clean with no imports left unused. Fixes: scylladb#965
The strings didn't use the characters they intended because the backslashes effectively resulted in special characters. We fix them by marking the strings as raw.
The operator `is not` comapres the memory addresses of two objects. Since we're comparing an expression against a literal, it made no sense and was reported by Python. Fix it by moving on to using the operator `!=`.
patch by Brad Schoening; reviewed by Brad Schoening and Bret McGuire reference: apache#1263
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 trailing tablet_version_block byte to the EXECUTE message body. The server reads exactly one such byte per EXECUTE on a connection that negotiated the extension, so the encoder writes one whenever the connection did -- coalescing an unset value to 0 -- and none otherwise. Later commits fill in the value from the cached tablet version. Deciding this from the connection's negotiated features rather than from the message is what lets one ExecuteMessage be sent, unmodified, on connections that negotiated differently.
Store the server-provided 64-bit tablet_version on each cached Tablet and add helpers to encode it into the one-byte tablet_version_block exchanged on the wire. The version stays None until learned: on a cold start, and on a TABLETS_ROUTING_V1 connection, which never reports one. * 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.
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 when the table is known but its tablet or version is not (a cold cache, a vnode table), which the server answers with fresh routing info. The block is 0 when the driver cannot resolve the request to a tablet at all: a non-token-aware request, which the server never version-checks, and one whose keyspace or table is unknown, where a payload could not be cached anyway and generating a random byte would be wasted work. * The routing key and its ring token are resolved once per request, in _create_response_future, and handed to both consumers that need them while sending: the tablet_version_block here and shard selection in HostConnection. This keeps cluster-dependent state off the statement, which a caller may share between concurrent requests. The cached tablet is likewise looked up once -- the cache is mutable, so a second lookup could disagree with the first. * Hashing the routing key is guarded by can_support_partitioner(). On a Murmur3 cluster whose murmur3 helper is unavailable, from_key() raises NoMurmur3, and the default load balancing policy drops token awareness entirely; this path runs regardless of the policy in use, so without the check it would raise on every prepared-statement execution. With no token the pool skips shard selection instead of retrying the hash. * 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 tablet is cached under the effective keyspace -- the statement's, else the session's -- so a prepared statement executed in a session keyspace lands under the same key the send path looks it up by. * HostConnection.tablets_routing_v1 becomes supports_tablet_routing: shard selection is identical under both versions, since the request goes to this host either way and the pool picks the shard this host owns for the tablet. Refs: SCYLLADB-288 Refs: SCYLLADB-291
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). The last of those needs a connection that negotiated both extensions, which the driver never does on its own, so the test patches ProtocolFeatures.add_startup_options. The patch delegates to the real implementation and only adds v1 on top. Enumerating the options itself would silently stop requesting any extension added later while ProtocolFeatures still reported it as negotiated -- that is parsed from SUPPORTED, not from what STARTUP asked for -- and an extension that changes the frame layout, such as SCYLLA_USE_METADATA_ID, would then desynchronize every request on the connection.
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._consistency_mode, derived from the per-keyspace `consistency` option in system_schema.scylla_keyspaces. It is a _ConsistencyMode enum -- EVENTUAL, LOCAL or GLOBAL -- so the mode the server reported is kept verbatim instead of being flattened into a boolean at parse time. A full refresh reads the option as part of _query_all's batch, so it costs no round trip of its own, and a single-keyspace refresh reads only that keyspace's row. Both degrade to EVENTUAL on a connection that did not negotiate TABLETS_ROUTING_V2 -- which covers non-Scylla clusters, since only Scylla advertises it -- and on Scylla versions that lack the table or column. A transient failure reading the table propagates instead, aborting the refresh so the modes already known are retried, rather than resetting every keyspace to eventual and so silently disabling leader routing and evicting the tablet cache. Scylla only implements `global` so far, so a keyspace's tablets have a Raft leader exactly when its mode is GLOBAL; `local` is reserved for a mode that does not exist yet and behaves like `eventual` everywhere. Callers that care compare against _ConsistencyMode.GLOBAL directly, so implementing `local` later only widens those comparisons and leaves the parser and the metadata untouched. A change of mode also invalidates the keyspace's cached tablets, the same way a replication-strategy change does: a tablet cached while the keyspace was eventually consistent carries no leader ordering and must not survive into a strongly-consistent keyspace, where it would be misread as a leader hint. The mode is also emitted by KeyspaceMetadata.as_cql_query, so a schema dump of a strongly-consistent keyspace recreates it as one instead of silently downgrading it to eventual consistency. Both names are underscore-prefixed to keep them private: they are not yet stable and we do not want to commit to a public API for them.
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. Tablet.leader names that ordering in one place, and reports None for a tablet with no replicas so callers do not each have to guard the lookup. The leader is yielded first only when the keyspace's consistency mode is GLOBAL -- the only mode Scylla implements, and so the only one whose tablets have a leader -- and when 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. Requests at consistency level ONE or LOCAL_ONE are left alone. Any single replica satisfies them, so preferring the leader would only concentrate load on it without buying any consistency. The level is read from the statement, so a request that inherits it from an execution profile looks unset here and is routed to the leader anyway; that costs a little leader contention and nothing in correctness, and is tracked separately in scylladb#953. The hint stays bounded by the wrapped policy. Among the hosts that policy is willing to use the leader outranks distance -- a REMOTE leader is yielded before a LOCAL_RACK replica, since every write and linearizable read has to reach the leader anyway and a globally-consistent table gains no consistency from staying in one datacenter. It never overrides the policy's own filter, though: a leader the child policy reports as IGNORED is not contacted, so under the default DCAwareRoundRobinPolicy, which ignores remote hosts, the request goes to a local replica and the server forwards it, exactly as it would without v2. Leader preference can be turned off per policy instance with the private _prefer_tablet_leader option, leaving strongly-consistent tables with plain token-aware ordering. It is private and defaults to on while strong consistency is experimental. 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 reads each keyspace's _consistency_mode from system_schema.scylla_keyspaces (statically, and as keyspaces are created and dropped), and that TokenAwarePolicy sends a leader-requiring 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 bounded by the load-balancing policy, and eventually-consistent tables keep their usual token-aware ordering. Include a table of how ScyllaDB serves each operation on such a table, so the routing distinction has a visible reason: ONE and LOCAL_ONE reads are non-linearizable and take no Raft read barrier, so they keep normal token-aware ordering, while QUORUM and LOCAL_QUORUM reads and writes go through the leader and are routed to it. Spell out how far the preference reaches, since "bounded by the policy" alone is ambiguous: among the hosts the wrapped policy is willing to use the leader outranks distance, but a leader that policy ignores is never contacted, so a datacenter-aware policy with no remote hosts keeps the request local and lets the server forward it. Document the private _prefer_tablet_leader option that turns the preference off, and note that it is unstable while strong consistency is experimental.
mykaul
force-pushed
the
perf/optimize-was-applied
branch
from
August 15, 2026 08:13
72aeb59 to
5f0b6bf
Compare
Add a fast path in ResultSet.was_applied that skips batch detection (isinstance checks + regex match) when the query has a known LWT status from the server PREPARE response. For BoundStatement queries where is_lwt() returns True, the batch_regex match on the query string is entirely avoided. This benefits the most common LWT use case: prepared INSERT/UPDATE IF statements executed via BoundStatement, where the driver already knows from the PREPARE response whether the statement is an LWT. The slow path (isinstance + regex) is preserved for: - BatchStatement queries (detected via isinstance) - SimpleStatement batch queries (detected via regex) - Any query where is_lwt() returns False The fast-path condition checks `isinstance(query, BatchStatement)` before looking up `is_lwt`, and uses a getattr/callable guard around the call instead of calling `query.is_lwt()` unconditionally. This protects `was_applied` from raising AttributeError for any query object that doesn't implement is_lwt() -- e.g. response_future.query left as None, which is a real, reachable value (see ResponseFuture.query's class-level default and Session.prepare()/prepare_on_all_hosts, which construct ResponseFuture(..., query=None, ...) explicitly) -- falling back to the slow path instead. Also adds explicit tests for the fast path, non-LWT fallback, BatchStatement handling, and a regression test for a query without is_lwt() in was_applied. Part of: scylladb#751 Signed-off-by: Yaniv Michael Kaul <yaniv.kaul@scylladb.com>
Construct a minimal ResultSet with a mocked response_future and real cassandra.query statement objects, and time actual accesses to rs.was_applied, instead of re-implementing a simplified stand-in for its fast-path/slow-path branching. This also means the slow path exercises the real ResultSet.batch_regex instead of a different, looser regex, so the reported cost reflects the real regex match. On this machine: ~0.21us/call for the fast path (known-LWT BoundStatement) vs ~0.35us/call for the slow path (SimpleStatement regex match), a ~1.7x speedup -- both call costs are far below a microsecond once measured against the real was_applied property instead of Mock-heavy stand-ins. Signed-off-by: Yaniv Michael Kaul <yaniv.kaul@scylladb.com>
mykaul
force-pushed
the
perf/optimize-was-applied
branch
from
August 15, 2026 09:03
5f0b6bf to
154c663
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ResultSet.was_appliedthat skips batch detection when the query has a known LWT status from the server PREPARE responseBoundStatementqueries whereis_lwt()returnsTrue, thebatch_regexmatch andisinstancechecks are entirely avoidedBatchStatement,SimpleStatementbatches, and non-LWT queriesMotivation
ResultSet.was_appliedpreviously always ran batch detection on every call: twoisinstancechecks plus a regex match against the query string. For the most common LWT case (prepared INSERT/UPDATE IF viaBoundStatement), the driver already knows from the server PREPARE response whether it's an LWT statement. Leveragingis_lwt()lets us skip the batch detection entirely in this fast path.This is part of the LWT prepared statement performance improvement effort documented in scylladb#751 (optimization B4).
Changes
cassandra/cluster.py-ResultSet.was_applied:self.response_future.queryin a local variable (avoids repeated attribute lookups)query.is_lwt()isTrueand it's not aBatchStatement, skip batch detection and go directly to single-row validation + result extractionBatchStatement,SimpleStatement, and non-LWT queriestests/unit/test_resultset.py:test_was_applied_lwt_fast_path— tests the fast path with known LWT queries (all row factories, applied/not-applied, too-many-rows error)test_was_applied_non_lwt_fallback— tests that non-LWTSimpleStatementcorrectly falls through to slow pathtest_was_applied_batch_statement— tests thatBatchStatementuses slow path with[applied]column validationTesting
All unit tests pass:
tests/unit/test_resultset.py— 17/17 passed (14 original + 3 new)tests/unit/test_query.py— 6/6 passedtests/unit/test_parameter_binding.py— 37/37 passed