*: add txn protocol version declaration - #1534
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
size/XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe protobuf contracts add transaction protocol declarations, structured incompatibility errors, store compatibility ranges, request-origin values, and shared-lock compatibility requirements. ChangesTransaction protocol compatibility
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Client
participant Context
participant Store
participant Error
Client->>Context: Set txn_protocol_version
Context->>Store: Send transaction request
Store->>Error: Return IncompatibleRequest for unsupported version
Error-->>Client: Provide compatible version range
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The transaction compatibility wire contracts and generated artifacts are consistent, with no actionable merge risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The proto summaries support the core ✨ Finishing Touches🧪 Generate unit tests (beta)
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. A rabbit checks the version gate, Comment |
Introduce a protocol version registry and the request declaration that lets a client tell the server which transaction protocol semantics it implements, so that requests from callers which cannot handle a feature can be rejected consistently instead of being served with best effort. - kvrpcpb: add the TxnProtocolVersion enum and Context.txn_protocol_version (tag 39). Version numbers accumulate monotonically: 0 is legacy, 1 means the client handles the structured incompatibility error, and 2 covers the shared lock protocol. A missing field and an explicit 0 both mean legacy. Also extend RequestOrigin with TiCDC, BR and TiFlash and document that the origin is client-provided auditing metadata, not an authorization decision. - errorpb: add IncompatibleRequest (reason, message, provided/min/max compatible version) and Error.incompatible_request (tag 23). The bounds are inclusive and the two error fields stay independent message fields. A rejection of a request that declared no version also sets ServerIsBusy.reason to the fixed "txn_protocol_incompatible" fallback for legacy clients; a declared non-zero version gets no fallback. - metapb: add TxnProtocolVersionRange and Store.txn_protocol_version_range (tag 14). The message field keeps its presence so that an absent range stays distinguishable from an explicit [0, 0]. Signed-off-by: zyguan <zhongyangguan@gmail.com>
9865138 to
a933797
Compare
| enum IncompatibleRequestReason { | ||
| IncompatibleRequestReasonUnknown = 0; | ||
| // The declared version is outside the compatible range returned in the error. | ||
| IncompatibleRequestReasonTxnProtocolVersionOutOfRange = 1; |
There was a problem hiding this comment.
Better to rename IncompatibleRequestReasonUnsupportedTxnProtocolVersion ?
There was a problem hiding this comment.
I slightly prefer TxnProtocolVersionOutOfRange here.
UnsupportedTxnProtocolVersion sounds like the protocol version itself is statically unsupported by the server, while compatibility here is contextual. The compatible range can be derived from the store's admission range, or narrowed by a feature-specific check. As a result, requests declaring the same version may be accepted in one case but rejected with [min, max] in another.
OutOfRange describes the actual rejection condition more precisely: the provided version is outside the compatible range returned with this particular error, without implying that the version is unsupported in general.
It also seems consistent with the current wire contract, which explicitly models compatibility as an inclusive [min, max] range.
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, hbisheng, lcwangchao, overvenus The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
Issue Number: close #1535
Introduce a protocol version registry and the request declaration that lets a client tell the server which transaction protocol semantics it implements, so that requests from callers which cannot handle a feature can be rejected consistently instead of being served with best effort.
Summary by CodeRabbit
Summary by CodeRabbit