feat: add server-side KvTransaction gRPC API (client gated by feature flag)#55
Conversation
Group the 11 flat crates under `crates/` into three category subdirectories that reflect the dependency architecture: `common/` (types, runtime-api, version, test-harness), `client/` (kvapi, kvapi-test-suite, kvapi-tests, client), and `server/` (sled-store, raft-store, service). The client crate now re-exports `kvapi` and `kvapi_test_suite`, and the service crate re-exports `raft_store` and `sled_store`, so downstream consumers only need one dependency per category. Changes: - Add `pub extern crate ... as ...` re-exports in client and service - Add `databend-meta-kvapi-test-suite` as a dependency of the client crate
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 877839b8f3
ℹ️ 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".
xp-trumpet
left a comment
There was a problem hiding this comment.
@xp-trumpet partially reviewed 325 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on drmingdrmer).
xp-trumpet
left a comment
There was a problem hiding this comment.
@xp-trumpet partially reviewed 34 files and all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on drmingdrmer).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 325 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl From<pb::TxnOp> for Operation { | ||
| fn from(op: pb::TxnOp) -> Self { | ||
| use crate::txn_op; | ||
|
|
||
| match op.request { | ||
| Some(txn_op::Request::Get(g)) => Operation::get(g.key), | ||
| Some(txn_op::Request::Put(p)) => Operation::Put(operation::Put { | ||
| key: p.key, | ||
| value: p.value, | ||
| expire_at_ms: p.expire_at, | ||
| ttl_ms: p.ttl_ms, | ||
| }), | ||
| Some(txn_op::Request::Delete(d)) => Operation::Delete(operation::Delete { | ||
| key: d.key, | ||
| match_seq: d.match_seq, | ||
| }), | ||
| Some(txn_op::Request::DeleteByPrefix(d)) => Operation::delete_by_prefix(d.prefix), | ||
| Some(txn_op::Request::FetchIncreaseU64(f)) => { | ||
| Operation::FetchIncreaseU64(operation::FetchIncreaseU64 { | ||
| key: f.key, | ||
| match_seq: f.match_seq, | ||
| delta: f.delta, | ||
| floor: f.max_value, | ||
| }) | ||
| } | ||
| Some(txn_op::Request::PutSequential(p)) => { | ||
| Operation::PutSequential(operation::PutSequential { | ||
| prefix: p.prefix, | ||
| sequence_key: p.sequence_key, | ||
| value: p.value, | ||
| expire_at_ms: p.expires_at_ms, | ||
| ttl_ms: p.ttl_ms, | ||
| }) | ||
| } | ||
| None => Operation::get(""), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
TxnOp { request: None } used to be skipped by the legacy evaluator (if let Some(request) { ... }). Converting it to Operation::get(\"\") changes behavior and can cause unintended reads of the empty key. Consider representing this as Option<Operation> (return None when request is missing) and filter_map when building operations, or introduce an explicit Noop variant that results in no response/no-op execution.
| impl CompareOperator { | ||
| fn from_condition_result(expected: i32) -> Self { | ||
| use crate::ConditionResult; | ||
|
|
||
| match <ConditionResult as num_traits::FromPrimitive>::from_i32(expected) { | ||
| Some(ConditionResult::Eq) => CompareOperator::Eq, | ||
| Some(ConditionResult::Gt) => CompareOperator::Gt, | ||
| Some(ConditionResult::Ge) => CompareOperator::Ge, | ||
| Some(ConditionResult::Lt) => CompareOperator::Lt, | ||
| Some(ConditionResult::Le) => CompareOperator::Le, | ||
| Some(ConditionResult::Ne) => CompareOperator::Ne, | ||
| None => CompareOperator::Eq, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
For invalid expected values, legacy code logged a warning and treated the condition as false. Defaulting to CompareOperator::Eq can incorrectly make a previously-false condition pass, changing transaction semantics. To preserve backward compatibility, treat invalid operators as always-false (e.g., return an Option<CompareOperator> and have the caller construct an always-false condition, or add an explicit Invalid operator handled as false).
| Err(e) => return Err(Status::internal(e.to_string())), | ||
| }; | ||
|
|
||
| let reply: KvTransactionReply = applied.try_into().expect("expect KvTransactionReply"); |
There was a problem hiding this comment.
Using expect() in the gRPC request path can panic the server process if AppliedState is not the expected variant (e.g., a regression in applier wiring). Prefer converting with error handling and returning Status::internal(...) on mismatch, so a single bad state does not crash the meta service.
| let reply: KvTransactionReply = applied.try_into().expect("expect KvTransactionReply"); | |
| let reply: KvTransactionReply = match applied.try_into() { | |
| Ok(reply) => reply, | |
| Err(e) => { | |
| return Err(Status::internal(format!( | |
| "failed to convert AppliedState to KvTransactionReply: {}", | |
| e | |
| ))); | |
| } | |
| }; |
| let kv_reply: pb::KvTransactionReply = | ||
| applied_state.try_into().expect("expect KvTransactionReply"); | ||
| let txn_reply: TxnReply = kv_reply.into_txn_reply(&txn); |
There was a problem hiding this comment.
Same concern as server-side: expect() here can panic the handler thread on an unexpected AppliedState, turning a recoverable server response mismatch into a crash. Consider mapping the conversion failure into a Status::internal(...) (or whatever error type is used in this forwarding path) and include enough context to debug the unexpected applied state.
xp-trumpet
left a comment
There was a problem hiding this comment.
@xp-trumpet partially reviewed 24 files.
Reviewable status: 318 of 325 files reviewed, 7 unresolved discussions (waiting on drmingdrmer).
…re flag) The existing `Transaction` endpoint stores protobuf `TxnRequest` directly in the raft log, coupling wire format to persistence. Any proto schema change risks breaking raft log deserialization. The new `KvTransaction` endpoint introduces a three-layer architecture: protobuf transport types for the wire, Rust-native serde types (`kv_transaction::Transaction`) for raft log storage, and conversions between them. This lets the proto schema evolve freely without affecting stored data. The new API uses a cleaner branch-based model where each branch has an optional predicate and a list of operations, evaluated in order. This replaces the legacy dual execution paths, stringly-typed `execution_path`, and misleading `success` field of `TxnRequest`. The server now exposes the `KvTransaction` RPC endpoint, but the client does not use it by default — `ClientHandle::transaction_v2()` is gated behind the `transaction-v2` feature flag and only enabled in tests. Production client code continues to use the old `Transaction` API. The new endpoint will be adopted on the client side in a future change once the server has been deployed widely enough. The applier is rewritten to operate on `kv_transaction::*` types. The old `Cmd::Transaction(TxnRequest)` path converts to `kv_transaction::Transaction` internally and delegates to the same code. Both paths return `AppliedState::KvTxnReply(pb::KvTransactionReply)`. Changes: - Add `kv_transaction` module with `Transaction`, `Branch`, `Predicate`, `Condition`, `Operation`, `Operand`, `CompareOperator` storage types - Add `Cmd::KvTransaction(Transaction)` variant - Replace `AppliedState::TxnReply` with `AppliedState::KvTxnReply` - Add `KvTransaction` RPC using redirect pattern (not forward), returning leader endpoint in gRPC metadata for client-side redirect - Gate `ClientHandle::transaction_v2()` behind `transaction-v2` feature flag - Register `Feature::KvTransaction` at version 260217.0.0
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 325 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
xp-trumpet
left a comment
There was a problem hiding this comment.
@xp-trumpet partially reviewed 8 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on drmingdrmer).
Changelog
feat: add server-side
KvTransactiongRPC API (client gated by feature flag)The existing
Transactionendpoint stores protobufTxnRequestdirectly in theraft log, coupling wire format to persistence. Any proto schema change risks
breaking raft log deserialization. The new
KvTransactionendpoint introduces athree-layer architecture: protobuf transport types for the wire, Rust-native
serde types (
kv_transaction::Transaction) for raft log storage, and conversionsbetween them. This lets the proto schema evolve freely without affecting stored
data.
The new API uses a cleaner branch-based model where each branch has an optional
predicate and a list of operations, evaluated in order. This replaces the legacy
dual execution paths, stringly-typed
execution_path, and misleadingsuccessfield of
TxnRequest.The server now exposes the
KvTransactionRPC endpoint, but the client does notuse it by default —
ClientHandle::transaction_v2()is gated behind thetransaction-v2feature flag and only enabled in tests. Production client codecontinues to use the old
TransactionAPI. The new endpoint will be adopted onthe client side in a future change once the server has been deployed widely
enough.
The applier is rewritten to operate on
kv_transaction::*types. The oldCmd::Transaction(TxnRequest)path converts tokv_transaction::Transactioninternally and delegates to the same code. Both paths return
AppliedState::KvTxnReply(pb::KvTransactionReply).Changes:
kv_transactionmodule withTransaction,Branch,Predicate,Condition,Operation,Operand,CompareOperatorstorage typesCmd::KvTransaction(Transaction)variantAppliedState::TxnReplywithAppliedState::KvTxnReplyKvTransactionRPC using redirect pattern (not forward), returningleader endpoint in gRPC metadata for client-side redirect
ClientHandle::transaction_v2()behindtransaction-v2feature flagFeature::KvTransactionat version 260217.0.0chore: Bump ver: 260217.0.0
refactor: reorganize crates into common/client/server categories
Group the 11 flat crates under
crates/into three categorysubdirectories that reflect the dependency architecture:
common/(types, runtime-api, version, test-harness),
client/(kvapi,kvapi-test-suite, kvapi-tests, client), and
server/(sled-store,raft-store, service).
The client crate now re-exports
kvapiandkvapi_test_suite, andthe service crate re-exports
raft_storeandsled_store, sodownstream consumers only need one dependency per category.
Changes:
pub extern crate ... as ...re-exports in client and servicedatabend-meta-kvapi-test-suiteas a dependency of the client crate