DRIVER-153: negotiate and implement SCYLLA_USE_METADATA_ID extension#770
DRIVER-153: negotiate and implement SCYLLA_USE_METADATA_ID extension#770nikagra wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Implements negotiation and support for Scylla’s SCYLLA_USE_METADATA_ID protocol extension to enable metadata-id based skip_meta behavior (backporting CQL v5 prepared-statement metadata-id semantics to earlier protocol versions).
Changes:
- Adds
SCYLLA_USE_METADATA_IDparsing fromSUPPORTEDand includes it inSTARTUPwhen negotiated. - Extends protocol encode/decode to read/write
result_metadata_idfor PREPARE/EXECUTE on pre-v5 when the extension is used, and fixes on-wire encoding of_SKIP_METADATA_FLAG. - Updates execution/result handling to conditionally use
skip_metaand to refresh cached prepared metadata when the server reports metadata changes.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cassandra/protocol_features.py |
Adds the SCYLLA_USE_METADATA_ID feature flag and includes it in negotiated STARTUP options. |
cassandra/protocol.py |
Writes _SKIP_METADATA_FLAG in query params; adds pre-v5 extension handling for result_metadata_id in PREPARE/EXECUTE. |
cassandra/cluster.py |
Adjusts when skip_meta is enabled and updates cached prepared metadata/id on METADATA_CHANGED responses. |
tests/unit/test_protocol_features.py |
Adds unit tests for feature parsing and STARTUP option inclusion. |
tests/unit/test_protocol.py |
Adds unit tests for skip-meta flag encoding and metadata-id handling in pre-v5 PREPARE/EXECUTE paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ade35d8 to
f42e225
Compare
|
I'm not sure where, but we should document this - with reference mainly to the scylladb docs about this feature. |
|
@mykaul Documentation I'm aware of is MetadataId extension in CQLv4 Requirement Document |
dkropachev
left a comment
dkropachev
left a comment
There was a problem hiding this comment.
One blocking correctness issue below: skip_meta is being enabled for prepared statements that can still have empty/absent cached result metadata.
6eea397 to
a86fd53
Compare
7ba5835 to
a86fd53
Compare
a86fd53 to
8880f03
Compare
170fd31 to
5fe1902
Compare
fcd3eba to
5fe1902
Compare
5fe1902 to
251b1a8
Compare
|
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:
📝 WalkthroughWalkthroughThis PR adds SCYLLA_USE_METADATA_ID negotiation, protocol metadata-id serialization and parsing, atomic prepared-statement metadata caching, and cache refresh handling for changed schemas. Execute messages conditionally send Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponseFuture
participant Server
participant PreparedStatement
Client->>ResponseFuture: execute prepared statement
ResponseFuture->>PreparedStatement: snapshot metadata and id
ResponseFuture->>Server: send ExecuteMessage
Server-->>ResponseFuture: return result metadata id and optional columns
ResponseFuture->>PreparedStatement: update cached metadata pair
ResponseFuture-->>Client: return rows
Possibly related PRs
Suggested labels: 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 |
|
Actionable comments posted: 0 |
de8d3fc to
bfc9760
Compare
|
🤖: All review issues (1–9) have been addressed and the branch has been squashed to the required 2-commit shape. Requesting re-review from @Lorak-mmk and @dkropachev. What changed since last review: Production commit (
Test commit (
CI on the 8-commit pre-squash branch: 18/19 passed; |
bfc9760 to
d3300e2
Compare
| paging_state=None, timestamp=None, skip_meta=False, | ||
| continuous_paging_options=None, result_metadata_id=None): | ||
| continuous_paging_options=None, result_metadata_id=None, | ||
| use_metadata_id=False): |
There was a problem hiding this comment.
Here ExecuteMessage got a new parameter, use_metadata_id, in addition to existing result_metadata_id
| elif isinstance(query, BoundStatement): | ||
| prepared_statement = query.prepared_statement | ||
| message = ExecuteMessage( | ||
| prepared_statement.query_id, query.values, cl, | ||
| serial_cl, fetch_size, paging_state, timestamp, | ||
| skip_meta=bool(prepared_statement.result_metadata), | ||
| continuous_paging_options=continuous_paging_options, | ||
| result_metadata_id=prepared_statement.result_metadata_id) | ||
| continuous_paging_options=continuous_paging_options) | ||
| elif isinstance(query, BatchStatement): |
There was a problem hiding this comment.
But here you remove result_metadata_id and skip_meta from this call to ExecuteMessage. Why?
| if self.prepared_statement and isinstance(message, ExecuteMessage): | ||
| has_result_metadata_id = self.prepared_statement.result_metadata_id is not None | ||
| has_result_metadata = bool(self.prepared_statement.result_metadata) | ||
| can_skip_meta = has_result_metadata_id and has_result_metadata and ( | ||
| ProtocolVersion.uses_prepared_metadata(connection.protocol_version) | ||
| or connection.features.use_metadata_id | ||
| ) | ||
| message.skip_meta = can_skip_meta | ||
| message.result_metadata_id = self.prepared_statement.result_metadata_id if can_skip_meta else None | ||
| message.use_metadata_id = connection.features.use_metadata_id | ||
|
|
There was a problem hiding this comment.
Ah, so instead of setting those when creating ExecuteMessage you started to set them here.
This seems a bit inelegant. Why did you decide on this approach? Can't we still pass those params to the constructor? And if we are going with your approach, why do we need those params in the constructor?
| def send_body(self, f, protocol_version): | ||
| write_string(f, self.query_id) | ||
| if ProtocolVersion.uses_prepared_metadata(protocol_version): | ||
| write_string(f, self.result_metadata_id) | ||
| if ProtocolVersion.uses_prepared_metadata(protocol_version) or self.use_metadata_id: | ||
| write_string(f, self.result_metadata_id if self.result_metadata_id is not None else b'') | ||
| self._write_query_params(f, protocol_version) |
There was a problem hiding this comment.
A more elegant approach, it seems to me, is to not have use_metadata_id in ExecuteMessage. Instead, pass ProtocolFeatures to this method, use it and version to decide wheter to send an id, and use it from ExecuteMessage
| new_result_metadata_id = getattr(response, 'result_metadata_id', None) | ||
| if self.prepared_statement and new_result_metadata_id is not None: | ||
| if response.column_metadata: | ||
| # Write result_metadata before result_metadata_id intentionally: | ||
| # a concurrent reader that still sees the old metadata_id will | ||
| # ask the server for full metadata and recover safely; a reader | ||
| # that sees the new metadata_id together with the new metadata | ||
| # is immediately correct. The opposite write order could expose | ||
| # a window where a reader uses a new metadata_id with stale metadata. | ||
| # Note: correctness of this ordering relies on CPython's GIL making | ||
| # individual attribute reads/writes effectively atomic. Other Python | ||
| # implementations (PyPy, Jython, etc.) may not provide this guarantee. | ||
| self.prepared_statement.result_metadata = response.column_metadata | ||
| else: | ||
| log.warning( | ||
| "Server sent a new result_metadata_id but no column metadata " | ||
| "for prepared statement %r. The cached column metadata will not " | ||
| "be updated; only result_metadata_id is refreshed.", | ||
| getattr(self.prepared_statement, 'query_id', None) | ||
| ) | ||
| self.prepared_statement.result_metadata_id = new_result_metadata_id |
There was a problem hiding this comment.
What is the concurrency model here? I don't think this code works if its possible for it to execute concurrently.
Consider the following:
- We start with schema X and perform two schema changes, to version Y and then Z.
- We issue two requests
- First request receives
Y_idandY, setsresult_metadatatoY, and then execution switches to second request - Second request receives
Z_idandZ, setsresult_metadatatoZ, thenresult_metadata_idtoZ_id. - Execution switches back to first request, it sets
result_metadata_idtoY_id.
Now we have result_metadata = Z, and result_metadata_id - Y_id. Any request executed won't receive new metadata (because id is the newest) but will try to deserialize results using older metadata.
| - Statements prepared before the extension was negotiated (e.g., during a rolling | ||
| upgrade) retain ``result_metadata_id=None`` and fall back to always requesting | ||
| full metadata, which is the safest option. | ||
|
|
There was a problem hiding this comment.
Sure, but at some point we'll receive the metadata and id, and then it will be skipped.
I'm saying that because this point may suggest that clients need to be restarted after such rolling upgrade, but they don't
| directly from the data. | ||
|
|
||
| For full protocol details see the ScyllaDB CQL extensions documentation: | ||
| https://opensource.docs.scylladb.com/stable/cql/cql-extensions.html |
There was a problem hiding this comment.
What is this link? Opensource is LONG deprecated, and this link is about CQL language extension, not CQL protocol extensions - totally irrelevant here.
| if self.prepared_statement and isinstance(message, ExecuteMessage): | ||
| has_result_metadata_id = self.prepared_statement.result_metadata_id is not None | ||
| has_result_metadata = bool(self.prepared_statement.result_metadata) | ||
| can_skip_meta = has_result_metadata_id and has_result_metadata and ( | ||
| ProtocolVersion.uses_prepared_metadata(connection.protocol_version) |
There was a problem hiding this comment.
When is bool(self.prepared_statement.result_metadata) true?
Is it true if server sent result metadata with 0 columns?
dkropachev
left a comment
dkropachev
left a comment
There was a problem hiding this comment.
Two non-duplicate correctness issues found in the current diff. I did not add a new skip_meta cached-metadata comment because that issue is already covered in the existing review thread.
| self._connection = connection | ||
| result_meta = self.prepared_statement.result_metadata if self.prepared_statement else [] | ||
|
|
||
| if self.prepared_statement and isinstance(message, ExecuteMessage): |
There was a problem hiding this comment.
This per-connection setup only runs in _query(), but _query_control_connection() can also send the original prepared ExecuteMessage during control-connection fallback. In that path the message keeps the constructor defaults (use_metadata_id=False, skip_meta=False, result_metadata_id=None). If the control connection is v4 and negotiated SCYLLA_USE_METADATA_ID, ExecuteMessage.send_body() will omit the required metadata-id field, shifting the frame layout and causing a protocol error.
Please share this setup logic with _query_control_connection() before its send_msg() call so both send paths decide use_metadata_id, skip_meta, and result_metadata_id from the actual connection being used.
| or connection.features.use_metadata_id | ||
| ) | ||
| message.skip_meta = can_skip_meta | ||
| message.result_metadata_id = self.prepared_statement.result_metadata_id if can_skip_meta else None |
There was a problem hiding this comment.
This can pair a fresh metadata id with a stale metadata snapshot. _set_result() writes prepared_statement.result_metadata first and result_metadata_id second, but _query() snapshots result_meta before reading result_metadata_id. A concurrent execute can capture old metadata, then read the new id here, send skip_meta=True, and decode a no-metadata response using stale result_meta.
Read result_metadata_id first, then snapshot result_metadata, and use those two local values consistently for both can_skip_meta and send_msg(..., result_metadata=...). That matches the write-ordering comment in _set_result() and avoids pairing a fresh id with stale cached metadata.
|
This PR seems to be solving similar problems to what I've been facing while working on TABLETS_ROUTING_V2 (cf. #913). In my work, I also need to pass per-request information and encode it in the message body ( I approached at least one problem in a bit different way (which turns out to be what @Lorak-mmk suggested in his comment here: https://github.com/scylladb/python-driver/pull/770/changes#r3442046951). Unfortunately, I broke the public API this way (I had to extend the argument list for python-driver/cassandra/protocol.py Lines 1060 to 1069 in 8f772c8 Since we're working on the same code and solving similar problems, maybe it would be a good idea to discuss our solutions and agree on a unified approach. This would prevent doubling the effort and, what's even more important, introducing two different solutions to the same issue. I'm explaining this here for visibility, but let's move the conversation to Slack. |
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>
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 only written when the metadata-id mechanism is active on the connection; without it, a schema change after PREPARE would leave the driver decoding rows with stale cached metadata. (This also revives the flag itself, which was dead code upstream: _write_query_params never wrote it.) 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. 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). _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. _execute_after_prepare refreshes the pair the same way, keeping the previous id when the reprepare response carries none.
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 metadata-id mechanism is active (extension on v4, native v5), and 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); - _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 the reprepare response and keeps the previous id when the response carries none.
d3300e2 to
8aecb92
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_protocol.py (1)
473-489: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
ClassVarannotation for the read-only frame-hex lookup table.Ruff RUF012 flags
EXPECTED_FRAMESas a mutable class attribute. It's only read here, so risk is low, but annotating withtyping.ClassVar[dict](or moving it to module scope) documents the intent and silences the lint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_protocol.py` around lines 473 - 489, Annotate the read-only EXPECTED_FRAMES class attribute with typing.ClassVar[dict] so Ruff RUF012 recognizes it as intentional class-level state. Preserve the existing frame mappings and lookup behavior.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.
Inline comments:
In `@tests/unit/test_protocol.py`:
- Around line 260-282: Add a standalone test method definition before the
`_METADATA_ID_FLAG` ROWS-decoding test body, using the surrounding test class’s
naming and `self` conventions. Keep its existing docstring, buffer setup,
`recv_results_metadata` call, and assertions inside the new method so pytest
discovers it independently from
`test_recv_results_prepared_v5_reads_metadata_id`.
---
Nitpick comments:
In `@tests/unit/test_protocol.py`:
- Around line 473-489: Annotate the read-only EXPECTED_FRAMES class attribute
with typing.ClassVar[dict] so Ruff RUF012 recognizes it as intentional
class-level state. Preserve the existing frame mappings and lookup 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: edb6a82d-1220-476f-9fe6-b25c6d7b238e
📒 Files selected for processing (9)
cassandra/cluster.pycassandra/protocol.pycassandra/protocol_features.pycassandra/query.pydocs/scylla-specific.rsttests/unit/test_protocol.pytests/unit/test_protocol_features.pytests/unit/test_query.pytests/unit/test_response_future.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/test_protocol_features.py
- cassandra/protocol_features.py
- docs/scylla-specific.rst
|
|
||
|
|
||
| """ | ||
| When _METADATA_ID_FLAG (0x0008) is set in a ROWS result, | ||
| recv_results_metadata must read and store the new result_metadata_id | ||
| sent by the server (METADATA_CHANGED signal), and still populate | ||
| column_metadata normally. | ||
| """ | ||
| # Wire layout for a ROWS result with METADATA_CHANGED: | ||
| # flags: int(0x0008) = _METADATA_ID_FLAG | ||
| # colcount: int(0) | ||
| # result_metadata_id: short(4) + b'new1' | ||
| # (no columns — colcount=0 — to keep the buffer minimal) | ||
| buf = io.BytesIO( | ||
| struct.pack('>i', 0x0008) # flags: METADATA_ID_FLAG | ||
| + struct.pack('>i', 0) # colcount = 0 | ||
| + struct.pack('>H', 4) + b'new1' # result_metadata_id = b'new1' | ||
| ) | ||
| msg = ResultMessage(kind=RESULT_KIND_ROWS) | ||
| msg.recv_results_metadata(buf, user_type_map={}) | ||
| assert msg.result_metadata_id == b'new1' | ||
| assert msg.column_metadata == [] | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing def line silently merges this test into the previous method.
Lines 262-281 (the _METADATA_ID_FLAG ROWS-decoding test) have no preceding def test_...(self):. Since the docstring at line 262 sits at the same 8-space indentation as the body of test_recv_results_prepared_v5_reads_metadata_id (ending line 259), Python treats it as a continuation of that method's body rather than a syntax error. This test never runs as its own unit — its assertions execute (and reuse/reassign buf/msg) inside the unrelated prepared-statement test, so the intended standalone coverage for _METADATA_ID_FLAG on ROWS results doesn't actually exist as a discoverable pytest test.
🐛 Proposed fix: restore the missing method signature
assert msg.query_id == b'ab'
assert msg.result_metadata_id == b'xyz'
+ def test_recv_results_metadata_metadata_id_flag_reads_metadata_id(self):
"""
When _METADATA_ID_FLAG (0x0008) is set in a ROWS result,
recv_results_metadata must read and store the new result_metadata_id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| """ | |
| When _METADATA_ID_FLAG (0x0008) is set in a ROWS result, | |
| recv_results_metadata must read and store the new result_metadata_id | |
| sent by the server (METADATA_CHANGED signal), and still populate | |
| column_metadata normally. | |
| """ | |
| # Wire layout for a ROWS result with METADATA_CHANGED: | |
| # flags: int(0x0008) = _METADATA_ID_FLAG | |
| # colcount: int(0) | |
| # result_metadata_id: short(4) + b'new1' | |
| # (no columns — colcount=0 — to keep the buffer minimal) | |
| buf = io.BytesIO( | |
| struct.pack('>i', 0x0008) # flags: METADATA_ID_FLAG | |
| + struct.pack('>i', 0) # colcount = 0 | |
| + struct.pack('>H', 4) + b'new1' # result_metadata_id = b'new1' | |
| ) | |
| msg = ResultMessage(kind=RESULT_KIND_ROWS) | |
| msg.recv_results_metadata(buf, user_type_map={}) | |
| assert msg.result_metadata_id == b'new1' | |
| assert msg.column_metadata == [] | |
| def test_recv_results_metadata_metadata_id_flag_reads_metadata_id(self): | |
| """ | |
| When _METADATA_ID_FLAG (0x0008) is set in a ROWS result, | |
| recv_results_metadata must read and store the new result_metadata_id | |
| sent by the server (METADATA_CHANGED signal), and still populate | |
| column_metadata normally. | |
| """ | |
| # Wire layout for a ROWS result with METADATA_CHANGED: | |
| # flags: int(0x0008) = _METADATA_ID_FLAG | |
| # colcount: int(0) | |
| # result_metadata_id: short(4) + b'new1' | |
| # (no columns — colcount=0 — to keep the buffer minimal) | |
| buf = io.BytesIO( | |
| struct.pack('>i', 0x0008) # flags: METADATA_ID_FLAG | |
| struct.pack('>i', 0) # colcount = 0 | |
| struct.pack('>H', 4) + b'new1' # result_metadata_id = b'new1' | |
| ) | |
| msg = ResultMessage(kind=RESULT_KIND_ROWS) | |
| msg.recv_results_metadata(buf, user_type_map={}) | |
| assert msg.result_metadata_id == b'new1' | |
| assert msg.column_metadata == [] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_protocol.py` around lines 260 - 282, Add a standalone test
method definition before the `_METADATA_ID_FLAG` ROWS-decoding test body, using
the surrounding test class’s naming and `self` conventions. Keep its existing
docstring, buffer setup, `recv_results_metadata` call, and assertions inside the
new method so pytest discovers it independently from
`test_recv_results_prepared_v5_reads_metadata_id`.
Summary
Implements the
SCYLLA_USE_METADATA_IDScylla CQL protocol extension (DRIVER-153), which backports the prepared-statement metadata-ID mechanism from CQL v5 to earlier protocol versions.When the extension is negotiated:
skip_meta=True)METADATA_CHANGEDflag and includes the new metadata ID + new column metadata in the response — the driver picks this up and updates its cached metadata automaticallyRebased on #934 (merged groundwork that threads each connection's negotiated
ProtocolFeaturesinto message serialization) and reworked to resolve the review discussion below.Design change from the previous version of this PR
Previously,
ExecuteMessagecarried ause_metadata_idflag andResponseFuture._query()mutatedskip_meta/result_metadata_idon the shared message after borrowing a connection. Per @Lorak-mmk's review (#770 (comment) and the surrounding thread), that design is gone:ExecuteMessageis immutable once constructed —skip_metaandresult_metadata_idare set from the prepared statement's cached metadata at construction time (in_create_response_future), same as before this extension existed.send_bodydecides what actually reaches the wire from(protocol_version, protocol_features)— the connection's negotiated features, supplied by protocol: pass negotiated ProtocolFeatures to message serialization #934's plumbing. The metadata-id field is written whenever the serving connection speaks CQL v5+ or negotiated the extension — always, not conditionally — with an emptyb''sentinel when the statement has no id yet (mixed cluster / rolling upgrade)._SKIP_METADATA_FLAGis only set when that same condition holds, so a statement executed against a connection without the extension never asks the server to skip metadata it has no way to recover.This fixes two problems the old design had:
_query()mutated the one sharedExecuteMessageper connection borrowed; with speculative retries or a mixed-feature cluster, two connections could interleave mutation and encoding of the same message. Immutability removes the race entirely._query_control_connectionnever went through the mutation block and would omit the id field on an extension-enabled control connection. Every send path now serializes correctly with zero extra code, because the decision is made insend_bodyfrom the connection actually being used.Other review threads resolved
PreparedStatementnow stores(result_metadata, result_metadata_id)as one tuple replaced in a single attribute assignment (update_result_metadata()), withresult_metadata/result_metadata_idas compatibility properties over it. A reader can no longer observe the metadata of one schema version paired with the id of another — the previous per-field write ordering (and its GIL-dependent correctness comment) is gone._set_result), the old code adopted the id anyway. That manufactures the exact unrecoverable state the pair-atomicity fix is meant to prevent: the server would then match the id and stop sending metadata, while the driver decodes against stale cached columns forever. Now that response is logged as a warning and nothing is cached — the next EXECUTE resends the old id, the server detects the mismatch, and the driver recovers with full metadata.METADATA_CHANGEDresponse; no client restart needed.docs/dev/protocol-extensions.md, which actually documents this extension.bool(result_metadata)question (@Lorak-mmk DRIVER-153: negotiate and implement SCYLLA_USE_METADATA_ID extension #770 (comment)):result_metadataisNonefor statements withNO_METADATAin their PREPARE response (LWT/conditional statements) and[]for statements returning zero columns (plain INSERT/UPDATE/DELETE). Both are falsy, and both correctly keepskip_metaoff — there's nothing to decode against in either case. Documented in a code comment at the construction site.Changes
cassandra/protocol_features.pyUSE_METADATA_ID = "SCYLLA_USE_METADATA_ID"constant anduse_metadata_idfield toProtocolFeaturescassandra/protocol.py_SKIP_METADATA_FLAGis now actually written to the wire — it was stored on_QueryMessagebut never sent (effectively dead code upstream)recv_results_prepared: readresult_metadata_idfor the Scylla extension (pre-v5) in addition to standard CQL v5+ExecuteMessage.send_body: decides both the metadata-id field and the skip-metadata flag from(protocol_version, protocol_features)at serialization time; writes theb''sentinel when the statement has no idcassandra/query.pyPreparedStatement.result_metadata/result_metadata_idbecome properties over a single(result_metadata, result_metadata_id)tuple; addupdate_result_metadata()for atomic pair replacementcassandra/cluster.py_create_response_future: snapshot the metadata pair once, constructExecuteMessageimmutably withskip_meta/result_metadata_idset from that snapshot_set_result: onMETADATA_CHANGED, replace the cached pair atomically; ignore (with a warning) a response that carries a new id without column metadata_execute_after_prepare: same atomic update on reprepare, keeping the previous id when the response carries nonedocs/scylla-specific.rstTest plan
test_protocol_features.py,test_protocol.py,test_query.py,test_response_future.pycovering: feature negotiation, STARTUP options, wire-format assertions for the metadata-id field and skip-metadata flag (present/suppressed, sentinel forNone, v4/v5), PREPARE response decoding with/without the extension, atomic metadata-pair replacement (constructor,update_result_metadata, compatibility setters),_create_response_futureconstruction gating (id present/absent, LWT/NO_METADATA, zero-column), a regression proving_querysends the message unmutated (the old speculative-execution race),_set_resultMETADATA_CHANGED and the anomalous-response warning path (nothing cached), and_execute_after_preparereprepare handlingbuild_ext --inplace) compiles the reworked signatures; tests pass against the compiled modulesPre-review checklist
./docs/.Fixes:annotations to PR description.