protocol: pass negotiated ProtocolFeatures to message serialization#934
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughMessage serialization now receives negotiated Sequence Diagram(s)sequenceDiagram
participant Connection
participant ProtocolHandler
participant Message
Connection->>ProtocolHandler: encode_message(protocol_features=self.features)
ProtocolHandler->>Message: send_body(protocol_features)
Message-->>ProtocolHandler: serialized body
ProtocolHandler-->>Connection: encoded frame
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
5f108fa to
e0878f3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_protocol.py (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate import of
UnsupportedOperation.
UnsupportedOperationis imported twice: once fromcassandra(line 19) and once fromcassandra.protocol(line 21). Remove the redundant import from thecassandra.protocolblock to avoid redefinition and keep the list clean.♻️ Proposed fix
from cassandra import ConsistencyLevel, ProtocolVersion, UnsupportedOperation from cassandra.protocol import ( - PrepareMessage, QueryMessage, ExecuteMessage, UnsupportedOperation, + PrepareMessage, QueryMessage, ExecuteMessage, BatchMessage, StartupMessage, OptionsMessage, RegisterMessage,🤖 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 19 - 24, Remove UnsupportedOperation from the cassandra.protocol import list in tests/unit/test_protocol.py, retaining the existing import from cassandra and leaving all other protocol imports unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/test_protocol.py`:
- Around line 19-24: Remove UnsupportedOperation from the cassandra.protocol
import list in tests/unit/test_protocol.py, retaining the existing import from
cassandra and leaving all other protocol imports unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca399a84-9a7f-4d9f-9180-8de6748670e7
📒 Files selected for processing (7)
CHANGELOG.rstcassandra/connection.pycassandra/protocol.pycassandra/protocol_features.pydocs/api/cassandra/protocol.rsttests/unit/test_connection.pytests/unit/test_protocol.py
🚧 Files skipped from review as they are similar to previous changes (6)
- CHANGELOG.rst
- cassandra/connection.py
- tests/unit/test_connection.py
- docs/api/cassandra/protocol.rst
- cassandra/protocol_features.py
- cassandra/protocol.py
Make ProtocolFeatures.__init__ keyword-only and build it by keyword in parse_from_supported. Independently developed protocol extensions (SCYLLA_USE_METADATA_ID, TABLETS_ROUTING_V2) each add fields to this class; keyword construction lets them do so without conflicting over positional-argument order. All existing callers already used keywords.
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>
Note in the protocol API docs that both contracted _ProtocolHandler methods receive the connection's negotiated ProtocolFeatures, spelling out the calling conventions: decode_message receives it positionally, encode_message as the required protocol_features keyword argument, so overrides must keep that parameter name. Add a CHANGELOG entry with upgrade guidance for custom protocol handlers (accept protocol_features, prefer **kwargs for future-proofing, forward it when delegating to send_body).
e0878f3 to
5dc0e6b
Compare
4 participants
Summary
Groundwork extracted from the discussions on #770 (
SCYLLA_USE_METADATA_ID) and #913 (TABLETS_ROUTING_V2): both extensions need to know, at message-serialization time, which protocol features the serving connection negotiated, so that extension fields are emitted exactly on the connections that negotiated them. This PR threads that information through the encode path once, so both feature PRs (and any future extension) can build on it instead of each changing the contract separately.This implements the approach proposed by @Lorak-mmk in #770 (comment) and follows up on @dawmd's suggestion (#770 (comment)) to unify the mechanism between #770 and #913 — the plumbing mirrors what #913 already implements, hence the co-authorship.
Changes
Connection.send_msgpasses the connection's negotiatedProtocolFeaturesto the encoder — the single choke point used by every send path, including the control-connection fallback._ProtocolHandler.encode_messageaccepts a new requiredprotocol_featuresargument (passed by keyword fromsend_msg, per review feedback — noNonedefault) and forwards it to every message'ssend_body, which gains the same parameter. The encode side becomes symmetric withdecode_message, which already receivesprotocol_features.encode_messagedocstring): messages carry connection-independent request data;send_bodydecides the wire format from(protocol_version, protocol_features). This avoids per-send-site setup and mutating shared message objects per attempt.ProtocolFeatures.__init__becomes keyword-only, so independently developed extensions (DRIVER-153: negotiate and implement SCYLLA_USE_METADATA_ID extension #770 addsuse_metadata_id, Implement TABLETS_ROUTING_V2 #913 addstablets_routing_v2) can add fields without conflicting over positional-argument order.Zero wire change
This is pure plumbing: no message consumes the parameter yet, so no bytes on the wire change. A new byte-identity test suite pins frames for representative messages (STARTUP/OPTIONS/REGISTER/AUTH_RESPONSE/PREPARE/QUERY/EXECUTE/BATCH across protocol v3/v4/v5) to the exact bytes produced before this change — both with no features and with all-default features.
Breaking change for custom protocol handlers
_ProtocolHandler.encode_messageis a documented contract; custom handlers that overrideencode_messagemust accept the requiredprotocol_featureskeyword argument — the parameter name matters, sincesend_msgpasses it by keyword (adding**kwargsis recommended for future-proofing) — and custom encoders that delegate tomsg.send_bodyshould forward it. Handlers that only customize decoding — the advertised use case (LazyProtocolHandler,NumpyProtocolHandler, custom deserializers) — are unaffected.There is deliberately no compatibility fallback for old-style encoders, and the argument is required rather than defaulted: extensions are negotiated per connection at STARTUP, before the per-request handler is known. A silently grandfathered old-style encoder would omit fields a negotiated extension requires — a data-dependent protocol error in production — whereas the signature change fails fast with a deterministic
TypeError. Precedent:decode_messagealready gainedprotocol_featuresthe same way.Test plan
send_msghands the connection's features to the encoder;encode_messageforwards them intosend_body(plain and compressed paths) and raisesTypeErrorwhen the argument is omitted (assertion phrased to hold under both CPython's and Cython's error wording)build_ext --inplace) compiles the new signatures; protocol tests pass against the compiled modulesPre-review checklist
./docs/.Fixes:annotations to PR description.