feat: expose Put.match_seq on the gRPC wire path#56
feat: expose Put.match_seq on the gRPC wire path#56drmingdrmer merged 3 commits intodatabendlabs:mainfrom
Put.match_seq on the gRPC wire path#56Conversation
…action operations `Put` and `PutSequential` duplicated three fields (`value`, `expire_at_ms`, `ttl_ms`) for value-with-expiration; `Put`, `Delete`, and `FetchIncreaseU64` duplicated two fields (`key`, `match_seq`) for conditional key targeting. This extracts them into `operation::Payload` and `operation::KeyLookup`, reducing structural duplication and making the domain model explicit. `Put` now composes both sub-structs (`target: KeyLookup`, `payload: Payload`), which also enables `Put.match_seq` -- previously only `Delete` and `FetchIncreaseU64` supported conditional sequence guards. The applier `execute_put()` now checks `op.match_seq()` and applies `MatchSeq::Exact` when set. Changes: - Add `Payload` with `new()`, `just()`, `to_meta_spec()` and `KeyLookup` with `new()`, `just()` in `operation.rs` - Add delegate accessors `key()`, `match_seq()` on `Put`, `Delete`, `FetchIncreaseU64` - Extend `Operation::match_seq()` builder chain to include `Put` - Add 3 integration tests for Put `match_seq` (unconditional / mismatch / match)
xp-trumpet
left a comment
xp-trumpet
left a comment
There was a problem hiding this comment.
@xp-trumpet reviewed 15 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on drmingdrmer).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcf9127573
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Expose Put.match_seq end-to-end for kv_transaction by adding the proto field, wiring it through conversion and state-machine application, adding a feature flag/feature registration, and updating integration tests to hit the real gRPC path.
Changes:
- Add
TxnPutRequest.match_seqto the protobuf API and propagate it into the Rust-nativekv_transactionoperation model. - Apply
match_seqsemantics forPutin the raft-store applier by converting it intoMatchSeq::Exact. - Introduce feature flag plumbing (
txn-put-match-seq) and update integration tests to cover conditional put throughtransaction_v2().
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/server/service/tests/it/grpc/metasrv_grpc_kv_transaction.rs | Adds gRPC-path integration tests for Put.match_seq (none/mismatch/match). |
| crates/server/service/Cargo.toml | Enables txn-put-match-seq for meta-client in dev-deps so tests can emit the new field. |
| crates/server/raft-store/src/applier/mod.rs | Applies Put.match_seq by setting UpsertKV’s MatchSeq::Exact and refactors to new Payload/KeyLookup structs. |
| crates/common/version/src/spec.rs | Registers KvTransactionPutMatchSeq feature span for server/client compatibility tracking. |
| crates/common/version/src/feat.rs | Adds new Feature::KvTransactionPutMatchSeq identifier. |
| crates/common/types/src/proto_ext/txn_request_ext.rs | Updates proto-ext tests to include the new match_seq field in TxnPutRequest literals. |
| crates/common/types/src/proto_ext/txn_put_request_ext.rs | Initializes match_seq in constructor and includes it in Display. |
| crates/common/types/src/proto_ext/txn_op_ext.rs | Gates setting match_seq on Put behind txn-put-match-seq; adds feature-gated unit tests. |
| crates/common/types/src/kv_transaction/operation.rs | Refactors operation model to use Payload and KeyLookup, enabling Put.match_seq in the domain model. |
| crates/common/types/src/kv_transaction/mod.rs | Updates builders to construct new operation structs and extends Operation::match_seq() to include Put. |
| crates/common/types/src/kv_transaction/display.rs | Updates formatting to reflect Payload/KeyLookup and show match_seq. |
| crates/common/types/src/kv_transaction/convert.rs | Wires proto match_seq into KeyLookup::new() during protobuf → domain conversion. |
| crates/common/types/proto/request.proto | Adds optional uint64 match_seq = 6 to TxnPutRequest. |
| crates/common/types/Cargo.toml | Adds txn-put-match-seq feature flag to databend-meta-types. |
| crates/client/client/Cargo.toml | Propagates txn-put-match-seq feature to enable types’ gate from the client crate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Write a value to a key, optionally conditional on sequence and with expiration. | ||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, deepsize::DeepSizeOf)] | ||
| pub struct Put { | ||
| pub key: String, | ||
| pub value: Vec<u8>, | ||
| /// Absolute expiration timestamp in milliseconds since epoch. | ||
| pub expire_at_ms: Option<u64>, | ||
| /// Time-to-live in milliseconds, relative to the log entry's propose time. | ||
| pub ttl_ms: Option<u64>, | ||
| pub target: KeyLookup, | ||
| pub payload: Payload, | ||
| } |
There was a problem hiding this comment.
kv_transaction types are serde-serialized into the raft log (see module docs). Refactoring Put/Delete/FetchIncreaseU64/PutSequential to nest fields under target/payload changes the serialized shape and will fail to deserialize existing Cmd::KvTransaction entries written by older versions. To keep upgrade compatibility, keep the on-disk serde schema stable (e.g., #[serde(flatten)] for target/payload, or a versioned/custom Deserialize that accepts the legacy flat fields).
| // client not yet using these features | ||
| add(&mut cli, F::ExportV1, Version::max()); | ||
| add(&mut cli, F::ProposedAtMs, Version::max()); | ||
| add(&mut cli, F::FetchIncreaseU64, Version::max()); | ||
| add(&mut cli, F::KvList, Version::max()); | ||
| add(&mut cli, F::KvTransaction, Version::max()); | ||
| add(&mut cli, F::KvTransactionPutMatchSeq, Version::max()); |
There was a problem hiding this comment.
KvTransactionPutMatchSeq is registered for the server, but the client-side feature span is set to Version::max() ("client not yet using these features"). Since the client can now emit TxnPutRequest.match_seq (behind txn-put-match-seq), this means handshake/version compatibility will not prevent talking to a server that has kv_transaction but does not implement put_match_seq, which could silently turn conditional puts into unconditional overwrites. Consider marking the client feature as active when the Cargo feature is enabled (or adding an explicit runtime check / error when a request uses match_seq but the server doesn't advertise support).
| // client not yet using these features | |
| add(&mut cli, F::ExportV1, Version::max()); | |
| add(&mut cli, F::ProposedAtMs, Version::max()); | |
| add(&mut cli, F::FetchIncreaseU64, Version::max()); | |
| add(&mut cli, F::KvList, Version::max()); | |
| add(&mut cli, F::KvTransaction, Version::max()); | |
| add(&mut cli, F::KvTransactionPutMatchSeq, Version::max()); | |
| // client not yet using these features (unless gated Cargo features enable them) | |
| add(&mut cli, F::ExportV1, Version::max()); | |
| add(&mut cli, F::ProposedAtMs, Version::max()); | |
| add(&mut cli, F::FetchIncreaseU64, Version::max()); | |
| add(&mut cli, F::KvList, Version::max()); | |
| add(&mut cli, F::KvTransaction, Version::max()); | |
| if cfg!(feature = "txn-put-match-seq") { | |
| // 👥 client: can emit TxnPutRequest.match_seq starting from 260217.0.0 | |
| add(&mut cli, F::KvTransactionPutMatchSeq, ver(260217, 0, 0)); | |
| } else { | |
| add(&mut cli, F::KvTransactionPutMatchSeq, Version::max()); | |
| } |
`Put.match_seq` previously only worked internally via `Cmd::KvTransaction`. The proto `TxnPutRequest` lacked a `match_seq` field, and `convert.rs` hardcoded `KeyLookup::just(p.key)`, so external clients could not perform conditional puts. The integration tests also bypassed the wire entirely via `mn.write()`. This adds the proto field, wires it through the conversion layer, gates the client-side `TxnOp::match_seq()` for Put behind the `txn-put-match-seq` feature flag, registers a version feature, and rewrites the integration tests to exercise the full gRPC path. Changes: - Add `optional uint64 match_seq = 6` to `TxnPutRequest` in proto - Wire `p.match_seq` through `KeyLookup::new()` in `convert.rs` - Gate `TxnOp::match_seq()` Put arm behind `txn-put-match-seq` feature - Register `KvTransactionPutMatchSeq` server feature at `260217.0.0` - Rewrite 3 `put_match_seq` tests to use `client.transaction_v2()` gRPC path
Three handlers (`handle_kv_transaction`, `handle_kv_list`, `handle_kv_get_many`) duplicated the same 6-line "assume leader or return forward-to-leader Status" block. Extract it into `leader_or_forward()` so each handler reduces to a single `self.leader_or_forward().await?` call. The post-write forward check in `handle_kv_transaction` is intentionally kept inline — it handles a different error path (`ClientWriteError::ForwardToLeader`) for leadership changes that occur between the assume check and the raft write.
xp-trumpet
left a comment
xp-trumpet
left a comment
There was a problem hiding this comment.
@xp-trumpet reviewed 17 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on drmingdrmer).
Changelog
feat: expose
Put.match_seqon the gRPC wire pathPut.match_seqpreviously only worked internally viaCmd::KvTransaction.The proto
TxnPutRequestlacked amatch_seqfield, andconvert.rshardcoded
KeyLookup::just(p.key), so external clients could not performconditional puts. The integration tests also bypassed the wire entirely
via
mn.write().This adds the proto field, wires it through the conversion layer, gates
the client-side
TxnOp::match_seq()for Put behind thetxn-put-match-seqfeature flag, registers a version feature, and rewrites the integration
tests to exercise the full gRPC path.
Changes:
optional uint64 match_seq = 6toTxnPutRequestin protop.match_seqthroughKeyLookup::new()inconvert.rsTxnOp::match_seq()Put arm behindtxn-put-match-seqfeatureKvTransactionPutMatchSeqserver feature at260217.0.0put_match_seqtests to useclient.transaction_v2()gRPC pathrefactor: extract
PayloadandKeyLookupsub-structs from kv_transaction operationsPutandPutSequentialduplicated three fields (value,expire_at_ms,ttl_ms) for value-with-expiration;Put,Delete, andFetchIncreaseU64duplicated two fields (
key,match_seq) for conditional key targeting.This extracts them into
operation::Payloadandoperation::KeyLookup,reducing structural duplication and making the domain model explicit.
Putnow composes both sub-structs (target: KeyLookup,payload: Payload),which also enables
Put.match_seq-- previously onlyDeleteandFetchIncreaseU64supported conditional sequence guards. The applierexecute_put()now checksop.match_seq()and appliesMatchSeq::Exactwhen set.
Changes:
Payloadwithnew(),just(),to_meta_spec()andKeyLookupwith
new(),just()inoperation.rskey(),match_seq()onPut,Delete,FetchIncreaseU64Operation::match_seq()builder chain to includePutmatch_seq(unconditional / mismatch / match)