test: simplify kvapi test suite and add missing coverage by drmingdrmer · Pull Request #54 · databendlabs/databend-meta

drmingdrmer · 2026-02-15T03:34:43Z

Changelog

test: simplify kvapi test suite and add missing coverage

Remove 6 redundant test methods (kv_write_read, kv_write_read_proposed_at,
kv_delete, kv_update, kv_upsert_with_ttl, kv_meta) whose behavior is now
covered by existing transaction tests after upsert_kv moved to KvApiExt.

Extract normalize_pb_meta helper to deduplicate 4 repeated meta-normalization
blocks in normalize_txn_response.

Add tests for previously untested transaction features:

fetch_increase_u64 with non-zero max_value and fetch_max_u64
Ne condition on Seq and Value targets
TxnPutRequest.expire_at via put().with_expires_at_ms()

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

refactor: move `upsert_kv` from `KVApi` trait to `KvApiExt` default method

Every KVApi::upsert_kv implementation already delegates to
transaction() internally (TxnRequest::from_upsert() → transaction()
→ TxnReply::into_upsert_reply()). The read-only SMV003KVApi just
panics with unreachable!(). Moving upsert_kv to KvApiExt as a
default method eliminates these redundant implementations and shrinks
KVApi to three primitive methods: get_many_kv, list_kv,
transaction.

Changes:

Add From<InvalidReply> for io::Error to support the ? operator in
the default method for io::Error-based implementations
Add From<errors::InvalidReply> bound to KVApi::Error so
into_upsert_reply() errors convert automatically
Re-export InvalidReply from errors module for consistency with
IncompleteStream
Remove upsert_via_txn and upsert_kv inherent methods from
ClientHandle, now covered by the extension trait

feat: remove `prev_value` flag from `TxnPutRequest` and `TxnDeleteRequest`

The server now always returns the previous value in transaction
put/delete responses, making the request-side prev_value flag
unnecessary. The field indices are reserved in proto to maintain
wire compatibility with older clients that still send the field.

A new TransactionPrevValue feature is added to version negotiation:
server support since 1.2.304, client adoption at 260214.0.0.

Changes:

Reserve proto field 3 in TxnPutRequest and field 2 in
TxnDeleteRequest
Remove prev_value parameter from TxnPutRequest::new() and
TxnDeleteRequest::new() constructors
Update serialization test fixtures to reflect removed field

test: add unit tests for `KvApiExt` with in-memory mock

Test the KvApiExt extension methods (get_kv, mget_kv,
get_kv_stream, list_kv_collect) in isolation using a MockKVApi
backed by BTreeMap, avoiding the need for a full Raft cluster. The
mock implements only get_many_kv and list_kv; write methods are
left as unimplemented!().

Changes:

Add MockKVApi with BTreeMap<String, protobuf::SeqV> storage
Add tokio dev-dependency to kvapi crate

chore: bump version to 260214.0.0 and enable client `KvGetMany` feature

The client now uses the kv_get_many streaming gRPC API instead of
batch mget_kv. This raises MIN_SERVER_VERSION to 1.2.869 (the
version that introduced the server-side kv_get_many endpoint) and
registers the KvGetMany feature as active starting from this release.

Previously KvGetMany was set to Version::max() (client not yet
using), reflecting that only the server implemented it. Now that the
client calls kv_get_many, the feature gate moves to 260214.0.0.

feat: add streaming get_many with channel-based retry on the client

The previous KVApi::get_many implementation collected all keys into a
Vec before issuing a single MGetKVReq batch RPC. This worked but
defeated the purpose of streaming: memory usage scaled with key count,
and a mid-stream input error still required collecting everything first.

This commit introduces StreamedGetMany, a new request type that
accepts a fallible key stream and uses an unbounded channel to decouple
the input from the gRPC call. On connection success, keys are fed into
the channel concurrently with reading the response; on retryable errors
(e.g. "not leader"), the unconsumed input stream is still available for
the next attempt. Input-side errors are captured in a shared slot and
appended to the output stream after the server response completes.

The kvapi crate gains split_err / ErrorSlot — a utility that
converts a fallible stream into an infallible stream plus an error
capture slot, needed because tonic streaming RPCs require infallible
input streams.

ClientHandle also gains set_current_endpoint so callers (tests in
particular) can pin which server the next RPC targets. The cluster test
builder now gives each client all endpoints but pins its starting
endpoint to the co-located server, ensuring some clients initially hit a
follower and exercise leader-forwarding.

Changes:

Add StreamedGetMany request type with its own Request/Response
message variants
Add handle_stream_get_many with channel-based retry loop
Add split_err / ErrorSlot to kvapi crate
Add set_current_endpoint on ClientHandle
Simplify KVApi::get_many impl to delegate to stream_get_many
Remove Clone bound from forwarding Req generic

feat: implement KVApi for ClientHandle and add kvapi integration tests

The kvapi-test-suite crate has ~35 KVApi test methods but nothing in
this repo ran them. This adds the missing pieces to make the standalone
repo self-contained for testing the full KVApi contract against a real
metasrv cluster via gRPC.

KVApi is implemented directly on ClientHandle<RT>, delegating to
existing methods (upsert_via_txn, list, request(Streamed(MGetKVReq)),
transaction). The get_many_kv impl collects keys from the input
stream until the first error, sends the batch to the server, then chains
the error onto the output — preserving fail-fast semantics while working
within the gRPC batch request model.

Arc<ClientHandle> gets KVApi for free via the blanket Deref impl,
so the test builder returns it directly without a wrapper type.

Changes:

Add kvapi::KVApi impl for ClientHandle<RT> in crates/client/src/kvapi_impl.rs
Add crates/kvapi-tests/ integration test crate with MetaSrvBuilder
implementing ApiBuilder for single-node and 3-node cluster tests

Improvement
Build/Testing/CI

The `kvapi-test-suite` crate has ~35 KVApi test methods but nothing in this repo ran them. This adds the missing pieces to make the standalone repo self-contained for testing the full KVApi contract against a real metasrv cluster via gRPC. `KVApi` is implemented directly on `ClientHandle<RT>`, delegating to existing methods (`upsert_via_txn`, `list`, `request(Streamed(MGetKVReq))`, `transaction`). The `get_many_kv` impl collects keys from the input stream until the first error, sends the batch to the server, then chains the error onto the output — preserving fail-fast semantics while working within the gRPC batch request model. `Arc<ClientHandle>` gets `KVApi` for free via the blanket `Deref` impl, so the test builder returns it directly without a wrapper type. Changes: - Add `kvapi::KVApi` impl for `ClientHandle<RT>` in `crates/client/src/kvapi_impl.rs` - Add `crates/kvapi-tests/` integration test crate with `MetaSrvBuilder` implementing `ApiBuilder` for single-node and 3-node cluster tests

The previous `KVApi::get_many` implementation collected all keys into a `Vec` before issuing a single `MGetKVReq` batch RPC. This worked but defeated the purpose of streaming: memory usage scaled with key count, and a mid-stream input error still required collecting everything first. This commit introduces `StreamedGetMany`, a new request type that accepts a fallible key stream and uses an unbounded channel to decouple the input from the gRPC call. On connection success, keys are fed into the channel concurrently with reading the response; on retryable errors (e.g. "not leader"), the unconsumed input stream is still available for the next attempt. Input-side errors are captured in a shared slot and appended to the output stream after the server response completes. The `kvapi` crate gains `split_err` / `ErrorSlot` — a utility that converts a fallible stream into an infallible stream plus an error capture slot, needed because tonic streaming RPCs require infallible input streams. `ClientHandle` also gains `set_current_endpoint` so callers (tests in particular) can pin which server the next RPC targets. The cluster test builder now gives each client all endpoints but pins its starting endpoint to the co-located server, ensuring some clients initially hit a follower and exercise leader-forwarding. Changes: - Add `StreamedGetMany` request type with its own `Request`/`Response` message variants - Add `handle_stream_get_many` with channel-based retry loop - Add `split_err` / `ErrorSlot` to `kvapi` crate - Add `set_current_endpoint` on `ClientHandle` - Simplify `KVApi::get_many` impl to delegate to `stream_get_many` - Remove `Clone` bound from forwarding `Req` generic

The client now uses the `kv_get_many` streaming gRPC API instead of batch `mget_kv`. This raises `MIN_SERVER_VERSION` to `1.2.869` (the version that introduced the server-side `kv_get_many` endpoint) and registers the `KvGetMany` feature as active starting from this release. Previously `KvGetMany` was set to `Version::max()` (client not yet using), reflecting that only the server implemented it. Now that the client calls `kv_get_many`, the feature gate moves to `260214.0.0`.

Test the `KvApiExt` extension methods (`get_kv`, `mget_kv`, `get_kv_stream`, `list_kv_collect`) in isolation using a `MockKVApi` backed by `BTreeMap`, avoiding the need for a full Raft cluster. The mock implements only `get_many_kv` and `list_kv`; write methods are left as `unimplemented!()`. Changes: - Add `MockKVApi` with `BTreeMap<String, protobuf::SeqV>` storage - Add `tokio` dev-dependency to kvapi crate

…uest` The server now always returns the previous value in transaction put/delete responses, making the request-side `prev_value` flag unnecessary. The field indices are `reserved` in proto to maintain wire compatibility with older clients that still send the field. A new `TransactionPrevValue` feature is added to version negotiation: server support since 1.2.304, client adoption at 260214.0.0. Changes: - Reserve proto field 3 in `TxnPutRequest` and field 2 in `TxnDeleteRequest` - Remove `prev_value` parameter from `TxnPutRequest::new()` and `TxnDeleteRequest::new()` constructors - Update serialization test fixtures to reflect removed field

…ethod Every `KVApi::upsert_kv` implementation already delegates to `transaction()` internally (`TxnRequest::from_upsert()` → `transaction()` → `TxnReply::into_upsert_reply()`). The read-only `SMV003KVApi` just panics with `unreachable!()`. Moving `upsert_kv` to `KvApiExt` as a default method eliminates these redundant implementations and shrinks `KVApi` to three primitive methods: `get_many_kv`, `list_kv`, `transaction`. Changes: - Add `From<InvalidReply> for io::Error` to support the `?` operator in the default method for `io::Error`-based implementations - Add `From<errors::InvalidReply>` bound to `KVApi::Error` so `into_upsert_reply()` errors convert automatically - Re-export `InvalidReply` from `errors` module for consistency with `IncompleteStream` - Remove `upsert_via_txn` and `upsert_kv` inherent methods from `ClientHandle`, now covered by the extension trait

Remove 6 redundant test methods (kv_write_read, kv_write_read_proposed_at, kv_delete, kv_update, kv_upsert_with_ttl, kv_meta) whose behavior is now covered by existing transaction tests after upsert_kv moved to KvApiExt. Extract normalize_pb_meta helper to deduplicate 4 repeated meta-normalization blocks in normalize_txn_response. Add tests for previously untested transaction features: - fetch_increase_u64 with non-zero max_value and fetch_max_u64 - Ne condition on Seq and Value targets - TxnPutRequest.expire_at via put().with_expires_at_ms() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This pull request refactors the KVApi trait structure and adds comprehensive test coverage. The main changes involve moving upsert_kv from the KVApi trait to KvApiExt as a default method, removing the prev_value flag from transaction put/delete requests (now that the server always returns previous values), implementing streaming get-many with retry support, adding KVApi integration tests, and simplifying the test suite by removing redundant test methods.

Changes:

Moved upsert_kv from KVApi trait to KvApiExt default implementation, eliminating redundant implementations across the codebase
Removed prev_value field from TxnPutRequest and TxnDeleteRequest, reserving field indices for backward compatibility
Added comprehensive test coverage for previously untested transaction features (fetch_increase_u64 with max_value, Ne conditions, expire_at)
Implemented KVApi for ClientHandle with streaming get_many_kv support using channel-based retry mechanism
Added kvapi-tests integration test crate for testing against real metasrv clusters

Reviewed changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
Cargo.toml	Bump version to 260214.0.0, add kvapi-tests crate to workspace
crates/version/src/lib.rs	Update MIN_SERVER_VERSION to 1.2.869 and version tests
crates/version/src/spec.rs	Add TransactionPrevValue and KvGetMany feature flags for client
crates/version/src/feat.rs	Add TransactionPrevValue feature enum variant
crates/version/src/changes.md	Document client adoption of kv_get_many gRPC API
crates/types/proto/request.proto	Reserve fields 3 and 2 in TxnPutRequest/TxnDeleteRequest for removed prev_value
crates/types/src/proto_ext/txn_put_request_ext.rs	Remove prev_value parameter from TxnPutRequest::new()
crates/types/src/proto_ext/txn_delete_request_ext.rs	Remove prev_value parameter from TxnDeleteRequest::new()
crates/types/src/proto_ext/txn_op_ext.rs	Update TxnOp factory methods to not pass prev_value
crates/types/src/proto_ext/txn_request_ext.rs	Update test fixtures for removed prev_value field
crates/types/tests/it/txn_serde.rs	Update serialization test fixture
crates/types/src/cmd/mod.rs	Update command serialization test
crates/types/src/errors/mod.rs	Re-export InvalidReply from errors module
crates/types/src/errors/meta_network_errors.rs	Add From for io::Error conversion
crates/kvapi/src/kvapi/api.rs	Remove upsert_kv from KVApi trait, add InvalidReply error bound
crates/kvapi/src/kvapi/kv_api_ext.rs	Add upsert_kv as default method in KvApiExt, add unit tests with MockKVApi
crates/kvapi/Cargo.toml	Add tokio dev-dependency for tests
crates/kvapi-test-suite/src/kvapi_test_suite.rs	Remove 6 redundant test methods, add 3 new test methods, extract normalize_pb_meta helper
crates/raft-store/src/sm_v003/sm_v003_kv_api.rs	Remove upsert_kv implementation from read-only SMV003KVApi
crates/client/src/lib.rs	Add kvapi_impl module, export StreamedGetMany
crates/client/src/kvapi_impl.rs	Implement KVApi trait for ClientHandle
crates/client/src/grpc_action.rs	Add StreamedGetMany request type, remove Clone bound from RequestFor
crates/client/src/grpc_client.rs	Add handle_streamed_get_many with channel-based retry logic
crates/client/src/client_handle.rs	Add set_current_endpoint, streamed_get_many methods, remove upsert_via_txn/upsert_kv
crates/client/src/message.rs	Add Request/Response variants for StreamedGetMany, remove Clone from Request derive
crates/client/Cargo.toml	Add async-trait, databend-meta-kvapi, futures-util dependencies
crates/client/tests/it/grpc_client.rs	Add KvApiExt import
crates/service/src/meta_node/meta_node.rs	Add Clone bound to RequestFor in handle_forwardable_request
crates/service/tests/it/grpc/*.rs	Add KvApiExt imports to multiple test files
crates/kvapi-tests/Cargo.toml	New integration test crate configuration
crates/kvapi-tests/src/lib.rs	New crate library file
crates/kvapi-tests/tests/it/main.rs	Integration test main module
crates/kvapi-tests/tests/it/test_kv_api.rs	Test runner using MetaSrvBuilder
crates/kvapi-tests/tests/it/metasrv_builder.rs	ApiBuilder implementation for real metasrv instances

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-15T03:39:09Z

crates/version/src/spec.rs

+            // 2026-02-14: since 260214.0.0
+            // 👥 client: remove `prev_value` from TxnPutRequest and TxnDeleteRequest
+            // Client starts relying on this feature for removing `prev_value`.
+            // In this version we removed KVApi::upsert, and provide a default impl upon transaction(), which require it to always return prev_value


The comment states "we removed KVApi::upsert" but upsert_kv was moved to KvApiExt as a default method, not removed. Consider updating to: "In this version we moved KVApi::upsert_kv to KvApiExt as a default impl upon transaction(), which requires it to always return prev_value"

Copilot · 2026-02-15T03:39:09Z

crates/version/src/changes.md


 - 2026-01-13: since 1.2.869
  🖥 server: add `kv_get_many` gRPC API: in protobuf, receive stream, return stream.
+- 


There is an extra empty bullet point line (containing only "- "). Consider removing this line to maintain consistency with the rest of the changelog.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f7dd7d99e7

ℹ️ 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".

chatgpt-codex-connector · 2026-02-15T03:40:39Z

crates/client/src/grpc_client.rs

+                    if is_status_retryable(&status) {
+                        warn!("kv_get_many retryable error: {:?}; retrying", status);
+                        last_status = Some(status);
+                        continue;


Rotate endpoint before retrying kv_get_many

When kv_get_many returns a retryable status, this branch immediately continues without rotating away from the failing target. get_established_client() reuses Endpoints::current_or_next(), so if the status has no leader metadata (e.g. transient Unavailable from one node), every retry can hit the same bad endpoint and exhaust attempts even when other endpoints are healthy. Other RPC paths in this client rotate on retryable errors, so this creates a real availability regression specific to streamed get-many.

Useful? React with 👍 / 👎.

xp-trumpet

@xp-trumpet reviewed 42 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @drmingdrmer).

…upport upsert_kv now delegates to transaction(), which uses RPCs that return redirect instead of forwarding to the leader. Clients created with a single endpoint via tc.grpc_client() cannot follow the redirect because the leader address is not in their endpoint list. Fix all service integration tests to create clients with all cluster endpoints and use set_current_endpoint() to control the initial connection target, matching the pattern in metasrv_builder::build_cluster(). Changes: - Update metasrv_grpc_kv_api_restart_cluster to use make_grpc_client with all endpoints instead of tc.grpc_client() - Update metasrv_connection_error to give every client all endpoints, iterating over set_current_endpoint values instead of address subsets - Update test_join and test_auto_sync_addr to use make_grpc_client with all endpoints

xp-trumpet

@xp-trumpet reviewed 4 files and all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @drmingdrmer).

Zhang Yanpo and others added 7 commits February 13, 2026 23:03

Copilot AI review requested due to automatic review settings February 15, 2026 03:34

Copilot started reviewing on behalf of drmingdrmer February 15, 2026 03:35 View session

Copilot AI reviewed Feb 15, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Feb 15, 2026

View reviewed changes

xp-trumpet previously approved these changes Feb 15, 2026

View reviewed changes

drmingdrmer dismissed xp-trumpet’s stale review via f53da1e February 15, 2026 04:26

xp-trumpet approved these changes Feb 15, 2026

View reviewed changes

drmingdrmer added this pull request to the merge queue Feb 15, 2026

Merged via the queue into databendlabs:main with commit db5b622 Feb 15, 2026
3 checks passed

drmingdrmer deleted the 017-xx branch February 15, 2026 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: simplify kvapi test suite and add missing coverage#54

test: simplify kvapi test suite and add missing coverage#54
drmingdrmer merged 8 commits intodatabendlabs:mainfrom
drmingdrmer:017-xx

drmingdrmer commented Feb 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 15, 2026

Uh oh!

Copilot AI Feb 15, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Feb 15, 2026

Uh oh!

xp-trumpet left a comment

Uh oh!

xp-trumpet left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

drmingdrmer commented Feb 15, 2026

Changelog

test: simplify kvapi test suite and add missing coverage

refactor: move upsert_kv from KVApi trait to KvApiExt default method

feat: remove prev_value flag from TxnPutRequest and TxnDeleteRequest

test: add unit tests for KvApiExt with in-memory mock

chore: bump version to 260214.0.0 and enable client KvGetMany feature

feat: add streaming get_many with channel-based retry on the client

feat: implement KVApi for ClientHandle and add kvapi integration tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Feb 15, 2026

Choose a reason for hiding this comment

Uh oh!

xp-trumpet left a comment

Choose a reason for hiding this comment

Uh oh!

xp-trumpet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor: move `upsert_kv` from `KVApi` trait to `KvApiExt` default method

feat: remove `prev_value` flag from `TxnPutRequest` and `TxnDeleteRequest`

test: add unit tests for `KvApiExt` with in-memory mock

chore: bump version to 260214.0.0 and enable client `KvGetMany` feature