From e50bf490268fdcaca6ab2436b3343ce5c38cc9ee Mon Sep 17 00:00:00 2001 From: Joshua Chi <112539+JoshuaChi@users.noreply.github.com> Date: Fri, 22 May 2026 17:16:29 +0800 Subject: [PATCH 1/3] refactor #260: expunge proto::ClientResult from user-facing API Replace proto::ClientResult with core::KvEntry as the return type of GrpcClient::get_with_policy() and get_multi_with_policy(). Previously these power-user methods leaked a protobuf-generated type into the public API surface despite the rest of the read API (ClientApi::get, get_linearizable, etc.) already returning plain Bytes. Changes: - client_ext.rs: into_read_results() now returns Vec> - grpc_client.rs: get_with_policy / get_multi_with_policy return KvEntry - lib.rs: protocol module replaces ClientResult export with KvEntry - examples/client-usage-standalone: updated to KvEntry pattern match Tests (TDD -- red before green): - test_into_read_results_success: explicit Vec> annotation - test_into_read_results_multiple_entries_are_kv_entries (new) - test_get_with_policy_returns_native_kv_entry (new) - test_get_multi_with_policy_returns_native_kv_entries (new) proto::ClientResult survives only inside d-engine-proto's wire layer. All user-visible read results are now proto-free native Rust types. --- d-engine-client/src/error_test.rs | 2 +- d-engine-client/src/grpc_client.rs | 14 +- d-engine-client/src/grpc_client_test.rs | 130 +++++- d-engine-client/src/lib.rs | 8 +- d-engine-client/src/mock_rpc.rs | 56 ++- d-engine-client/src/mock_rpc_service.rs | 4 +- d-engine-client/src/pool.rs | 2 +- d-engine-client/src/pool_test.rs | 2 +- d-engine-client/src/proto/client_ext.rs | 23 +- d-engine-client/src/proto/client_ext_test.rs | 53 ++- d-engine-core/src/client/client_api.rs | 4 +- d-engine-core/src/client/client_api_error.rs | 5 +- .../src/client/client_api_error_test.rs | 4 +- d-engine-core/src/client/mod.rs | 9 + d-engine-core/src/client/types.rs | 270 ++++++++++++ d-engine-core/src/client/types_test.rs | 401 ++++++++++++++++++ d-engine-core/src/event.rs | 4 +- .../raft_role/buffers/propose_batch_buffer.rs | 2 +- .../buffers/propose_batch_buffer_test.rs | 2 +- .../src/raft_role/candidate_state_test.rs | 37 +- .../src/raft_role/follower_state_test.rs | 36 +- d-engine-core/src/raft_role/leader_state.rs | 51 ++- .../leader_state_test/backpressure_test.rs | 42 +- .../leader_state_test/buffer_cleanup_test.rs | 34 +- .../leader_state_test/client_read_test.rs | 57 ++- .../leader_state_test/client_write_test.rs | 176 +++++++- .../leader_state_test/commit_index_test.rs | 2 +- .../leader_state_test/deadline_test.rs | 31 +- .../leader_state_test/event_handling_test.rs | 22 +- .../leader_state_test/fatal_error_test.rs | 8 +- .../pending_lease_reads_test.rs | 8 +- .../leader_state_test/pending_reads_test.rs | 14 +- .../leader_state_test/replication_test.rs | 2 +- .../src/raft_role/learner_state_test.rs | 37 +- d-engine-core/src/raft_role/role_state.rs | 28 +- d-engine-core/src/replication/mod.rs | 2 +- .../default_state_machine_handler.rs | 7 +- .../src/state_machine_handler/mod.rs | 4 +- d-engine-server/src/api/embedded_client.rs | 143 ++++--- d-engine-server/src/lib.rs | 13 +- .../src/network/grpc/grpc_raft_service.rs | 23 +- d-engine-server/src/proto_convert.rs | 238 +++++++++++ d-engine-server/src/proto_convert_test.rs | 390 +++++++++++++++++ .../leader_failover_cas_standalone.rs | 2 +- d-engine-server/tests/client_manager/mod.rs | 4 +- d-engine-server/tests/common/mod.rs | 2 +- .../embedded_client_operations.rs | 2 +- .../learner_readonly_sync_standalone.rs | 2 +- .../watch_events_grpc_standalone.rs | 2 +- examples/client-usage-standalone/src/main.rs | 4 +- 50 files changed, 2055 insertions(+), 363 deletions(-) create mode 100644 d-engine-core/src/client/types.rs create mode 100644 d-engine-core/src/client/types_test.rs create mode 100644 d-engine-server/src/proto_convert.rs create mode 100644 d-engine-server/src/proto_convert_test.rs diff --git a/d-engine-client/src/error_test.rs b/d-engine-client/src/error_test.rs index 48b9261f..529def7d 100644 --- a/d-engine-client/src/error_test.rs +++ b/d-engine-client/src/error_test.rs @@ -1,4 +1,4 @@ -use d_engine_proto::error::ErrorCode; +use d_engine_core::client::ErrorCode; use tonic::Code; use tonic::Status; diff --git a/d-engine-client/src/grpc_client.rs b/d-engine-client/src/grpc_client.rs index 65500246..a11b5054 100644 --- a/d-engine-client/src/grpc_client.rs +++ b/d-engine-client/src/grpc_client.rs @@ -3,18 +3,18 @@ use std::sync::Arc; use arc_swap::ArcSwap; use bytes::Bytes; use d_engine_core::ScanResult; +use d_engine_core::client::ErrorCode; +use d_engine_core::client::KvEntry; +use d_engine_core::config::ReadConsistencyPolicy; use d_engine_proto::client::ClientReadRequest; -use d_engine_proto::client::ClientResult; use d_engine_proto::client::ClientWriteRequest; use d_engine_proto::client::MembershipSnapshot; -use d_engine_proto::client::ReadConsistencyPolicy; use d_engine_proto::client::ScanRequest; use d_engine_proto::client::WatchMembershipRequest; use d_engine_proto::client::WatchRequest; use d_engine_proto::client::WatchResponse; use d_engine_proto::client::WriteCommand; use d_engine_proto::client::raft_client_service_client::RaftClientServiceClient; -use d_engine_proto::error::ErrorCode; use rand::Rng; use rand::SeedableRng; use rand::rngs::StdRng; @@ -56,7 +56,7 @@ impl GrpcClient { &self, key: impl AsRef<[u8]>, consistency_policy: Option, - ) -> std::result::Result, ClientApiError> { + ) -> std::result::Result, ClientApiError> { // Delegate to multi-get implementation let mut results = self.get_multi_with_policy(std::iter::once(key), consistency_policy).await?; @@ -73,7 +73,7 @@ impl GrpcClient { &self, keys: impl IntoIterator>, consistency_policy: Option, - ) -> std::result::Result>, ClientApiError> { + ) -> std::result::Result>, ClientApiError> { let _timer = ScopedTimer::new("client::get_multi"); let client_inner = self.client_inner.load(); @@ -92,7 +92,9 @@ impl GrpcClient { let request = ClientReadRequest { client_id: client_inner.client_id, keys, - consistency_policy: consistency_policy.map(|p| p as i32), + consistency_policy: consistency_policy + .clone() + .map(|p| d_engine_proto::client::ReadConsistencyPolicy::from(p) as i32), }; // Select client based on policy (if specified) diff --git a/d-engine-client/src/grpc_client_test.rs b/d-engine-client/src/grpc_client_test.rs index 0f3769a3..f8d19f40 100644 --- a/d-engine-client/src/grpc_client_test.rs +++ b/d-engine-client/src/grpc_client_test.rs @@ -5,12 +5,12 @@ use arc_swap::ArcSwap; use bytes::Bytes; use d_engine_core::ClientApi; use d_engine_core::client::ClientApiResult; -use d_engine_proto::client::ClientResponse; -use d_engine_proto::client::ClientResult; -use d_engine_proto::client::ReadConsistencyPolicy; +use d_engine_core::client::ClientResponse; +use d_engine_core::client::ErrorCode; +use d_engine_core::client::KvEntry; +use d_engine_core::config::ReadConsistencyPolicy; use d_engine_proto::client::WatchEventType; use d_engine_proto::client::WatchResponse; -use d_engine_proto::error::ErrorCode; use d_engine_proto::server::cluster::ClusterMembership; use tokio::sync::oneshot; use tracing_test::traced_test; @@ -219,7 +219,7 @@ async fn test_get_success() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: key.clone(), value: value.clone(), }]), @@ -369,7 +369,7 @@ async fn test_get_multi_success_linear() { // Create a single response containing all key-value pairs let mut client_results = Vec::new(); for (i, key) in keys.iter().enumerate() { - client_results.push(ClientResult { + client_results.push(KvEntry { key: key.clone(), value: values[i].clone(), }); @@ -438,7 +438,7 @@ async fn test_get_multi_success_non_linear() { // Create a single response containing all key-value pairs let mut client_results = Vec::new(); for (i, key) in keys.iter().enumerate() { - client_results.push(ClientResult { + client_results.push(KvEntry { key: key.clone(), value: match &values[i] { Some(value) => value.clone(), @@ -580,7 +580,7 @@ async fn test_get_linearizable_success() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: key.clone(), value: value.clone(), }]), @@ -618,7 +618,7 @@ async fn test_get_lease_success() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: key.clone(), value: value.clone(), }]), @@ -656,7 +656,7 @@ async fn test_get_eventual_success() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: key.clone(), value: value.clone(), }]), @@ -699,7 +699,7 @@ async fn test_get_multi_success() { let mut client_results = Vec::new(); for (i, key) in keys.iter().enumerate() { - client_results.push(ClientResult { + client_results.push(KvEntry { key: key.clone(), value: values[i].clone(), }); @@ -752,11 +752,11 @@ async fn test_get_multi_with_mixed_results() { // Only key1 and key3 exist, key2 is missing let client_results = vec![ - ClientResult { + KvEntry { key: keys[0].clone(), value: Bytes::from("value1".to_string()), }, - ClientResult { + KvEntry { key: keys[2].clone(), value: Bytes::from("value3".to_string()), }, @@ -882,7 +882,7 @@ async fn test_client_refresh_with_new_endpoints() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: Bytes::from("key1"), value: Bytes::from("value1"), }]), @@ -920,7 +920,7 @@ async fn test_client_refresh_with_new_endpoints() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: Bytes::from("key2"), value: Bytes::from("value2"), }]), @@ -951,7 +951,7 @@ async fn test_client_refresh_with_none_endpoints() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: Bytes::from("key1"), value: Bytes::from("value1"), }]), @@ -1003,7 +1003,7 @@ async fn test_client_refresh_with_multiple_endpoints() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: Bytes::from("key1"), value: Bytes::from("value1"), }]), @@ -1017,7 +1017,7 @@ async fn test_client_refresh_with_multiple_endpoints() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: Bytes::from("key2"), value: Bytes::from("value2"), }]), @@ -1077,7 +1077,7 @@ async fn test_client_refresh_failure_invalid_endpoints() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: Bytes::from("key1"), value: Bytes::from("value1"), }]), @@ -1134,7 +1134,7 @@ async fn test_client_refresh_preserves_kv_and_cluster_clients() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: Bytes::from("key1"), value: Bytes::from("value1"), }]), @@ -1168,7 +1168,7 @@ async fn test_client_refresh_preserves_kv_and_cluster_clients() { None::< Box std::result::Result + Send + Sync>, >, - ClientResponse::read_results(vec![ClientResult { + ClientResponse::read_results(vec![KvEntry { key: Bytes::from("key2"), value: Bytes::from("value2"), }]), @@ -1738,3 +1738,91 @@ mod scan_tests { assert!(result.entries.is_empty()); } } + +// ─── Type boundary tests: get_with_policy / get_multi_with_policy must return KvEntry ────── + +#[tokio::test] +#[traced_test] +async fn test_get_with_policy_returns_native_kv_entry() { + let key = Bytes::from("kv_key"); + let value = Bytes::from("kv_value"); + let (_tx, rx) = oneshot::channel::<()>(); + let (_channel, port) = MockNode::simulate_client_read_mock_server( + rx, + None::< + Box std::result::Result + Send + Sync>, + >, + ClientResponse::read_results(vec![KvEntry { + key: key.clone(), + value: value.clone(), + }]), + ) + .await + .unwrap(); + + let endpoints = vec![format!("http://localhost:{}", port)]; + let config = ClientConfig::default(); + let pool = ConnectionPool::create(endpoints.clone(), config.clone()).await.unwrap(); + let client = GrpcClient::new(Arc::new(ArcSwap::from_pointee(ClientInner { + pool, + client_id: 1, + config, + endpoints, + }))); + + // Explicit type annotation: return type must be KvEntry, not proto::ClientResult + let entry: Option = client + .get_with_policy(key.clone(), Some(ReadConsistencyPolicy::LinearizableRead)) + .await + .unwrap(); + + assert_eq!(entry, Some(KvEntry { key, value })); +} + +#[tokio::test] +#[traced_test] +async fn test_get_multi_with_policy_returns_native_kv_entries() { + let k1 = Bytes::from("mk1"); + let v1 = Bytes::from("mv1"); + let k2 = Bytes::from("mk2"); + let v2 = Bytes::from("mv2"); + let (_tx, rx) = oneshot::channel::<()>(); + let (_channel, port) = MockNode::simulate_client_read_mock_server( + rx, + None::< + Box std::result::Result + Send + Sync>, + >, + ClientResponse::read_results(vec![ + KvEntry { + key: k1.clone(), + value: v1.clone(), + }, + KvEntry { + key: k2.clone(), + value: v2.clone(), + }, + ]), + ) + .await + .unwrap(); + + let endpoints = vec![format!("http://localhost:{}", port)]; + let config = ClientConfig::default(); + let pool = ConnectionPool::create(endpoints.clone(), config.clone()).await.unwrap(); + let client = GrpcClient::new(Arc::new(ArcSwap::from_pointee(ClientInner { + pool, + client_id: 1, + config, + endpoints, + }))); + + // Explicit type annotation: must be Vec>, not Vec> + let results: Vec> = client + .get_multi_with_policy(vec![k1.clone(), k2.clone()].into_iter(), None) + .await + .unwrap(); + + assert_eq!(results.len(), 2); + assert_eq!(results[0], Some(KvEntry { key: k1, value: v1 })); + assert_eq!(results[1], Some(KvEntry { key: k2, value: v2 })); +} diff --git a/d-engine-client/src/lib.rs b/d-engine-client/src/lib.rs index ca05eebb..6f399b92 100644 --- a/d-engine-client/src/lib.rs +++ b/d-engine-client/src/lib.rs @@ -73,8 +73,8 @@ mod utils; pub use builder::*; pub use config::*; +pub use d_engine_core::client::ErrorCode; pub use d_engine_core::client::{ClientApi, ClientApiError, ClientApiResult}; -pub use d_engine_proto::error::ErrorCode; pub use grpc_client::*; pub(crate) use pool::*; @@ -83,12 +83,12 @@ pub(crate) use pool::*; /// Protocol types needed for client operations /// /// These types are used in the public API and must be imported for client usage: -/// - `ClientResult`: Response type from read operations +/// - `KvEntry`: Key-value pair returned by read operations /// - `ReadConsistencyPolicy`: Consistency guarantees for reads /// - `WriteCommand`: Write operation specifications pub mod protocol { - pub use d_engine_proto::client::ClientResult; - pub use d_engine_proto::client::ReadConsistencyPolicy; + pub use d_engine_core::client::KvEntry; + pub use d_engine_core::config::ReadConsistencyPolicy; pub use d_engine_proto::client::WatchEventType; pub use d_engine_proto::client::WatchRequest; pub use d_engine_proto::client::WatchResponse; diff --git a/d-engine-client/src/mock_rpc.rs b/d-engine-client/src/mock_rpc.rs index 5b7538be..f9c1c51d 100644 --- a/d-engine-client/src/mock_rpc.rs +++ b/d-engine-client/src/mock_rpc.rs @@ -1,6 +1,7 @@ use std::pin::Pin; use std::sync::Arc; +use d_engine_core::client::ClientResponse as CoreClientResponse; use d_engine_proto::client::ClientReadRequest; use d_engine_proto::client::ClientResponse; use d_engine_proto::client::ClientWriteRequest; @@ -28,8 +29,8 @@ pub struct MockRpcService { // Expected responses for each method pub expected_update_cluster_conf_response: Option>, - pub expected_client_propose_response: Option>, - pub expected_client_read_response: Option>, + pub expected_client_propose_response: Option>, + pub expected_client_read_response: Option>, #[allow(clippy::type_complexity)] pub expected_metadata_response: @@ -63,6 +64,49 @@ impl MockRpcService { } } +/// Convert core `ClientResponse` to the proto wire type for test mock gRPC handlers. +/// Both `ErrorCode` enums are `#[repr(i32)]` with identical discriminants. +fn core_to_proto_response(r: CoreClientResponse) -> ClientResponse { + use d_engine_core::client::{ClientResponsePayload, WriteResult}; + use d_engine_proto::client::{ + ClientResult, ReadResults as ProtoReadResults, WriteResult as ProtoWriteResult, + client_response::SuccessResult, + }; + use d_engine_proto::error::ErrorMetadata; + + let error_code = r.error as i32; + let metadata = if r.leader_hint.is_some() || r.retry_after_ms.is_some() { + Some(ErrorMetadata { + retry_after_ms: r.retry_after_ms, + leader_id: r.leader_hint.as_ref().map(|h| h.leader_id.to_string()), + leader_address: r.leader_hint.map(|h| h.address), + debug_message: None, + }) + } else { + None + }; + let success_result = r.result.map(|payload| match payload { + ClientResponsePayload::Write(WriteResult { succeeded }) => { + SuccessResult::WriteResult(ProtoWriteResult { succeeded }) + } + ClientResponsePayload::Read(rd) => SuccessResult::ReadData(ProtoReadResults { + results: rd + .entries + .into_iter() + .map(|e| ClientResult { + key: e.key, + value: e.value, + }) + .collect(), + }), + }); + ClientResponse { + error: error_code, + metadata, + success_result, + } +} + #[tonic::async_trait] impl ClusterManagementService for MockRpcService { async fn update_cluster_conf( @@ -124,7 +168,9 @@ impl RaftClientService for MockRpcService { _request: tonic::Request, ) -> std::result::Result, tonic::Status> { match &self.expected_client_propose_response { - Some(Ok(response)) => Ok(tonic::Response::new(response.clone())), + Some(Ok(response)) => Ok(tonic::Response::new(core_to_proto_response( + response.clone(), + ))), Some(Err(status)) => Err(status.clone()), None => Err(tonic::Status::unknown( "No mock handle_client_write response set", @@ -137,7 +183,9 @@ impl RaftClientService for MockRpcService { _request: tonic::Request, ) -> std::result::Result, tonic::Status> { match &self.expected_client_read_response { - Some(Ok(response)) => Ok(tonic::Response::new(response.clone())), + Some(Ok(response)) => Ok(tonic::Response::new(core_to_proto_response( + response.clone(), + ))), Some(Err(status)) => Err(status.clone()), None => Err(tonic::Status::unknown( "No mock handle_client_read response set", diff --git a/d-engine-client/src/mock_rpc_service.rs b/d-engine-client/src/mock_rpc_service.rs index ae1b8e12..73357db5 100644 --- a/d-engine-client/src/mock_rpc_service.rs +++ b/d-engine-client/src/mock_rpc_service.rs @@ -1,7 +1,7 @@ use std::net::SocketAddr; use std::sync::Arc; -use d_engine_proto::client::ClientResponse; +use d_engine_core::client::ClientResponse; use d_engine_proto::client::MembershipSnapshot; use d_engine_proto::client::WatchResponse; use d_engine_proto::client::raft_client_service_server::RaftClientServiceServer; @@ -197,7 +197,7 @@ impl MockNode { cas_should_succeed: bool, ) -> std::result::Result<(Channel, u16), tonic::Status> { let response = if cas_should_succeed { - ClientResponse::cas_success() + ClientResponse::write_success() } else { ClientResponse::cas_failure() }; diff --git a/d-engine-client/src/pool.rs b/d-engine-client/src/pool.rs index 56726c38..022d670e 100644 --- a/d-engine-client/src/pool.rs +++ b/d-engine-client/src/pool.rs @@ -1,6 +1,6 @@ use std::time::Duration; -use d_engine_proto::error::ErrorCode; +use d_engine_core::client::ErrorCode; use d_engine_proto::server::cluster::ClusterMembership; use d_engine_proto::server::cluster::MetadataRequest; use d_engine_proto::server::cluster::NodeMeta; diff --git a/d-engine-client/src/pool_test.rs b/d-engine-client/src/pool_test.rs index 8a896776..4ff692bf 100644 --- a/d-engine-client/src/pool_test.rs +++ b/d-engine-client/src/pool_test.rs @@ -4,8 +4,8 @@ use std::sync::atomic::Ordering; use std::time::Duration; use std::vec; +use d_engine_core::client::ErrorCode; use d_engine_proto::common::NodeStatus; -use d_engine_proto::error::ErrorCode; use d_engine_proto::server::cluster::ClusterMembership; use d_engine_proto::server::cluster::NodeMeta; use tokio::sync::oneshot; diff --git a/d-engine-client/src/proto/client_ext.rs b/d-engine-client/src/proto/client_ext.rs index 90c0c13b..8d9ed615 100644 --- a/d-engine-client/src/proto/client_ext.rs +++ b/d-engine-client/src/proto/client_ext.rs @@ -1,7 +1,7 @@ +use d_engine_core::client::ErrorCode; +use d_engine_core::client::KvEntry; use d_engine_proto::client::ClientResponse; -use d_engine_proto::client::ClientResult; use d_engine_proto::client::client_response::SuccessResult; -use d_engine_proto::error::ErrorCode; use tracing::error; use crate::ClientApiError; @@ -19,13 +19,17 @@ pub trait ClientResponseExt { /// /// # Returns /// Vector of optional key-value pairs wrapped in Result - fn into_read_results(self) -> std::result::Result>, ClientApiError>; + fn into_read_results(self) -> std::result::Result>, ClientApiError>; /// Validate error code in response header /// /// # Internal Logic /// Converts numeric error code to enum variant fn validate_error(&self) -> std::result::Result<(), ClientApiError>; + + /// Returns true if this is a successful write with `succeeded = true`. + #[allow(dead_code)] + fn is_write_success(&self) -> bool; } impl ClientResponseExt for ClientResponse { @@ -49,7 +53,7 @@ impl ClientResponseExt for ClientResponse { "Unexpected response type for write operation: expected WriteResult, found {found}" ); Err(ClientApiError::Protocol { - code: d_engine_proto::error::ErrorCode::InvalidResponse, + code: ErrorCode::InvalidResponse, message: format!( "Unexpected response type: expected WriteResult, found {found}" ), @@ -63,7 +67,7 @@ impl ClientResponseExt for ClientResponse { /// /// # Returns /// Vector of optional key-value pairs wrapped in Result - fn into_read_results(self) -> std::result::Result>, ClientApiError> { + fn into_read_results(self) -> std::result::Result>, ClientApiError> { self.validate_error()?; match &self.success_result { Some(SuccessResult::ReadData(data)) => data @@ -71,7 +75,7 @@ impl ClientResponseExt for ClientResponse { .clone() .into_iter() .map(|item| { - Ok(Some(ClientResult { + Ok(Some(KvEntry { key: item.key, value: item.value, })) @@ -88,7 +92,7 @@ impl ClientResponseExt for ClientResponse { found ); Err(ClientApiError::Protocol { - code: d_engine_proto::error::ErrorCode::InvalidResponse, + code: ErrorCode::InvalidResponse, message: format!("Unexpected response type: expected ReadData, found {found}",), supported_versions: None, }) @@ -106,4 +110,9 @@ impl ClientResponseExt for ClientResponse { e => Err(e.into()), } } + + fn is_write_success(&self) -> bool { + self.error == ErrorCode::Success as i32 + && matches!(&self.success_result, Some(SuccessResult::WriteResult(w)) if w.succeeded) + } } diff --git a/d-engine-client/src/proto/client_ext_test.rs b/d-engine-client/src/proto/client_ext_test.rs index 15f52833..3f237c0e 100644 --- a/d-engine-client/src/proto/client_ext_test.rs +++ b/d-engine-client/src/proto/client_ext_test.rs @@ -1,10 +1,11 @@ use bytes::Bytes; +use d_engine_core::client::ErrorCode; +use d_engine_core::client::KvEntry; use d_engine_proto::client::ClientResponse; use d_engine_proto::client::ClientResult; use d_engine_proto::client::ReadResults; use d_engine_proto::client::WriteResult; use d_engine_proto::client::client_response::SuccessResult; -use d_engine_proto::error::ErrorCode; use super::*; use crate::ClientApiError; @@ -111,11 +112,53 @@ fn test_into_read_results_success() { })), }; - let result = response.into_read_results(); - assert!(result.is_ok()); - let data = result.unwrap(); + // Type annotation is the spec: into_read_results must return KvEntry, not proto ClientResult + let data: Vec> = response.into_read_results().unwrap(); assert_eq!(data.len(), 1); - assert!(data[0].is_some()); + assert_eq!( + data[0], + Some(KvEntry { + key: Bytes::from(vec![1, 2, 3]), + value: Bytes::from(vec![4, 5, 6]), + }) + ); +} + +#[test] +fn test_into_read_results_multiple_entries_are_kv_entries() { + let response = ClientResponse { + error: ErrorCode::Success as i32, + metadata: None, + success_result: Some(SuccessResult::ReadData(ReadResults { + results: vec![ + ClientResult { + key: Bytes::from("k1"), + value: Bytes::from("v1"), + }, + ClientResult { + key: Bytes::from("k2"), + value: Bytes::from("v2"), + }, + ], + })), + }; + + let data: Vec> = response.into_read_results().unwrap(); + assert_eq!(data.len(), 2); + assert_eq!( + data[0], + Some(KvEntry { + key: Bytes::from("k1"), + value: Bytes::from("v1") + }) + ); + assert_eq!( + data[1], + Some(KvEntry { + key: Bytes::from("k2"), + value: Bytes::from("v2") + }) + ); } #[test] diff --git a/d-engine-core/src/client/client_api.rs b/d-engine-core/src/client/client_api.rs index 26d70115..48d66f0b 100644 --- a/d-engine-core/src/client/client_api.rs +++ b/d-engine-core/src/client/client_api.rs @@ -27,8 +27,8 @@ use crate::ScanResult; use crate::client::client_api_error::ClientApiResult; +use crate::config::ReadConsistencyPolicy; use bytes::Bytes; -use d_engine_proto::client::ReadConsistencyPolicy; /// Unified key-value store interface. /// @@ -255,7 +255,7 @@ pub trait ClientApi: Send + Sync { /// # Example /// /// ```rust,ignore - /// use d_engine_proto::client::ReadConsistencyPolicy; + /// use crate::config::ReadConsistencyPolicy; /// /// let keys = vec![ /// Bytes::from("key1"), diff --git a/d-engine-core/src/client/client_api_error.rs b/d-engine-core/src/client/client_api_error.rs index f3754344..3c5a62ba 100644 --- a/d-engine-core/src/client/client_api_error.rs +++ b/d-engine-core/src/client/client_api_error.rs @@ -1,6 +1,6 @@ use std::error::Error; -use d_engine_proto::error::ErrorCode; +use crate::client::types::ErrorCode; use serde::Deserialize; use serde::Serialize; use tokio::task::JoinError; @@ -97,8 +97,7 @@ pub enum ClientApiError { // pub enum GeneralErrorType { // General, // } -// Re-export LeaderHint from proto (network layer use) -pub use d_engine_proto::common::LeaderHint; +pub use crate::client::types::LeaderHint; impl From for ClientApiError { /// Converts a tonic transport error into a ClientApiError /// diff --git a/d-engine-core/src/client/client_api_error_test.rs b/d-engine-core/src/client/client_api_error_test.rs index f51593a7..3c00f548 100644 --- a/d-engine-core/src/client/client_api_error_test.rs +++ b/d-engine-core/src/client/client_api_error_test.rs @@ -3,7 +3,7 @@ #[cfg(test)] mod client_api_error_tests { use crate::client::client_api_error::{ClientApiError, ClientApiResult}; - use d_engine_proto::error::ErrorCode; + use crate::client::types::ErrorCode; /// Test: NotLeader error construction and code mapping #[test] @@ -243,7 +243,7 @@ mod client_api_error_tests { #[cfg(test)] mod transport_error_tests { use crate::client::client_api_error::ClientApiError; - use d_engine_proto::error::ErrorCode; + use crate::client::types::ErrorCode; // `tonic::transport::Error` has no public constructor, so these tests trigger // real (but near-instant) connection failures to obtain an actual transport error, diff --git a/d-engine-core/src/client/mod.rs b/d-engine-core/src/client/mod.rs index 57532699..4205b4bf 100644 --- a/d-engine-core/src/client/mod.rs +++ b/d-engine-core/src/client/mod.rs @@ -5,9 +5,18 @@ pub mod client_api; pub mod client_api_error; +pub mod types; pub use client_api::ClientApi; pub use client_api_error::{ClientApiError, ClientApiResult}; +pub use types::{ + ClientReadRequest, ClientResponse, ClientResponsePayload, ClientWriteRequest, ErrorCode, + KvEntry, LeaderHint, ReadResults, WriteOperation, WriteResult, +}; +// ReadConsistencyPolicy lives in config but is part of the public client API surface. +pub use crate::config::ReadConsistencyPolicy; #[cfg(test)] mod client_api_error_test; +#[cfg(test)] +mod types_test; diff --git a/d-engine-core/src/client/types.rs b/d-engine-core/src/client/types.rs new file mode 100644 index 00000000..b2fe55c0 --- /dev/null +++ b/d-engine-core/src/client/types.rs @@ -0,0 +1,270 @@ +//! Native client API types — transport-agnostic, no proto dependency. +//! +//! # Design rationale +//! +//! These types form the boundary between d-engine-core and any transport layer +//! (gRPC today, potentially HTTP/QUIC in the future). By keeping them proto-free, +//! adding a new transport only requires a new adapter crate; d-engine-core itself +//! needs no changes. +//! +//! Raft-internal protocol types (AppendEntriesRequest, VoteRequest, Entry, LogId …) +//! are intentionally NOT here — they remain as proto-generated types because they +//! are d-engine's persistent log format, not a transport concern. + +use bytes::Bytes; +use serde::Deserialize; +use serde::Serialize; + +// ReadConsistencyPolicy already exists as a native type in crate::config; re-export it +// here so all client-facing types live in one module. +pub use crate::config::ReadConsistencyPolicy; + +// ─── Write request ──────────────────────────────────────────────────────────── + +/// A KV write operation submitted by a client. +/// +/// Passed from the transport layer into core via `ClientCmd::Propose`. +/// Core serializes `command` to Raft log bytes internally; the transport layer +/// never needs to touch prost or proto types. +#[derive(Debug, Clone, PartialEq)] +pub struct ClientWriteRequest { + pub client_id: u32, + pub command: Option, +} + +/// Decoded write operation — the unit submitted by a client. +/// +/// Mirrors proto `WriteCommand` in shape but carries no prost annotations. +/// Core owns the serialization to Raft log bytes (`WriteOperation → proto::WriteCommand → bytes`); +/// transport adapters work with this native type only. +#[derive(Debug, Clone, PartialEq)] +pub enum WriteOperation { + Insert { + key: Bytes, + value: Bytes, + /// `None` = no expiration. Proto encodes this as `ttl_secs = 0`. + ttl_secs: Option, + }, + Delete { + key: Bytes, + }, + CompareAndSwap { + key: Bytes, + /// `None` means the key must not exist for the swap to succeed. + expected: Option, + new_value: Bytes, + }, +} + +// ─── Read request ───────────────────────────────────────────────────────────── + +/// A KV read request submitted by a client. +#[derive(Debug, Clone, PartialEq)] +pub struct ClientReadRequest { + pub client_id: u32, + pub keys: Vec, + /// When `None`, the cluster's configured default policy applies. + pub consistency_policy: Option, +} + +// ─── Response ───────────────────────────────────────────────────────────────── + +/// Response returned to the client after a write or read. +#[derive(Debug, Clone, PartialEq)] +pub struct ClientResponse { + pub error: ErrorCode, + /// Present when `error == LeaderChanged` to help the client reconnect. + pub leader_hint: Option, + /// Suggested retry delay in milliseconds, when present. + pub retry_after_ms: Option, + /// Present on success only. + pub result: Option, +} + +/// Payload carried in a successful `ClientResponse`. +#[derive(Debug, Clone, PartialEq)] +pub enum ClientResponsePayload { + Write(WriteResult), + Read(ReadResults), +} + +/// Result of a successful write operation. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct WriteResult { + pub succeeded: bool, +} + +/// Results of a successful read operation. +#[derive(Debug, Clone, PartialEq)] +pub struct ReadResults { + pub entries: Vec, +} + +/// A single key-value pair returned in a read response. +#[derive(Debug, Clone, PartialEq)] +pub struct KvEntry { + pub key: Bytes, + pub value: Bytes, +} + +// ─── Error / hint types ─────────────────────────────────────────────────────── + +/// Leader location hint — helps the client redirect after a `LeaderChanged` error. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct LeaderHint { + pub leader_id: u32, + pub address: String, +} + +/// Client-facing error codes — transport-agnostic, mirrors proto `ErrorCode`. +/// +/// Values are intentionally kept in sync with proto so that server-layer +/// conversions (`core::ErrorCode ↔ proto::ErrorCode`) are trivial numeric casts. +/// If a new variant is added here, add it to proto's `.proto` file too. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[repr(i32)] +pub enum ErrorCode { + // Success + Success = 0, + // Network layer (1000-1999) + ConnectionTimeout = 1001, + InvalidAddress = 1002, + LeaderChanged = 1003, + JoinError = 1004, + // Protocol layer (2000-2999) + InvalidResponse = 2001, + VersionMismatch = 2002, + // Storage layer (3000-3999) + DiskFull = 3001, + DataCorruption = 3002, + StorageIoError = 3003, + StoragePermissionDenied = 3004, + KeyNotExist = 3005, + // Business logic (4000-4999) + NotLeader = 4001, + StaleOperation = 4002, + InvalidRequest = 4003, + RateLimited = 4004, + ClusterUnavailable = 4005, + ProposeFailed = 4006, + TermOutdated = 4007, + RetryRequired = 4008, + // Watch layer (5000-5999) + WatchBufferOverflow = 5001, + // Unclassified + General = 8888, + Uncategorized = 9999, +} + +// ─── ErrorCode numeric conversion ───────────────────────────────────────────── + +impl TryFrom for ErrorCode { + type Error = (); + fn try_from(n: i32) -> Result { + match n { + 0 => Ok(Self::Success), + 1001 => Ok(Self::ConnectionTimeout), + 1002 => Ok(Self::InvalidAddress), + 1003 => Ok(Self::LeaderChanged), + 1004 => Ok(Self::JoinError), + 2001 => Ok(Self::InvalidResponse), + 2002 => Ok(Self::VersionMismatch), + 3001 => Ok(Self::DiskFull), + 3002 => Ok(Self::DataCorruption), + 3003 => Ok(Self::StorageIoError), + 3004 => Ok(Self::StoragePermissionDenied), + 3005 => Ok(Self::KeyNotExist), + 4001 => Ok(Self::NotLeader), + 4002 => Ok(Self::StaleOperation), + 4003 => Ok(Self::InvalidRequest), + 4004 => Ok(Self::RateLimited), + 4005 => Ok(Self::ClusterUnavailable), + 4006 => Ok(Self::ProposeFailed), + 4007 => Ok(Self::TermOutdated), + 4008 => Ok(Self::RetryRequired), + 5001 => Ok(Self::WatchBufferOverflow), + 8888 => Ok(Self::General), + 9999 => Ok(Self::Uncategorized), + _ => Err(()), + } + } +} + +// ─── ClientResponse constructors ───────────────────────────────────────────── + +impl ClientResponse { + #[inline] + pub fn write_success() -> Self { + Self { + error: ErrorCode::Success, + leader_hint: None, + retry_after_ms: None, + result: Some(ClientResponsePayload::Write(WriteResult { + succeeded: true, + })), + } + } + + #[inline] + pub fn cas_failure() -> Self { + Self { + error: ErrorCode::Success, + leader_hint: None, + retry_after_ms: None, + result: Some(ClientResponsePayload::Write(WriteResult { + succeeded: false, + })), + } + } + + #[inline] + pub fn read_results(entries: Vec) -> Self { + Self { + error: ErrorCode::Success, + leader_hint: None, + retry_after_ms: None, + result: Some(ClientResponsePayload::Read(ReadResults { entries })), + } + } + + #[inline] + pub fn client_error(error: ErrorCode) -> Self { + Self { + error, + leader_hint: None, + retry_after_ms: None, + result: None, + } + } + + #[inline] + pub fn not_leader(leader_hint: Option) -> Self { + Self { + error: ErrorCode::NotLeader, + leader_hint, + retry_after_ms: None, + result: None, + } + } + + #[inline] + pub fn is_write_success(&self) -> bool { + self.error == ErrorCode::Success + && matches!( + self.result, + Some(ClientResponsePayload::Write(WriteResult { + succeeded: true + })) + ) + } + + #[inline] + pub fn is_term_outdated(&self) -> bool { + self.error == ErrorCode::TermOutdated + } + + #[inline] + pub fn is_propose_failure(&self) -> bool { + self.error == ErrorCode::ProposeFailed + } +} diff --git a/d-engine-core/src/client/types_test.rs b/d-engine-core/src/client/types_test.rs new file mode 100644 index 00000000..80564390 --- /dev/null +++ b/d-engine-core/src/client/types_test.rs @@ -0,0 +1,401 @@ +use bytes::Bytes; + +use super::types::*; + +// ─── ErrorCode ──────────────────────────────────────────────────────────────── + +/// Every ErrorCode variant must be reachable via an exhaustive match. +/// If a variant is added to proto without being added here, this test catches it +/// (because the proto ↔ core conversion will fail to compile). +#[test] +fn test_error_code_all_variants_are_exhaustive() { + let codes = [ + ErrorCode::Success, + ErrorCode::ConnectionTimeout, + ErrorCode::InvalidAddress, + ErrorCode::LeaderChanged, + ErrorCode::JoinError, + ErrorCode::InvalidResponse, + ErrorCode::VersionMismatch, + ErrorCode::DiskFull, + ErrorCode::DataCorruption, + ErrorCode::StorageIoError, + ErrorCode::StoragePermissionDenied, + ErrorCode::KeyNotExist, + ErrorCode::NotLeader, + ErrorCode::StaleOperation, + ErrorCode::InvalidRequest, + ErrorCode::RateLimited, + ErrorCode::ClusterUnavailable, + ErrorCode::ProposeFailed, + ErrorCode::TermOutdated, + ErrorCode::RetryRequired, + ErrorCode::WatchBufferOverflow, + ErrorCode::General, + ErrorCode::Uncategorized, + ]; + // 23 variants total — if the count changes, update this assertion and the proto .proto file + assert_eq!(codes.len(), 23); +} + +#[test] +fn test_error_code_is_copy() { + let code = ErrorCode::NotLeader; + let _copy = code; // must compile without move + let _still_valid = code; +} + +#[test] +fn test_error_code_discriminants_match_proto_wire_values() { + // Numeric values must stay in sync with proto ErrorCode for the + // server-layer numeric-cast conversion to be correct. + assert_eq!(ErrorCode::Success as i32, 0); + assert_eq!(ErrorCode::ConnectionTimeout as i32, 1001); + assert_eq!(ErrorCode::NotLeader as i32, 4001); + assert_eq!(ErrorCode::WatchBufferOverflow as i32, 5001); + assert_eq!(ErrorCode::General as i32, 8888); + assert_eq!(ErrorCode::Uncategorized as i32, 9999); +} + +// ─── WriteOperation ─────────────────────────────────────────────────────────── + +#[test] +fn test_write_operation_insert_carries_correct_fields() { + let op = WriteOperation::Insert { + key: Bytes::from("k"), + value: Bytes::from("v"), + ttl_secs: Some(30), + }; + match op { + WriteOperation::Insert { + key, + value, + ttl_secs, + } => { + assert_eq!(key, Bytes::from("k")); + assert_eq!(value, Bytes::from("v")); + assert_eq!(ttl_secs, Some(30)); + } + _ => panic!("wrong variant"), + } +} + +#[test] +fn test_write_operation_insert_no_ttl_is_none() { + let op = WriteOperation::Insert { + key: Bytes::from("k"), + value: Bytes::from("v"), + ttl_secs: None, + }; + match op { + WriteOperation::Insert { ttl_secs: None, .. } => {} + _ => panic!("expected None ttl"), + } +} + +#[test] +fn test_write_operation_delete_carries_key() { + let op = WriteOperation::Delete { + key: Bytes::from("del-key"), + }; + match op { + WriteOperation::Delete { key } => assert_eq!(key, Bytes::from("del-key")), + _ => panic!("wrong variant"), + } +} + +#[test] +fn test_write_operation_cas_with_expected_value() { + let op = WriteOperation::CompareAndSwap { + key: Bytes::from("k"), + expected: Some(Bytes::from("old")), + new_value: Bytes::from("new"), + }; + match op { + WriteOperation::CompareAndSwap { + key, + expected, + new_value, + } => { + assert_eq!(key, Bytes::from("k")); + assert_eq!(expected, Some(Bytes::from("old"))); + assert_eq!(new_value, Bytes::from("new")); + } + _ => panic!("wrong variant"), + } +} + +/// CAS where expected=None means "key must not exist" — a distinct semantic from +/// "any value is accepted". This test encodes that contract. +#[test] +fn test_write_operation_cas_key_must_not_exist_when_expected_is_none() { + let op = WriteOperation::CompareAndSwap { + key: Bytes::from("k"), + expected: None, + new_value: Bytes::from("new"), + }; + match op { + WriteOperation::CompareAndSwap { expected: None, .. } => {} + _ => panic!("expected None should mean key-must-not-exist"), + } +} + +// ─── ClientWriteRequest ─────────────────────────────────────────────────────── + +#[test] +fn test_client_write_request_fields() { + let req = ClientWriteRequest { + client_id: 42, + command: Some(WriteOperation::Delete { + key: Bytes::from("k"), + }), + }; + assert_eq!(req.client_id, 42); + assert!(req.command.is_some()); +} + +#[test] +fn test_client_write_request_empty_command_is_none() { + let req = ClientWriteRequest { + client_id: 1, + command: None, + }; + assert!(req.command.is_none()); +} + +// ─── ClientReadRequest ──────────────────────────────────────────────────────── + +#[test] +fn test_client_read_request_consistency_defaults_to_none() { + let req = ClientReadRequest { + client_id: 1, + keys: vec![Bytes::from("k")], + consistency_policy: None, + }; + // None means "use cluster default" — callers must not silently assume LeaseRead + assert!(req.consistency_policy.is_none()); +} + +/// ReadConsistencyPolicy is defined in config and re-exported here. +/// This test ensures all three variants are reachable and the type is Copy-compatible. +#[test] +fn test_read_consistency_policy_all_variants_reachable() { + let policies = [ + ReadConsistencyPolicy::LeaseRead, + ReadConsistencyPolicy::LinearizableRead, + ReadConsistencyPolicy::EventualConsistency, + ]; + assert_eq!(policies.len(), 3); +} + +// ─── ClientResponse ─────────────────────────────────────────────────────────── + +#[test] +fn test_client_response_successful_write() { + let resp = ClientResponse { + error: ErrorCode::Success, + leader_hint: None, + retry_after_ms: None, + result: Some(ClientResponsePayload::Write(WriteResult { + succeeded: true, + })), + }; + assert_eq!(resp.error, ErrorCode::Success); + assert!(matches!( + resp.result, + Some(ClientResponsePayload::Write(WriteResult { + succeeded: true + })) + )); +} + +#[test] +fn test_client_response_successful_read() { + let resp = ClientResponse { + error: ErrorCode::Success, + leader_hint: None, + retry_after_ms: None, + result: Some(ClientResponsePayload::Read(ReadResults { + entries: vec![KvEntry { + key: Bytes::from("k"), + value: Bytes::from("v"), + }], + })), + }; + match resp.result { + Some(ClientResponsePayload::Read(r)) => { + assert_eq!(r.entries.len(), 1); + assert_eq!(r.entries[0].key, Bytes::from("k")); + } + _ => panic!("expected Read payload"), + } +} + +#[test] +fn test_client_response_not_leader_carries_hint() { + let resp = ClientResponse { + error: ErrorCode::NotLeader, + leader_hint: Some(LeaderHint { + leader_id: 2, + address: "127.0.0.1:5002".into(), + }), + retry_after_ms: Some(100), + result: None, + }; + assert_eq!(resp.error, ErrorCode::NotLeader); + let hint = resp.leader_hint.unwrap(); + assert_eq!(hint.leader_id, 2); + assert_eq!(hint.address, "127.0.0.1:5002"); + assert_eq!(resp.retry_after_ms, Some(100)); + assert!(resp.result.is_none()); +} + +// ─── LeaderHint ─────────────────────────────────────────────────────────────── + +#[test] +fn test_leader_hint_fields() { + let hint = LeaderHint { + leader_id: 3, + address: "10.0.0.1:5003".into(), + }; + assert_eq!(hint.leader_id, 3); + assert_eq!(hint.address, "10.0.0.1:5003"); +} + +// ─── ClientResponse constructors ───────────────────────────────────────────── + +#[test] +fn test_write_success_constructor() { + let resp = ClientResponse::write_success(); + assert_eq!(resp.error, ErrorCode::Success); + assert!(resp.leader_hint.is_none()); + assert!(resp.retry_after_ms.is_none()); + assert!(matches!( + resp.result, + Some(ClientResponsePayload::Write(WriteResult { + succeeded: true + })) + )); +} + +#[test] +fn test_cas_failure_constructor() { + let resp = ClientResponse::cas_failure(); + assert_eq!(resp.error, ErrorCode::Success); + assert!(resp.leader_hint.is_none()); + assert!(resp.retry_after_ms.is_none()); + assert!(matches!( + resp.result, + Some(ClientResponsePayload::Write(WriteResult { + succeeded: false + })) + )); +} + +#[test] +fn test_read_results_constructor_empty() { + let resp = ClientResponse::read_results(vec![]); + assert_eq!(resp.error, ErrorCode::Success); + assert!(resp.leader_hint.is_none()); + assert!(resp.retry_after_ms.is_none()); + match resp.result { + Some(ClientResponsePayload::Read(r)) => assert!(r.entries.is_empty()), + _ => panic!("expected Read payload"), + } +} + +#[test] +fn test_read_results_constructor_with_entries() { + let entries = vec![ + KvEntry { + key: Bytes::from("k1"), + value: Bytes::from("v1"), + }, + KvEntry { + key: Bytes::from("k2"), + value: Bytes::from("v2"), + }, + ]; + let resp = ClientResponse::read_results(entries); + match resp.result { + Some(ClientResponsePayload::Read(r)) => { + assert_eq!(r.entries.len(), 2); + assert_eq!(r.entries[0].key, Bytes::from("k1")); + assert_eq!(r.entries[1].value, Bytes::from("v2")); + } + _ => panic!("expected Read payload"), + } +} + +#[test] +fn test_client_error_constructor() { + let resp = ClientResponse::client_error(ErrorCode::NotLeader); + assert_eq!(resp.error, ErrorCode::NotLeader); + assert!(resp.leader_hint.is_none()); + assert!(resp.retry_after_ms.is_none()); + assert!(resp.result.is_none()); +} + +#[test] +fn test_not_leader_with_hint() { + let hint = LeaderHint { + leader_id: 2, + address: "10.0.0.2:5002".into(), + }; + let resp = ClientResponse::not_leader(Some(hint)); + assert_eq!(resp.error, ErrorCode::NotLeader); + let h = resp.leader_hint.unwrap(); + assert_eq!(h.leader_id, 2); + assert_eq!(h.address, "10.0.0.2:5002"); + assert!(resp.result.is_none()); +} + +#[test] +fn test_not_leader_without_hint() { + let resp = ClientResponse::not_leader(None); + assert_eq!(resp.error, ErrorCode::NotLeader); + assert!(resp.leader_hint.is_none()); + assert!(resp.result.is_none()); +} + +// ─── ClientResponse predicates ──────────────────────────────────────────────── + +#[test] +fn test_is_write_success_true_for_write_success() { + assert!(ClientResponse::write_success().is_write_success()); +} + +#[test] +fn test_is_write_success_false_for_cas_failure() { + assert!(!ClientResponse::cas_failure().is_write_success()); +} + +#[test] +fn test_is_write_success_false_for_error_response() { + assert!(!ClientResponse::client_error(ErrorCode::NotLeader).is_write_success()); +} + +#[test] +fn test_is_write_success_false_for_read_response() { + assert!(!ClientResponse::read_results(vec![]).is_write_success()); +} + +#[test] +fn test_is_term_outdated_true() { + assert!(ClientResponse::client_error(ErrorCode::TermOutdated).is_term_outdated()); +} + +#[test] +fn test_is_term_outdated_false_for_success() { + assert!(!ClientResponse::write_success().is_term_outdated()); +} + +#[test] +fn test_is_propose_failure_true() { + assert!(ClientResponse::client_error(ErrorCode::ProposeFailed).is_propose_failure()); +} + +#[test] +fn test_is_propose_failure_false_for_success() { + assert!(!ClientResponse::write_success().is_propose_failure()); +} diff --git a/d-engine-core/src/event.rs b/d-engine-core/src/event.rs index 3e384164..24c0f2d3 100644 --- a/d-engine-core/src/event.rs +++ b/d-engine-core/src/event.rs @@ -1,8 +1,6 @@ use std::path::PathBuf; -use d_engine_proto::client::ClientReadRequest; -use d_engine_proto::client::ClientResponse; -use d_engine_proto::client::ClientWriteRequest; +use crate::client::{ClientReadRequest, ClientResponse, ClientWriteRequest}; use d_engine_proto::common::LogId; use d_engine_proto::server::cluster::ClusterConfChangeRequest; use d_engine_proto::server::cluster::ClusterConfUpdateResponse; diff --git a/d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs b/d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs index c7425b34..5278c439 100644 --- a/d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs +++ b/d-engine-core/src/raft_role/buffers/propose_batch_buffer.rs @@ -23,7 +23,7 @@ use rand::distr::SampleString; use std::sync::Arc; -use d_engine_proto::client::ClientResponse; +use crate::client::ClientResponse; use d_engine_proto::common::EntryPayload; use tokio::time::Instant; use tonic::Status; diff --git a/d-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rs b/d-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rs index 2e31ce49..7ea5fc0d 100644 --- a/d-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rs +++ b/d-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rs @@ -1,4 +1,4 @@ -use d_engine_proto::client::ClientResponse; +use crate::client::ClientResponse; use d_engine_proto::common::EntryPayload; use tonic::Status; diff --git a/d-engine-core/src/raft_role/candidate_state_test.rs b/d-engine-core/src/raft_role/candidate_state_test.rs index fac6637c..a0dcc1eb 100644 --- a/d-engine-core/src/raft_role/candidate_state_test.rs +++ b/d-engine-core/src/raft_role/candidate_state_test.rs @@ -1,11 +1,11 @@ use std::sync::Arc; -use d_engine_proto::client::ClientReadRequest; -use d_engine_proto::client::ClientWriteRequest; -use d_engine_proto::client::ReadConsistencyPolicy; -use d_engine_proto::client::WriteCommand; +use crate::client::ClientReadRequest; +use crate::client::ClientWriteRequest; +use crate::client::ErrorCode; +use crate::client::WriteOperation; +use crate::config::ReadConsistencyPolicy; use d_engine_proto::common::LogId; -use d_engine_proto::error::ErrorCode; use d_engine_proto::server::cluster::ClusterConfChangeRequest; use d_engine_proto::server::cluster::ClusterConfUpdateResponse; use d_engine_proto::server::cluster::ClusterMembership; @@ -849,7 +849,9 @@ async fn test_handle_client_write_returns_not_leader() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 1, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }, resp_tx, ); @@ -1072,7 +1074,7 @@ mod handle_client_read_request { let mut state = CandidateState::::new(1, context.node_config.clone()); let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![], }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); @@ -1108,7 +1110,7 @@ mod handle_client_read_request { let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency as i32), + consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency), keys: vec![], }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); @@ -1117,7 +1119,7 @@ mod handle_client_read_request { state.push_client_cmd(cmd, &context); let response = resp_rx.recv().await.unwrap().unwrap(); - assert_eq!(response.error, ErrorCode::Success as i32); + assert_eq!(response.error, ErrorCode::Success); } } @@ -1248,18 +1250,13 @@ async fn test_new_leader_initializes_empty_buffers() { // Send first write request to new Leader if let crate::RaftRole::Leader(ref mut leader) = raft.role { let (response_tx, _response_rx) = crate::MaybeCloneOneshot::new(); - let write_cmd = d_engine_proto::client::WriteCommand { - operation: Some(d_engine_proto::client::write_command::Operation::Insert( - d_engine_proto::client::write_command::Insert { - key: bytes::Bytes::from("first_key"), - value: bytes::Bytes::from("first_value"), - ttl_secs: 0, - }, - )), - }; - let write_req = d_engine_proto::client::ClientWriteRequest { + let write_req = ClientWriteRequest { client_id: 1, - command: Some(write_cmd), + command: Some(WriteOperation::Insert { + key: bytes::Bytes::from("first_key"), + value: bytes::Bytes::from("first_value"), + ttl_secs: None, + }), }; let cmd = crate::ClientCmd::Propose(write_req, response_tx); diff --git a/d-engine-core/src/raft_role/follower_state_test.rs b/d-engine-core/src/raft_role/follower_state_test.rs index f1ac340f..41d686b9 100644 --- a/d-engine-core/src/raft_role/follower_state_test.rs +++ b/d-engine-core/src/raft_role/follower_state_test.rs @@ -1,11 +1,11 @@ +use crate::client::ClientReadRequest; +use crate::client::ClientWriteRequest; +use crate::client::ErrorCode; +use crate::client::WriteOperation; +use crate::config::ReadConsistencyPolicy; use crate::test_utils::create_test_snapshot_stream; -use d_engine_proto::client::ClientReadRequest; -use d_engine_proto::client::ClientWriteRequest; -use d_engine_proto::client::ReadConsistencyPolicy; -use d_engine_proto::client::WriteCommand; use d_engine_proto::common::LogId; use d_engine_proto::common::NodeRole; -use d_engine_proto::error::ErrorCode; use d_engine_proto::server::cluster::ClusterConfChangeRequest; use d_engine_proto::server::cluster::ClusterConfUpdateResponse; use d_engine_proto::server::cluster::ClusterMembership; @@ -1106,7 +1106,9 @@ async fn test_handle_client_write_request_redirects_to_leader() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 1, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }, resp_tx, ); @@ -1173,7 +1175,7 @@ async fn test_handle_client_read_request_linearizable_redirects_to_leader() { let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![], }; @@ -1226,7 +1228,7 @@ async fn test_handle_client_read_request_eventual_consistency_succeeds() { let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency as i32), + consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency), keys: vec![], }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); @@ -1239,7 +1241,7 @@ async fn test_handle_client_read_request_eventual_consistency_succeeds() { let response = resp_rx.recv().await.unwrap().unwrap(); assert_eq!( response.error, - ErrorCode::Success as i32, + ErrorCode::Success, "EventualConsistency read should succeed on follower" ); } @@ -1761,8 +1763,8 @@ mod handle_client_read_request { use super::*; use crate::RaftNodeConfig; use crate::config::ReadConsistencyPolicy as ServerPolicy; + use crate::config::ReadConsistencyPolicy as ClientPolicy; use crate::convert::safe_kv_bytes; - use d_engine_proto::client::ReadConsistencyPolicy as ClientPolicy; /// Test: Follower rejects LeaseRead policy /// @@ -1791,7 +1793,7 @@ mod handle_client_read_request { let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ClientPolicy::LeaseRead as i32), + consistency_policy: Some(ClientPolicy::LeaseRead), keys: vec![safe_kv_bytes(1)], }; @@ -1899,7 +1901,7 @@ mod handle_client_read_request { let response = resp_rx.recv().await.unwrap().unwrap(); assert_eq!( response.error, - ErrorCode::Success as i32, + ErrorCode::Success, "EventualConsistency read should succeed on follower" ); } @@ -1937,7 +1939,7 @@ mod handle_client_read_request { // Client requests LinearizableRead but server will ignore it let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ClientPolicy::LinearizableRead as i32), + consistency_policy: Some(ClientPolicy::LinearizableRead), keys: vec![safe_kv_bytes(1)], }; @@ -1950,7 +1952,7 @@ mod handle_client_read_request { let response = resp_rx.recv().await.unwrap().unwrap(); assert_eq!( response.error, - ErrorCode::Success as i32, + ErrorCode::Success, "Follower should serve read using server default EventualConsistency, ignoring client LinearizableRead" ); } @@ -1988,7 +1990,7 @@ mod handle_client_read_request { // Client requests EventualConsistency but server will ignore it let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ClientPolicy::EventualConsistency as i32), + consistency_policy: Some(ClientPolicy::EventualConsistency), keys: vec![safe_kv_bytes(1)], }; @@ -2286,7 +2288,7 @@ async fn test_follower_rejects_strong_consistency_reads() { let read_req = ClientReadRequest { client_id: 1, keys: vec![bytes::Bytes::from("lease_key")], - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), }; let start = tokio::time::Instant::now(); @@ -2326,7 +2328,7 @@ async fn test_follower_rejects_strong_consistency_reads() { let read_req = ClientReadRequest { client_id: 1, keys: vec![bytes::Bytes::from("linear_key")], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let start = tokio::time::Instant::now(); diff --git a/d-engine-core/src/raft_role/leader_state.rs b/d-engine-core/src/raft_role/leader_state.rs index 77a3d6ee..a2ff42a9 100644 --- a/d-engine-core/src/raft_role/leader_state.rs +++ b/d-engine-core/src/raft_role/leader_state.rs @@ -41,12 +41,11 @@ use crate::alias::MOF; use crate::alias::ROF; use crate::alias::SMHOF; use crate::alias::TROF; +use crate::client::{ClientReadRequest, ClientResponse, ErrorCode}; use crate::event::ClientCmd; use crate::network::Transport; use crate::stream::create_production_snapshot_stream; use async_trait::async_trait; -use d_engine_proto::client::ClientReadRequest; -use d_engine_proto::client::ClientResponse; use d_engine_proto::common::AddNode; use d_engine_proto::common::BatchPromote; use d_engine_proto::common::BatchRemove; @@ -55,7 +54,6 @@ use d_engine_proto::common::LogId; use d_engine_proto::common::NodeRole::Leader; use d_engine_proto::common::NodeStatus; use d_engine_proto::common::membership_change::Change; -use d_engine_proto::error::ErrorCode; use d_engine_proto::server::cluster::ClusterConfUpdateResponse; use d_engine_proto::server::cluster::JoinRequest; use d_engine_proto::server::cluster::JoinResponse; @@ -945,7 +943,7 @@ impl RaftRoleState for LeaderState { // Convert command to payload (will be merged in drain_batch) if let Some(cmd) = req.command { - let payload = client_command_to_entry_payloads(vec![cmd]) + let payload = client_command_to_entry_payloads(vec![write_op_to_proto(cmd)]) .into_iter() .next() .expect("client_command_to_entry_payloads should return 1 element"); @@ -3862,12 +3860,12 @@ impl LeaderState { &self, req: &ClientReadRequest, ) -> ServerReadConsistencyPolicy { - use d_engine_proto::client::ReadConsistencyPolicy as ClientReadConsistencyPolicy; + use crate::config::ReadConsistencyPolicy as ClientReadConsistencyPolicy; - if req.has_consistency_policy() { + if let Some(policy) = &req.consistency_policy { // Client explicitly specified policy if self.node_config.raft.read_consistency.allow_client_override { - match req.consistency_policy() { + match policy { ClientReadConsistencyPolicy::LeaseRead => { ServerReadConsistencyPolicy::LeaseRead } @@ -4063,3 +4061,42 @@ pub fn calculate_safe_batch_size( available.saturating_sub(1) } } + +// Serialize a native WriteOperation to a proto WriteCommand for Raft log encoding. +// Proto bytes in the log must remain stable; this conversion is the only place +// that touches prost types on the write path. +#[inline] +pub(crate) fn write_op_to_proto( + op: crate::client::WriteOperation +) -> d_engine_proto::client::WriteCommand { + use crate::client::WriteOperation; + use d_engine_proto::client::WriteCommand; + use d_engine_proto::client::write_command::{CompareAndSwap, Delete, Insert, Operation}; + match op { + WriteOperation::Insert { + key, + value, + ttl_secs, + } => WriteCommand { + operation: Some(Operation::Insert(Insert { + key, + value, + ttl_secs: ttl_secs.unwrap_or(0), + })), + }, + WriteOperation::Delete { key } => WriteCommand { + operation: Some(Operation::Delete(Delete { key })), + }, + WriteOperation::CompareAndSwap { + key, + expected, + new_value, + } => WriteCommand { + operation: Some(Operation::CompareAndSwap(CompareAndSwap { + key, + expected_value: expected, + new_value, + })), + }, + } +} diff --git a/d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs b/d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs index 3a84a73a..aaa205c8 100644 --- a/d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs @@ -1,7 +1,7 @@ -use d_engine_proto::client::ClientReadRequest; -use d_engine_proto::client::ClientWriteRequest; -use d_engine_proto::client::ReadConsistencyPolicy; -use d_engine_proto::client::WriteCommand; +use crate::client::ClientReadRequest; +use crate::client::ClientWriteRequest; +use crate::client::WriteOperation; +use crate::config::ReadConsistencyPolicy; use tokio::sync::watch; use crate::BackpressureConfig; @@ -40,7 +40,9 @@ async fn test_backpressure_write_limit_enforcement() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: i, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }, resp_tx, ); @@ -58,7 +60,9 @@ async fn test_backpressure_write_limit_enforcement() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 3, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }, resp_tx, ); @@ -106,7 +110,7 @@ async fn test_backpressure_read_limit_enforcement() { let cmd = ClientCmd::Read( ClientReadRequest { client_id: i, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![], }, resp_tx, @@ -125,7 +129,7 @@ async fn test_backpressure_read_limit_enforcement() { let cmd = ClientCmd::Read( ClientReadRequest { client_id: 4, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![], }, resp_tx, @@ -173,7 +177,9 @@ async fn test_backpressure_unlimited_when_zero() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: i, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }, resp_tx, ); @@ -214,7 +220,9 @@ async fn test_backpressure_write_and_read_independent() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: i, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }, resp_tx, ); @@ -226,7 +234,9 @@ async fn test_backpressure_write_and_read_independent() { let write_cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 99, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }, write_tx, ); @@ -245,7 +255,7 @@ async fn test_backpressure_write_and_read_independent() { let read_cmd = ClientCmd::Read( ClientReadRequest { client_id: 99, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![], }, read_tx, @@ -286,7 +296,7 @@ async fn test_backpressure_all_read_policies() { let cmd = ClientCmd::Read( ClientReadRequest { client_id: i, - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), keys: vec![], }, resp_tx, @@ -301,7 +311,7 @@ async fn test_backpressure_all_read_policies() { let cmd = ClientCmd::Read( ClientReadRequest { client_id: 3, - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), keys: vec![], }, resp_tx, @@ -325,7 +335,7 @@ async fn test_backpressure_all_read_policies() { let cmd = ClientCmd::Read( ClientReadRequest { client_id: i, - consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency as i32), + consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency), keys: vec![], }, resp_tx, @@ -340,7 +350,7 @@ async fn test_backpressure_all_read_policies() { let cmd = ClientCmd::Read( ClientReadRequest { client_id: 3, - consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency as i32), + consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency), keys: vec![], }, resp_tx, diff --git a/d-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rs b/d-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rs index 4294c533..67658549 100644 --- a/d-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/buffer_cleanup_test.rs @@ -4,9 +4,9 @@ use crate::MockBuilder; use crate::RaftOneshot; use crate::RaftRole; use crate::RoleEvent; +use crate::client::ClientWriteRequest; +use crate::client::WriteOperation; use crate::raft_role::role_state::RaftRoleState; -use d_engine_proto::client::ClientWriteRequest; -use d_engine_proto::client::WriteCommand; use std::sync::Arc; use tokio::sync::watch; @@ -68,18 +68,13 @@ async fn test_leader_stepdown_clears_pending_write_buffer() { let (response_tx, response_rx) = MaybeCloneOneshot::new(); response_receivers.push(response_rx); - let write_cmd = WriteCommand { - operation: Some(d_engine_proto::client::write_command::Operation::Insert( - d_engine_proto::client::write_command::Insert { - key: bytes::Bytes::from(format!("key_{i}")), - value: bytes::Bytes::from(format!("value_{i}")), - ttl_secs: 0, - }, - )), - }; let write_req = ClientWriteRequest { client_id: 1, - command: Some(write_cmd), + command: Some(WriteOperation::Insert { + key: bytes::Bytes::from(format!("key_{i}")), + value: bytes::Bytes::from(format!("value_{i}")), + ttl_secs: None, + }), }; let cmd = ClientCmd::Propose(write_req, response_tx); @@ -154,18 +149,13 @@ async fn test_leader_stepdown_clears_pending_write_buffer() { // Verify: New Follower rejects subsequent writes if let RaftRole::Follower(ref mut follower) = raft.role { let (response_tx, mut response_rx) = MaybeCloneOneshot::new(); - let write_cmd = WriteCommand { - operation: Some(d_engine_proto::client::write_command::Operation::Insert( - d_engine_proto::client::write_command::Insert { - key: bytes::Bytes::from("new_key"), - value: bytes::Bytes::from("new_value"), - ttl_secs: 0, - }, - )), - }; let write_req = ClientWriteRequest { client_id: 1, - command: Some(write_cmd), + command: Some(WriteOperation::Insert { + key: bytes::Bytes::from("new_key"), + value: bytes::Bytes::from("new_value"), + ttl_secs: None, + }), }; let cmd = ClientCmd::Propose(write_req, response_tx); diff --git a/d-engine-core/src/raft_role/leader_state_test/client_read_test.rs b/d-engine-core/src/raft_role/leader_state_test/client_read_test.rs index f4c0a1f5..67d972d1 100644 --- a/d-engine-core/src/raft_role/leader_state_test/client_read_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/client_read_test.rs @@ -2,6 +2,7 @@ use crate::ClientCmd; use crate::MockMembership; use crate::MockStateMachineHandler; use crate::ReadConsistencyPolicy; +use crate::client::{ClientReadRequest, ErrorCode}; use crate::convert::safe_kv_bytes; use crate::maybe_clone_oneshot::MaybeCloneOneshot; use crate::maybe_clone_oneshot::RaftOneshot; @@ -12,10 +13,8 @@ use crate::test_utils::mock::MockTypeConfig; use crate::test_utils::node_config; use crate::{Error, MockRaftLog, MockReplicationCore, RaftNodeConfig}; use bytes::Bytes; -use d_engine_proto::client::ClientReadRequest; use d_engine_proto::common::NodeRole::Follower; use d_engine_proto::common::NodeStatus; -use d_engine_proto::error::ErrorCode; use d_engine_proto::server::cluster::NodeMeta; use std::sync::Arc; use std::time::Duration; @@ -428,7 +427,7 @@ async fn test_linearizable_read_quorum_failure() { let client_read_request = ClientReadRequest { client_id: 1, keys, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); let cmd = ClientCmd::Read(client_read_request, resp_tx); @@ -529,7 +528,7 @@ async fn test_linearizable_read_quorum_success() { let keys = vec![safe_kv_bytes(1)]; let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys, }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); @@ -625,7 +624,7 @@ async fn test_linearizable_read_encounters_higher_term() { let keys = vec![safe_kv_bytes(1)]; let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys, }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); @@ -708,7 +707,7 @@ async fn test_lease_read_with_valid_lease() { let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), keys: vec![safe_kv_bytes(1)], }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); @@ -719,7 +718,7 @@ async fn test_lease_read_with_valid_lease() { state.flush_cmd_buffers(&context, &role_tx).await.expect("should succeed"); let response = resp_rx.recv().await.unwrap().unwrap(); - assert_eq!(response.error, ErrorCode::Success as i32); + assert_eq!(response.error, ErrorCode::Success); } /// Test expired lease detection @@ -804,7 +803,7 @@ async fn test_expired_lease_single_voter_refreshed_immediately() { let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), keys: vec![safe_kv_bytes(1)], }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); @@ -914,7 +913,7 @@ async fn test_unspecified_policy_defaults_to_linearizable_read() { .expect("ApplyCompleted should succeed"); let response = resp_rx.recv().await.unwrap().unwrap(); - assert_eq!(response.error, ErrorCode::Success as i32); + assert_eq!(response.error, ErrorCode::Success); } /// Test EventualConsistency policy serves reads immediately @@ -962,7 +961,7 @@ async fn test_eventual_consistency_serves_immediately() { let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency as i32), + consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency), keys: vec![safe_kv_bytes(1)], }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); @@ -973,7 +972,7 @@ async fn test_eventual_consistency_serves_immediately() { state.flush_cmd_buffers(&context, &role_tx).await.expect("should succeed"); let response = resp_rx.recv().await.unwrap().unwrap(); - assert_eq!(response.error, ErrorCode::Success as i32); + assert_eq!(response.error, ErrorCode::Success); // Should succeed immediately without any verification } @@ -1022,7 +1021,7 @@ async fn test_server_default_overrides_client_policy() { let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), // Should be ignored + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), // Should be ignored keys: vec![safe_kv_bytes(1)], }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); @@ -1033,7 +1032,7 @@ async fn test_server_default_overrides_client_policy() { state.flush_cmd_buffers(&context, &role_tx).await.expect("should succeed"); let response = resp_rx.recv().await.unwrap().unwrap(); - assert_eq!(response.error, ErrorCode::Success as i32); + assert_eq!(response.error, ErrorCode::Success); // Should use server default (EventualConsistency) and succeed immediately } @@ -1093,7 +1092,7 @@ async fn test_linearizable_read_batch_shared_quorum() { let req1 = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key1")], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (tx1, mut rx1) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req1, tx1), &ctx); @@ -1101,7 +1100,7 @@ async fn test_linearizable_read_batch_shared_quorum() { let req2 = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key2")], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (tx2, mut rx2) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req2, tx2), &ctx); @@ -1109,7 +1108,7 @@ async fn test_linearizable_read_batch_shared_quorum() { let req3 = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key3")], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (tx3, mut rx3) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req3, tx3), &ctx); @@ -1199,7 +1198,7 @@ async fn test_lease_reuse_after_linearizable_read_refresh() { let req1 = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key1")], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (tx1, mut rx1) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req1, tx1), &ctx); @@ -1222,7 +1221,7 @@ async fn test_lease_reuse_after_linearizable_read_refresh() { let req2 = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key2")], - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), }; let (tx2, mut rx2) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req2, tx2), &ctx); @@ -1293,7 +1292,7 @@ async fn test_eventual_consistency_ignores_stale_lease() { let req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key1")], - consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency as i32), + consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency), }; let (tx, mut rx) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req, tx), &ctx); @@ -1361,7 +1360,7 @@ async fn test_client_policy_override_allowed() { let req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key1")], - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), }; let (tx, mut rx) = MaybeCloneOneshot::new(); @@ -1437,7 +1436,7 @@ async fn test_client_policy_override_denied() { let req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key1")], - consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency as i32), + consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency), }; let (tx, mut rx) = MaybeCloneOneshot::new(); @@ -1521,7 +1520,7 @@ async fn test_drain_single_request_no_delay() { let req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key1")], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (tx, mut rx) = MaybeCloneOneshot::new(); @@ -1602,7 +1601,7 @@ async fn test_drain_multiple_requests_natural_batch() { let req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from(format!("key{i}"))], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (tx, rx) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req, tx), &ctx); @@ -1682,7 +1681,7 @@ async fn test_drain_max_batch_size_limit() { let req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from(format!("key{i}"))], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (tx, _rx) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req, tx), &ctx); @@ -1776,7 +1775,7 @@ async fn test_linearizable_read_batch_single_quorum() { let req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from(format!("key{i}"))], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (tx, rx) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req, tx), &ctx); @@ -1845,7 +1844,7 @@ async fn test_lease_read_valid_lease_serves_immediately() { let req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key1")], - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), }; let (tx, mut rx) = MaybeCloneOneshot::new(); @@ -1916,7 +1915,7 @@ async fn test_linearizable_read_rejected_when_noop_not_committed() { ClientReadRequest { client_id: 1, keys: vec![safe_kv_bytes(1)], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }, resp_tx, ); @@ -2022,7 +2021,7 @@ async fn test_lease_read_empty_payload_verification_hangs_in_multi_node() { let req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"key1")], - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), }; let (tx, _rx) = MaybeCloneOneshot::new(); @@ -2169,7 +2168,7 @@ async fn test_linearizable_read_served_without_quorum_in_minority_partition() { let req = ClientReadRequest { client_id: 1, keys: vec![safe_kv_bytes(1)], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Read(req, resp_tx), &ctx); diff --git a/d-engine-core/src/raft_role/leader_state_test/client_write_test.rs b/d-engine-core/src/raft_role/leader_state_test/client_write_test.rs index 3dfcb839..30748a35 100644 --- a/d-engine-core/src/raft_role/leader_state_test/client_write_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/client_write_test.rs @@ -8,6 +8,7 @@ use crate::ClientCmd; use crate::MockRaftLog; use crate::MockReplicationCore; use crate::RaftRequestWithSignal; +use crate::client::{ClientWriteRequest, WriteOperation}; use crate::client_command_to_entry_payloads; use crate::event::{NewCommitData, RoleEvent}; use crate::maybe_clone_oneshot::MaybeCloneOneshot; @@ -19,13 +20,10 @@ use crate::test_utils::MockBuilder; use crate::test_utils::mock::MockTypeConfig; use crate::test_utils::node_config; use bytes::Bytes; -use d_engine_proto::client::ClientWriteRequest; -use d_engine_proto::client::WriteCommand; use d_engine_proto::common::AddNode; use d_engine_proto::common::EntryPayload; use d_engine_proto::common::NodeStatus; use d_engine_proto::common::membership_change::Change; -use d_engine_proto::error::ErrorCode; use rand::distr::SampleString; use std::collections::VecDeque; use std::sync::Arc; @@ -50,13 +48,13 @@ struct ProcessRaftRequestTestContext { /// Verify client response succeeds async fn assert_client_response( mut rx: crate::MaybeCloneOneshotReceiver< - std::result::Result, + std::result::Result, > ) { match rx.recv().await { Ok(Ok(response)) => assert_eq!( - ErrorCode::try_from(response.error).unwrap(), - ErrorCode::Success, + response.error, + crate::client::ErrorCode::Success, "Expected success response" ), Ok(Err(e)) => panic!("Unexpected error response: {e:?}"), @@ -144,7 +142,9 @@ async fn test_process_raft_request_immediate_execution() { // Prepare test request let request = ClientWriteRequest { client_id: 0, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }; use crate::maybe_clone_oneshot::MaybeCloneOneshot; let (tx, rx) = >::new(); @@ -156,7 +156,13 @@ async fn test_process_raft_request_immediate_execution() { .execute_request_immediately( RaftRequestWithSignal { id: rand::distr::Alphanumeric.sample_string(&mut rand::rng(), 21), - payloads: client_command_to_entry_payloads(request.command.into_iter().collect()), + payloads: client_command_to_entry_payloads( + request + .command + .into_iter() + .map(crate::raft_role::leader_state::write_op_to_proto) + .collect(), + ), senders: vec![tx], wait_for_apply_event: true, }, @@ -367,7 +373,9 @@ async fn test_process_raft_request_two_consecutive_forced_sends() { // Prepare test requests let request = ClientWriteRequest { client_id: 0, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }; use crate::maybe_clone_oneshot::MaybeCloneOneshot; let (tx1, rx1) = >::new(); @@ -381,7 +389,12 @@ async fn test_process_raft_request_two_consecutive_forced_sends() { RaftRequestWithSignal { id: rand::distr::Alphanumeric.sample_string(&mut rand::rng(), 21), payloads: client_command_to_entry_payloads( - request.command.clone().into_iter().collect(), + request + .command + .clone() + .into_iter() + .map(crate::raft_role::leader_state::write_op_to_proto) + .collect(), ), senders: vec![tx1], wait_for_apply_event: true, @@ -397,7 +410,13 @@ async fn test_process_raft_request_two_consecutive_forced_sends() { .execute_request_immediately( RaftRequestWithSignal { id: rand::distr::Alphanumeric.sample_string(&mut rand::rng(), 21), - payloads: client_command_to_entry_payloads(request.command.into_iter().collect()), + payloads: client_command_to_entry_payloads( + request + .command + .into_iter() + .map(crate::raft_role::leader_state::write_op_to_proto) + .collect(), + ), senders: vec![tx2], wait_for_apply_event: true, }, @@ -486,7 +505,9 @@ async fn test_process_raft_request_batching_enabled() { // Prepare test request let request = ClientWriteRequest { client_id: 0, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }; use crate::ClientCmd; use crate::maybe_clone_oneshot::MaybeCloneOneshot; @@ -554,7 +575,9 @@ async fn test_drain_single_write_no_delay() { let start = Instant::now(); let req = ClientWriteRequest { client_id: 1, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }; let (tx, mut rx) = MaybeCloneOneshot::new(); @@ -648,7 +671,9 @@ async fn test_drain_multiple_writes_natural_batch() { for i in 0..10 { let req = ClientWriteRequest { client_id: i, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }; let (tx, rx) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Propose(req, tx), &ctx); @@ -731,7 +756,9 @@ async fn test_drain_max_batch_size_limit() { for i in 0..10 { let req = ClientWriteRequest { client_id: i, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }; let (tx, _rx) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Propose(req, tx), &ctx); @@ -817,7 +844,9 @@ async fn test_write_batch_single_replication() { for i in 0..5 { let req = ClientWriteRequest { client_id: i, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }; let (tx, rx) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Propose(req, tx), &ctx); @@ -856,6 +885,111 @@ async fn test_write_batch_single_replication() { drop(_shutdown_tx); } +// ============================================================================ +// write_op_to_proto conversion tests +// ============================================================================ + +/// Verify Insert with TTL converts correctly to proto WriteCommand. +#[test] +fn test_write_op_to_proto_insert_with_ttl() { + use crate::client::WriteOperation; + use crate::raft_role::leader_state::write_op_to_proto; + use d_engine_proto::client::write_command::Operation; + + let op = WriteOperation::Insert { + key: Bytes::from("k"), + value: Bytes::from("v"), + ttl_secs: Some(60), + }; + let cmd = write_op_to_proto(op); + match cmd.operation { + Some(Operation::Insert(ins)) => { + assert_eq!(ins.key, Bytes::from("k")); + assert_eq!(ins.value, Bytes::from("v")); + assert_eq!(ins.ttl_secs, 60); + } + other => panic!("expected Insert, got {:?}", other), + } +} + +/// Verify Insert with no TTL maps ttl_secs=None → proto ttl_secs=0. +#[test] +fn test_write_op_to_proto_insert_no_ttl_maps_to_zero() { + use crate::client::WriteOperation; + use crate::raft_role::leader_state::write_op_to_proto; + use d_engine_proto::client::write_command::Operation; + + let op = WriteOperation::Insert { + key: Bytes::from("k"), + value: Bytes::from("v"), + ttl_secs: None, + }; + let cmd = write_op_to_proto(op); + match cmd.operation { + Some(Operation::Insert(ins)) => assert_eq!(ins.ttl_secs, 0), + other => panic!("expected Insert, got {:?}", other), + } +} + +/// Verify Delete converts correctly to proto WriteCommand. +#[test] +fn test_write_op_to_proto_delete() { + use crate::client::WriteOperation; + use crate::raft_role::leader_state::write_op_to_proto; + use d_engine_proto::client::write_command::Operation; + + let op = WriteOperation::Delete { + key: Bytes::from("del-key"), + }; + let cmd = write_op_to_proto(op); + match cmd.operation { + Some(Operation::Delete(del)) => assert_eq!(del.key, Bytes::from("del-key")), + other => panic!("expected Delete, got {:?}", other), + } +} + +/// Verify CAS with expected value converts correctly. +#[test] +fn test_write_op_to_proto_cas_with_expected() { + use crate::client::WriteOperation; + use crate::raft_role::leader_state::write_op_to_proto; + use d_engine_proto::client::write_command::Operation; + + let op = WriteOperation::CompareAndSwap { + key: Bytes::from("k"), + expected: Some(Bytes::from("old")), + new_value: Bytes::from("new"), + }; + let cmd = write_op_to_proto(op); + match cmd.operation { + Some(Operation::CompareAndSwap(cas)) => { + assert_eq!(cas.key, Bytes::from("k")); + assert_eq!(cas.expected_value, Some(Bytes::from("old"))); + assert_eq!(cas.new_value, Bytes::from("new")); + } + other => panic!("expected CompareAndSwap, got {:?}", other), + } +} + +/// CAS with expected=None maps to expected_value=None (key-must-not-exist semantics). +#[test] +fn test_write_op_to_proto_cas_key_must_not_exist() { + use crate::client::WriteOperation; + use crate::raft_role::leader_state::write_op_to_proto; + use d_engine_proto::client::write_command::Operation; + + let op = WriteOperation::CompareAndSwap { + key: Bytes::from("k"), + expected: None, + new_value: Bytes::from("new"), + }; + let cmd = write_op_to_proto(op); + match cmd.operation { + Some(Operation::CompareAndSwap(cas)) => assert!(cas.expected_value.is_none()), + other => panic!("expected CompareAndSwap, got {:?}", other), + } +} + // ============================================================================ // wait_for_apply_event Semantic Contract Tests // @@ -932,7 +1066,9 @@ async fn test_client_write_deferred_until_sm_apply() { let req = ClientWriteRequest { client_id: 1, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }; let (tx, mut rx) = MaybeCloneOneshot::new(); state.push_client_cmd(ClientCmd::Propose(req, tx), &ctx); @@ -1139,7 +1275,7 @@ async fn test_config_change_quorum_check_responds_immediately_without_sm_apply() fn write_request() -> ( RaftRequestWithSignal, crate::maybe_clone_oneshot::MaybeCloneOneshotReceiver< - std::result::Result, + std::result::Result, >, ) { let (tx, rx) = >::new(); @@ -1274,8 +1410,8 @@ async fn test_single_voter_client_write_completes_after_log_flushed() { ); let client_response = response.unwrap(); assert_eq!( - ErrorCode::try_from(client_response.error).unwrap(), - ErrorCode::Success, + client_response.error, + crate::client::ErrorCode::Success, "client must receive Success error code" ); } diff --git a/d-engine-core/src/raft_role/leader_state_test/commit_index_test.rs b/d-engine-core/src/raft_role/leader_state_test/commit_index_test.rs index 0c4aae22..2fce443c 100644 --- a/d-engine-core/src/raft_role/leader_state_test/commit_index_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/commit_index_test.rs @@ -43,7 +43,7 @@ fn success_response( fn write_request() -> ( RaftRequestWithSignal, crate::maybe_clone_oneshot::MaybeCloneOneshotReceiver< - std::result::Result, + std::result::Result, >, ) { let (tx, rx) = >::new(); diff --git a/d-engine-core/src/raft_role/leader_state_test/deadline_test.rs b/d-engine-core/src/raft_role/leader_state_test/deadline_test.rs index b52fd920..0440de55 100644 --- a/d-engine-core/src/raft_role/leader_state_test/deadline_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/deadline_test.rs @@ -5,6 +5,7 @@ //! `pending_commit_actions`, and leaves live entries intact. use crate::MaybeCloneOneshot; +use crate::client::ClientReadRequest; use crate::maybe_clone_oneshot::RaftOneshot; use crate::raft_role::leader_state::{ LeaderState, PendingLeaseRead, PendingReadBatch, PostCommitAction, PostCommitEntry, @@ -13,7 +14,6 @@ use crate::raft_role::leader_state::{ use crate::role_state::RaftRoleState; use crate::test_utils::MockBuilder; use crate::test_utils::mock::MockTypeConfig; -use d_engine_proto::client::ClientReadRequest; use std::collections::VecDeque; use std::time::Duration; use tokio::sync::{mpsc, watch}; @@ -24,7 +24,7 @@ use tonic::Code; // Helpers // ============================================================================ -type WriteResponse = std::result::Result; +type WriteResponse = std::result::Result; type ReadResponse = WriteResponse; /// Build a minimal context + LeaderState sufficient for tick() deadline tests. @@ -77,8 +77,9 @@ fn read_batch( ) { let (tx, rx) = MaybeCloneOneshot::new(); let req = ClientReadRequest { + client_id: 0, keys: vec![], - ..Default::default() + consistency_policy: None, }; let batch = PendingReadBatch { deadline, @@ -193,7 +194,11 @@ async fn test_tick_drains_expired_pending_lease_read() { let (tx, mut rx) = MaybeCloneOneshot::new(); state.pending_lease_reads.push_back(PendingLeaseRead { - request: ClientReadRequest::default(), + request: ClientReadRequest { + client_id: 0, + keys: vec![], + consistency_policy: None, + }, sender: tx, deadline: expired(), }); @@ -219,7 +224,11 @@ async fn test_tick_keeps_non_expired_pending_lease_read() { let (tx, mut rx) = MaybeCloneOneshot::new(); state.pending_lease_reads.push_back(PendingLeaseRead { - request: ClientReadRequest::default(), + request: ClientReadRequest { + client_id: 0, + keys: vec![], + consistency_policy: None, + }, sender: tx, deadline: far_future(), }); @@ -243,12 +252,20 @@ async fn test_tick_drains_only_expired_lease_reads_in_mixed_queue() { let (live_tx, mut live_rx) = MaybeCloneOneshot::new(); state.pending_lease_reads.push_back(PendingLeaseRead { - request: ClientReadRequest::default(), + request: ClientReadRequest { + client_id: 0, + keys: vec![], + consistency_policy: None, + }, sender: exp_tx, deadline: expired(), }); state.pending_lease_reads.push_back(PendingLeaseRead { - request: ClientReadRequest::default(), + request: ClientReadRequest { + client_id: 0, + keys: vec![], + consistency_policy: None, + }, sender: live_tx, deadline: far_future(), }); diff --git a/d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs b/d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs index f19a655e..059dd4b0 100644 --- a/d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/event_handling_test.rs @@ -3,7 +3,7 @@ //! This module tests the `handle_raft_event` method for various Raft events //! including vote requests, append entries, client operations, and more. -use d_engine_proto::client::WriteCommand; +use crate::client::WriteOperation; use std::sync::Arc; use tokio::sync::{mpsc, watch}; use tonic::Code; @@ -14,7 +14,9 @@ use crate::Error; use crate::MockRaftLog; use crate::MockReplicationCore; use crate::MockStateMachineHandler; +use crate::client::{ClientReadRequest, ClientWriteRequest}; use crate::config::RaftNodeConfig; +use crate::config::ReadConsistencyPolicy; use crate::event::RaftEvent; use crate::event::{NewCommitData, RoleEvent}; use crate::maybe_clone_oneshot::RaftOneshot; @@ -24,8 +26,6 @@ use crate::test_utils::mock::mock_raft_context; use crate::test_utils::mock::{MockBuilder, MockTypeConfig}; use crate::test_utils::{create_test_chunk, create_test_snapshot_stream}; use crate::utils::convert::safe_kv_bytes; -use d_engine_proto::client::ReadConsistencyPolicy; -use d_engine_proto::client::{ClientReadRequest, ClientWriteRequest}; use d_engine_proto::server::cluster::{ ClusterConfChangeRequest, ClusterMembership, MetadataRequest, }; @@ -519,7 +519,9 @@ async fn test_handle_client_propose_success() { let cmd = ClientCmd::Propose( ClientWriteRequest { client_id: 1, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }, resp_tx, ); @@ -576,7 +578,7 @@ async fn test_handle_client_read_linearizable_failure() { let client_read_request = ClientReadRequest { client_id: 1, keys, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; use crate::maybe_clone_oneshot::MaybeCloneOneshot; let (resp_tx, mut resp_rx) = >::new(); @@ -666,7 +668,7 @@ async fn test_handle_client_read_linearizable_success() { let keys = vec![safe_kv_bytes(1)]; let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys, }; use crate::maybe_clone_oneshot::MaybeCloneOneshot; @@ -756,7 +758,7 @@ async fn test_handle_client_read_encounters_higher_term() { let keys = vec![safe_kv_bytes(1)]; let client_read_request = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys, }; use crate::maybe_clone_oneshot::MaybeCloneOneshot; @@ -896,12 +898,10 @@ async fn test_drain_read_buffer_clears_pending_reads_on_stepdown() { use crate::maybe_clone_oneshot::MaybeCloneOneshot; let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); let cmd = ClientCmd::Read( - d_engine_proto::client::ClientReadRequest { + ClientReadRequest { client_id: 1, keys: vec![safe_kv_bytes(1)], - consistency_policy: Some( - d_engine_proto::client::ReadConsistencyPolicy::LinearizableRead as i32, - ), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }, resp_tx, ); diff --git a/d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs b/d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs index 52496f63..f94bab6c 100644 --- a/d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/fatal_error_test.rs @@ -10,12 +10,12 @@ use crate::MockTypeConfig; use crate::RaftEvent; use crate::ReadConsistencyPolicy; use crate::RoleEvent; +use crate::client::ClientReadRequest; use crate::maybe_clone_oneshot::MaybeCloneOneshot; use crate::maybe_clone_oneshot::RaftOneshot; use crate::role_state::RaftRoleState; use crate::test_utils::MockBuilder; use bytes::Bytes; -use d_engine_proto::client::ClientReadRequest; use std::sync::Arc; use tokio::sync::{mpsc, watch}; use tonic::Code; @@ -145,7 +145,7 @@ async fn test_fatal_error_drains_all_pending_queues() { let lin_req = ClientReadRequest { client_id: 1, keys: vec![Bytes::from_static(b"k1")], - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), }; leader.push_client_cmd(ClientCmd::Read(lin_req, lin_tx), &ctx); assert_eq!( @@ -159,7 +159,7 @@ async fn test_fatal_error_drains_all_pending_queues() { let lease_req = ClientReadRequest { client_id: 2, keys: vec![Bytes::from_static(b"k2")], - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), }; leader.test_update_lease_timestamp(); leader.push_client_cmd(ClientCmd::Read(lease_req, lease_tx), &ctx); @@ -174,7 +174,7 @@ async fn test_fatal_error_drains_all_pending_queues() { let ev_req = ClientReadRequest { client_id: 3, keys: vec![Bytes::from_static(b"k3")], - consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency as i32), + consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency), }; leader.push_client_cmd(ClientCmd::Read(ev_req, ev_tx), &ctx); assert_eq!( diff --git a/d-engine-core/src/raft_role/leader_state_test/pending_lease_reads_test.rs b/d-engine-core/src/raft_role/leader_state_test/pending_lease_reads_test.rs index 02a2b2d2..7a99420c 100644 --- a/d-engine-core/src/raft_role/leader_state_test/pending_lease_reads_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/pending_lease_reads_test.rs @@ -8,7 +8,7 @@ use std::sync::Arc; use std::time::Duration; -use d_engine_proto::client::ClientReadRequest; +use crate::client::ClientReadRequest; use d_engine_proto::common::LogId; use d_engine_proto::common::NodeRole::Follower; use d_engine_proto::common::NodeStatus; @@ -41,12 +41,12 @@ use crate::test_utils::mock::MockTypeConfig; fn make_lease_read_cmd( sender: crate::MaybeCloneOneshotSender< - std::result::Result, + std::result::Result, > ) -> ClientCmd { let req = ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), keys: vec![safe_kv_bytes(1)], }; ClientCmd::Read(req, sender) @@ -55,7 +55,7 @@ fn make_lease_read_cmd( fn make_lease_read_request() -> ClientReadRequest { ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LeaseRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LeaseRead), keys: vec![safe_kv_bytes(1)], } } diff --git a/d-engine-core/src/raft_role/leader_state_test/pending_reads_test.rs b/d-engine-core/src/raft_role/leader_state_test/pending_reads_test.rs index 45f2a9c2..9fed74c5 100644 --- a/d-engine-core/src/raft_role/leader_state_test/pending_reads_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/pending_reads_test.rs @@ -26,7 +26,7 @@ use std::collections::VecDeque; use std::sync::Arc; use std::time::Duration; -use d_engine_proto::client::ClientReadRequest; +use crate::client::ClientReadRequest; use d_engine_proto::common::LogId; use d_engine_proto::common::NodeRole::Follower; use d_engine_proto::common::NodeStatus; @@ -63,13 +63,13 @@ use crate::test_utils::mock::mock_raft_builder::mock_state_machine; /// Build a LinearizableRead client command wired to the given sender. fn linear_read_cmd( sender: crate::MaybeCloneOneshotSender< - std::result::Result, + std::result::Result, > ) -> ClientCmd { ClientCmd::Read( ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![safe_kv_bytes(1)], }, sender, @@ -286,7 +286,7 @@ async fn test_pending_reads_drained_by_quorum_ack() { requests.push_back(( ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![safe_kv_bytes(1)], }, resp_tx, @@ -357,7 +357,7 @@ async fn test_pending_reads_slow_path_drained_by_apply_completed() { requests.push_back(( ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![safe_kv_bytes(1)], }, resp_tx, @@ -429,7 +429,7 @@ async fn test_pending_reads_partition_scenario_times_out() { requests.push_back(( ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![safe_kv_bytes(1)], }, resp_tx, @@ -495,7 +495,7 @@ async fn test_pending_reads_cleared_on_stepdown() { requests.push_back(( ClientReadRequest { client_id: 1, - consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead as i32), + consistency_policy: Some(ReadConsistencyPolicy::LinearizableRead), keys: vec![safe_kv_bytes(1)], }, resp_tx, diff --git a/d-engine-core/src/raft_role/leader_state_test/replication_test.rs b/d-engine-core/src/raft_role/leader_state_test/replication_test.rs index 26e24a83..aeb38c9c 100644 --- a/d-engine-core/src/raft_role/leader_state_test/replication_test.rs +++ b/d-engine-core/src/raft_role/leader_state_test/replication_test.rs @@ -21,13 +21,13 @@ use crate::{MaybeCloneOneshot, PeerUpdate}; use crate::RaftRequestWithSignal; +use crate::client::ClientResponse; use crate::event::RoleEvent; use crate::maybe_clone_oneshot::RaftOneshot; use crate::raft_role::leader_state::LeaderState; use crate::role_state::RaftRoleState; use crate::test_utils::mock::MockTypeConfig; use crate::test_utils::mock::mock_raft_context; -use d_engine_proto::client::ClientResponse; use d_engine_proto::common::{EntryPayload, LogId, NodeRole::Follower, NodeStatus}; use d_engine_proto::server::cluster::{ClusterMembership, NodeMeta}; use d_engine_proto::server::replication::append_entries_response; diff --git a/d-engine-core/src/raft_role/learner_state_test.rs b/d-engine-core/src/raft_role/learner_state_test.rs index bf4e7962..25169c52 100644 --- a/d-engine-core/src/raft_role/learner_state_test.rs +++ b/d-engine-core/src/raft_role/learner_state_test.rs @@ -8,6 +8,12 @@ use crate::NewCommitData; use crate::RaftEvent; use crate::RaftOneshot; use crate::RoleEvent; +use crate::client::ClientReadRequest; +use crate::client::ClientResponsePayload; +use crate::client::ClientWriteRequest; +use crate::client::KvEntry; +use crate::client::WriteOperation; +use crate::config::ReadConsistencyPolicy; use crate::raft_role::learner_state::LearnerState; use crate::raft_role::role_state::RaftRoleState; use crate::test_utils::create_test_snapshot_stream; @@ -15,7 +21,6 @@ use crate::test_utils::mock::MockTypeConfig; use crate::test_utils::mock::mock_raft_context; use crate::test_utils::mock::mock_raft_context_with_temp; use crate::test_utils::node_config; -use d_engine_proto::client::WriteCommand; use d_engine_proto::common::LogId; use d_engine_proto::common::NodeRole; use d_engine_proto::common::NodeStatus; @@ -474,9 +479,11 @@ async fn test_learner_rejects_client_write_request() { let (resp_tx, mut resp_rx) = MaybeCloneOneshot::new(); let cmd = ClientCmd::Propose( - d_engine_proto::client::ClientWriteRequest { + ClientWriteRequest { client_id: 1, - command: Some(WriteCommand::default()), + command: Some(WriteOperation::Delete { + key: bytes::Bytes::new(), + }), }, resp_tx, ); @@ -508,7 +515,7 @@ async fn test_learner_rejects_client_read_request() { let mut state = LearnerState::::new(1, context.node_config.clone()); - let client_read_request = d_engine_proto::client::ClientReadRequest { + let client_read_request = ClientReadRequest { client_id: 1, consistency_policy: None, keys: vec![], @@ -1463,7 +1470,7 @@ async fn test_learner_serves_eventual_read_locally() { .times(1) .withf(|keys| keys.len() == 1 && keys[0] == "eventual_key") .returning(|_| { - Some(vec![d_engine_proto::client::ClientResult { + Some(vec![KvEntry { key: bytes::Bytes::from("eventual_key"), value: bytes::Bytes::from("eventual_value"), }]) @@ -1474,12 +1481,10 @@ async fn test_learner_serves_eventual_read_locally() { // Send EventualConsistency read request let (response_tx, mut response_rx) = MaybeCloneOneshot::new(); - let read_req = d_engine_proto::client::ClientReadRequest { + let read_req = ClientReadRequest { client_id: 1, keys: vec![bytes::Bytes::from("eventual_key")], - consistency_policy: Some( - d_engine_proto::client::ReadConsistencyPolicy::EventualConsistency as i32, - ), + consistency_policy: Some(ReadConsistencyPolicy::EventualConsistency), }; let start = tokio::time::Instant::now(); @@ -1504,18 +1509,16 @@ async fn test_learner_serves_eventual_read_locally() { if let Ok(response) = result { match response { Ok(read_response) => { - // Check success_result for ReadResults - match read_response.success_result { - Some(d_engine_proto::client::client_response::SuccessResult::ReadData( - read_data, - )) => { - assert!(!read_data.results.is_empty(), "Should have read results"); + // Check result for ReadResults + match read_response.result { + Some(ClientResponsePayload::Read(read_data)) => { + assert!(!read_data.entries.is_empty(), "Should have read results"); assert_eq!( - read_data.results[0].value, + read_data.entries[0].value, bytes::Bytes::from("eventual_value") ); } - other => panic!("Expected ReadData variant, got: {other:?}"), + other => panic!("Expected Read variant, got: {other:?}"), } } Err(e) => { diff --git a/d-engine-core/src/raft_role/role_state.rs b/d-engine-core/src/raft_role/role_state.rs index 4decc059..e154a8cd 100644 --- a/d-engine-core/src/raft_role/role_state.rs +++ b/d-engine-core/src/raft_role/role_state.rs @@ -1,5 +1,5 @@ +use crate::client::{ClientReadRequest, ClientResponse, KvEntry, LeaderHint}; use async_trait::async_trait; -use d_engine_proto::client::ClientResponse; use d_engine_proto::server::election::VotedFor; use d_engine_proto::server::replication::AppendEntriesRequest; use d_engine_proto::server::replication::AppendEntriesResponse; @@ -283,7 +283,7 @@ pub trait RaftRoleState: Send + Sync + 'static { ctx: &RaftContext, ) { use crate::ReadConsistencyPolicy as ServerReadConsistencyPolicy; - use d_engine_proto::client::ReadConsistencyPolicy as ClientReadConsistencyPolicy; + use crate::config::ReadConsistencyPolicy as ClientReadConsistencyPolicy; match cmd { ClientCmd::Propose(_, sender) => { @@ -298,10 +298,10 @@ pub trait RaftRoleState: Send + Sync + 'static { // Determine effective read policy, mirroring leader_state logic: // 1. If client specified a policy AND server allows override, use client policy. // 2. Otherwise (no policy, or override disabled), use server default. - let effective_policy = if req.has_consistency_policy() + let effective_policy = if let Some(ref policy) = req.consistency_policy && ctx.node_config().raft.read_consistency.allow_client_override { - match req.consistency_policy() { + match policy { ClientReadConsistencyPolicy::EventualConsistency => { ServerReadConsistencyPolicy::EventualConsistency } @@ -337,16 +337,12 @@ pub trait RaftRoleState: Send + Sync + 'static { /// May return stale data but provides best read performance and availability. fn process_eventual_read_local( &self, - req: d_engine_proto::client::ClientReadRequest, - sender: crate::MaybeCloneOneshotSender< - std::result::Result, - >, + req: ClientReadRequest, + sender: crate::MaybeCloneOneshotSender>, ctx: &RaftContext, ) { - use d_engine_proto::client::ClientResponse; - // Read directly from local state machine without any consistency checks - let results = ctx + let results: Vec = ctx .state_machine_handler() .read_from_state_machine(req.keys) .unwrap_or_default(); @@ -431,15 +427,15 @@ pub trait RaftRoleState: Send + Sync + 'static { let leader_node = members.iter().find(|n| n.id == lid); if let Some(leader) = leader_node { - return ClientResponse::not_leader( - Some(lid.to_string()), - Some(leader.address.clone()), - ); + return ClientResponse::not_leader(Some(LeaderHint { + leader_id: lid, + address: leader.address.clone(), + })); } } // No leader info available, return basic NOT_LEADER error - ClientResponse::not_leader(None, None) + ClientResponse::not_leader(None) } /// When a Follower receives an AppendEntries request, it performs the following logic (as diff --git a/d-engine-core/src/replication/mod.rs b/d-engine-core/src/replication/mod.rs index 58317dbd..cded9e9a 100644 --- a/d-engine-core/src/replication/mod.rs +++ b/d-engine-core/src/replication/mod.rs @@ -15,8 +15,8 @@ pub mod replication_handler_test; use std::collections::HashMap; use std::sync::Arc; +use crate::client::ClientResponse; use async_trait::async_trait; -use d_engine_proto::client::ClientResponse; use d_engine_proto::common::Entry; use d_engine_proto::common::EntryPayload; use d_engine_proto::server::replication::AppendEntriesRequest; diff --git a/d-engine-core/src/state_machine_handler/default_state_machine_handler.rs b/d-engine-core/src/state_machine_handler/default_state_machine_handler.rs index 81316ec1..da36a56c 100644 --- a/d-engine-core/src/state_machine_handler/default_state_machine_handler.rs +++ b/d-engine-core/src/state_machine_handler/default_state_machine_handler.rs @@ -7,12 +7,12 @@ use std::sync::atomic::AtomicU64; use std::sync::atomic::Ordering; use std::time::Duration; +use crate::client::KvEntry; use async_compression::tokio::bufread::GzipDecoder; use async_compression::tokio::write::GzipEncoder; use async_stream::try_stream; use async_trait::async_trait; use bytes::Bytes; -use d_engine_proto::client::ClientResult; use d_engine_proto::common::Entry; use d_engine_proto::common::LogId; use d_engine_proto::server::storage::SnapshotAck; @@ -304,15 +304,14 @@ where apply_result } - /// TODO: decouple client related commands with RAFT internal logic fn read_from_state_machine( &self, keys: Vec, - ) -> Option> { + ) -> Option> { let mut result = Vec::new(); for key in keys { if let Ok(Some(value)) = self.state_machine.get(&key) { - result.push(ClientResult { key, value }); + result.push(KvEntry { key, value }); } } diff --git a/d-engine-core/src/state_machine_handler/mod.rs b/d-engine-core/src/state_machine_handler/mod.rs index d053778d..bbf6ac66 100644 --- a/d-engine-core/src/state_machine_handler/mod.rs +++ b/d-engine-core/src/state_machine_handler/mod.rs @@ -52,8 +52,8 @@ mod wait_applied_test; #[cfg(test)] mod worker_test; +use crate::client::KvEntry; use async_trait::async_trait; -use d_engine_proto::client::ClientResult; use d_engine_proto::server::storage::SnapshotChunk; use d_engine_proto::server::storage::SnapshotMetadata; use futures::stream::BoxStream; @@ -105,7 +105,7 @@ where fn read_from_state_machine( &self, keys: Vec, - ) -> Option>; + ) -> Option>; /// Receives and installs a snapshot stream pushed by the leader /// Used when leader proactively sends snapshot updates to followers diff --git a/d-engine-server/src/api/embedded_client.rs b/d-engine-server/src/api/embedded_client.rs index b90c80aa..892bd391 100644 --- a/d-engine-server/src/api/embedded_client.rs +++ b/d-engine-server/src/api/embedded_client.rs @@ -25,13 +25,11 @@ use d_engine_core::MaybeCloneOneshot; use d_engine_core::RaftEvent; use d_engine_core::RaftOneshot; use d_engine_core::ScanResult; -use d_engine_core::client::{ClientApi, ClientApiError, ClientApiResult}; -use d_engine_proto::client::ClientReadRequest; -use d_engine_proto::client::ClientWriteRequest; -use d_engine_proto::client::ReadConsistencyPolicy; -use d_engine_proto::client::WriteCommand; -use d_engine_proto::common::LeaderHint; -use d_engine_proto::error::ErrorCode; +use d_engine_core::client::{ + ClientApi, ClientApiError, ClientApiResult, ClientReadRequest, ClientResponsePayload, + ClientWriteRequest, ErrorCode, LeaderHint, WriteOperation, +}; +use d_engine_core::config::ReadConsistencyPolicy; use tokio::sync::mpsc; #[cfg(feature = "watch")] @@ -134,21 +132,20 @@ impl EmbeddedClient { self } - /// Map ErrorCode and ErrorMetadata to ClientApiError fn map_error_response( - error_code: i32, - metadata: Option, + error: ErrorCode, + leader_hint: Option, ) -> ClientApiError { - match ErrorCode::try_from(error_code) { - Ok(ErrorCode::NotLeader) => { - let (leader_id, leader_address) = if let Some(meta) = metadata { - (meta.leader_id, meta.leader_address) + match error { + ErrorCode::NotLeader => { + let (leader_id, leader_address) = if let Some(hint) = leader_hint { + (Some(hint.leader_id.to_string()), Some(hint.address)) } else { (None, None) }; not_leader_error(leader_id, leader_address) } - _ => server_error(format!("Error code: {error_code}")), + _ => server_error(format!("Error code: {error:?}")), } } @@ -162,14 +159,13 @@ impl EmbeddedClient { key: impl AsRef<[u8]>, value: impl AsRef<[u8]>, ) -> ClientApiResult<()> { - let command = WriteCommand::insert( - Bytes::copy_from_slice(key.as_ref()), - Bytes::copy_from_slice(value.as_ref()), - ); - let request = ClientWriteRequest { client_id: self.client_id, - command: Some(command), + command: Some(WriteOperation::Insert { + key: Bytes::copy_from_slice(key.as_ref()), + value: Bytes::copy_from_slice(value.as_ref()), + ttl_secs: None, + }), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); @@ -187,8 +183,11 @@ impl EmbeddedClient { let response = result.map_err(|status| server_error(format!("RPC error: {}", status.message())))?; - if response.error != ErrorCode::Success as i32 { - return Err(Self::map_error_response(response.error, response.metadata)); + if response.error != ErrorCode::Success { + return Err(Self::map_error_response( + response.error, + response.leader_hint, + )); } Ok(()) @@ -270,7 +269,7 @@ impl EmbeddedClient { let request = ClientReadRequest { client_id: self.client_id, keys: vec![Bytes::copy_from_slice(key.as_ref())], - consistency_policy: Some(consistency as i32), + consistency_policy: Some(consistency), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); @@ -288,17 +287,16 @@ impl EmbeddedClient { let response = result.map_err(|status| server_error(format!("RPC error: {}", status.message())))?; - if response.error != ErrorCode::Success as i32 { - return Err(Self::map_error_response(response.error, response.metadata)); + if response.error != ErrorCode::Success { + return Err(Self::map_error_response( + response.error, + response.leader_hint, + )); } - match response.success_result { - Some(d_engine_proto::client::client_response::SuccessResult::ReadData( - read_results, - )) => { - // If results list is empty, key doesn't exist - // Otherwise, return the value (even if empty bytes) - Ok(read_results.results.first().map(|r| r.value.clone())) + match response.result { + Some(ClientResponsePayload::Read(read_results)) => { + Ok(read_results.entries.first().map(|e| e.value.clone())) } _ => Ok(None), } @@ -347,7 +345,7 @@ impl EmbeddedClient { let request = ClientReadRequest { client_id: self.client_id, keys: keys.to_vec(), - consistency_policy: Some(consistency as i32), + consistency_policy: Some(consistency), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); @@ -365,19 +363,20 @@ impl EmbeddedClient { let response = result.map_err(|status| server_error(format!("RPC error: {}", status.message())))?; - if response.error != ErrorCode::Success as i32 { - return Err(Self::map_error_response(response.error, response.metadata)); + if response.error != ErrorCode::Success { + return Err(Self::map_error_response( + response.error, + response.leader_hint, + )); } - match response.success_result { - Some(d_engine_proto::client::client_response::SuccessResult::ReadData( - read_results, - )) => { + match response.result { + Some(ClientResponsePayload::Read(read_results)) => { // Reconstruct result vector in requested key order. // Server only returns results for keys that exist, so we must // map by key to preserve positional correspondence with input. let results_by_key: std::collections::HashMap<_, _> = - read_results.results.into_iter().map(|r| (r.key, r.value)).collect(); + read_results.entries.into_iter().map(|e| (e.key, e.value)).collect(); Ok(keys.iter().map(|k| results_by_key.get(k).cloned()).collect()) } @@ -394,11 +393,11 @@ impl EmbeddedClient { &self, key: impl AsRef<[u8]>, ) -> ClientApiResult<()> { - let command = WriteCommand::delete(Bytes::copy_from_slice(key.as_ref())); - let request = ClientWriteRequest { client_id: self.client_id, - command: Some(command), + command: Some(WriteOperation::Delete { + key: Bytes::copy_from_slice(key.as_ref()), + }), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); @@ -416,8 +415,11 @@ impl EmbeddedClient { let response = result.map_err(|status| server_error(format!("RPC error: {}", status.message())))?; - if response.error != ErrorCode::Success as i32 { - return Err(Self::map_error_response(response.error, response.metadata)); + if response.error != ErrorCode::Success { + return Err(Self::map_error_response( + response.error, + response.leader_hint, + )); } Ok(()) @@ -682,15 +684,13 @@ impl ClientApi for EmbeddedClient { value: impl AsRef<[u8]> + Send, ttl_secs: u64, ) -> ClientApiResult<()> { - let command = WriteCommand::insert_with_ttl( - Bytes::copy_from_slice(key.as_ref()), - Bytes::copy_from_slice(value.as_ref()), - ttl_secs, - ); - let request = ClientWriteRequest { client_id: self.client_id, - command: Some(command), + command: Some(WriteOperation::Insert { + key: Bytes::copy_from_slice(key.as_ref()), + value: Bytes::copy_from_slice(value.as_ref()), + ttl_secs: Some(ttl_secs), + }), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); @@ -708,8 +708,11 @@ impl ClientApi for EmbeddedClient { let response = result.map_err(|status| server_error(format!("RPC error: {}", status.message())))?; - if response.error != ErrorCode::Success as i32 { - return Err(Self::map_error_response(response.error, response.metadata)); + if response.error != ErrorCode::Success { + return Err(Self::map_error_response( + response.error, + response.leader_hint, + )); } Ok(()) @@ -742,15 +745,13 @@ impl ClientApi for EmbeddedClient { expected_value: Option + Send>, new_value: impl AsRef<[u8]> + Send, ) -> ClientApiResult { - let command = WriteCommand::compare_and_swap( - Bytes::copy_from_slice(key.as_ref()), - expected_value.map(|v| Bytes::copy_from_slice(v.as_ref())), - Bytes::copy_from_slice(new_value.as_ref()), - ); - let request = ClientWriteRequest { client_id: self.client_id, - command: Some(command), + command: Some(WriteOperation::CompareAndSwap { + key: Bytes::copy_from_slice(key.as_ref()), + expected: expected_value.map(|v| Bytes::copy_from_slice(v.as_ref())), + new_value: Bytes::copy_from_slice(new_value.as_ref()), + }), }; let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); @@ -768,14 +769,15 @@ impl ClientApi for EmbeddedClient { let response = result.map_err(|status| server_error(format!("RPC error: {}", status.message())))?; - if response.error != ErrorCode::Success as i32 { - return Err(Self::map_error_response(response.error, response.metadata)); + if response.error != ErrorCode::Success { + return Err(Self::map_error_response( + response.error, + response.leader_hint, + )); } - match response.success_result { - Some(d_engine_proto::client::client_response::SuccessResult::WriteResult(result)) => { - Ok(result.succeeded) - } + match response.result { + Some(ClientResponsePayload::Write(result)) => Ok(result.succeeded), _ => Err(server_error("Invalid CAS response".to_string())), } } @@ -795,11 +797,12 @@ impl ClientApi for EmbeddedClient { async fn get_multi_with_policy( &self, keys: &[Bytes], - consistency_policy: Option, + consistency_policy: Option, ) -> ClientApiResult>> { self.get_multi_with_consistency( keys, - consistency_policy.unwrap_or(ReadConsistencyPolicy::LinearizableRead), + consistency_policy + .unwrap_or(d_engine_core::config::ReadConsistencyPolicy::LinearizableRead), ) .await } diff --git a/d-engine-server/src/lib.rs b/d-engine-server/src/lib.rs index 84aa9fb4..2e14bf30 100644 --- a/d-engine-server/src/lib.rs +++ b/d-engine-server/src/lib.rs @@ -93,6 +93,11 @@ // ==================== Core Public API ==================== +/// Proto ↔ core type conversions — the single place that bridges gRPC and core types. +mod proto_convert; +#[cfg(test)] +mod proto_convert_test; + /// Node lifecycle management /// /// Contains [`Node`] and [`NodeBuilder`] for server setup. @@ -137,6 +142,10 @@ pub use d_engine_core::ProstError; pub use d_engine_core::SnapshotError; // -------------------- Client API -------------------- pub use api::EmbeddedClient; +/// Error code discriminator returned by [`ClientApiError::code()`]. +/// Use this to pattern-match error categories without inspecting message strings: +/// `e.code() == ErrorCode::NotLeader` +pub use d_engine_core::client::ErrorCode; /// Unified client operations trait (put/get/delete/CAS/watch). /// Re-exported here so embedded-mode users don't need a separate d-engine-client dependency. pub use d_engine_core::client::{ClientApi, ClientApiError, ClientApiResult}; @@ -144,10 +153,6 @@ pub use d_engine_core::client::{ClientApi, ClientApiError, ClientApiResult}; /// /// Implement this trait to create your own storage engine. pub use d_engine_core::{StateMachine, StorageEngine}; -/// Error code discriminator returned by [`ClientApiError::code()`]. -/// Use this to pattern-match error categories without inspecting message strings: -/// `e.code() == ErrorCode::NotLeader` -pub use d_engine_proto::error::ErrorCode; pub use node::Node; pub use node::NodeBuilder; // Re-export storage implementations diff --git a/d-engine-server/src/network/grpc/grpc_raft_service.rs b/d-engine-server/src/network/grpc/grpc_raft_service.rs index b288a7e2..b3439215 100644 --- a/d-engine-server/src/network/grpc/grpc_raft_service.rs +++ b/d-engine-server/src/network/grpc/grpc_raft_service.rs @@ -59,6 +59,7 @@ use tracing::info; use tracing::warn; use crate::Node; +use crate::proto_convert; #[tonic::async_trait] impl RaftElectionService for Node @@ -410,19 +411,21 @@ where let cmd_tx = self.cmd_tx.clone(); let request_future = async move { - let req: ClientWriteRequest = request.into_inner(); - // Extract request and validate - if req.command.is_none() { + let proto_req: ClientWriteRequest = request.into_inner(); + if proto_req.command.is_none() { return Err(Status::invalid_argument("Command cannot be empty")); } + let core_req = proto_convert::to_core_write_req(proto_req); let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); cmd_tx - .send(d_engine_core::ClientCmd::Propose(req, resp_tx)) + .send(d_engine_core::ClientCmd::Propose(core_req, resp_tx)) .await .map_err(|_| Status::internal("Command channel closed"))?; - handle_rpc_timeout(resp_rx, timeout_duration, "handle_client_write").await + handle_rpc_timeout(resp_rx, timeout_duration, "handle_client_write") + .await + .map(|resp| resp.map(proto_convert::to_proto_response)) }; let cancellation_future = async move { @@ -451,18 +454,18 @@ where return Err(Status::unavailable("Service is not ready")); } + let core_req = proto_convert::to_core_read_req(request.into_inner()); let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); self.cmd_tx - .send(d_engine_core::ClientCmd::Read( - request.into_inner(), - resp_tx, - )) + .send(d_engine_core::ClientCmd::Read(core_req, resp_tx)) .await .map_err(|_| Status::internal("Command channel closed"))?; let timeout_duration = Duration::from_millis(self.node_config.raft.general_raft_timeout_duration_in_ms); - handle_rpc_timeout(resp_rx, timeout_duration, "handle_client_read").await + handle_rpc_timeout(resp_rx, timeout_duration, "handle_client_read") + .await + .map(|resp| resp.map(proto_convert::to_proto_response)) } /// Scan all keys under a prefix. diff --git a/d-engine-server/src/proto_convert.rs b/d-engine-server/src/proto_convert.rs new file mode 100644 index 00000000..5583c079 --- /dev/null +++ b/d-engine-server/src/proto_convert.rs @@ -0,0 +1,238 @@ +//! Conversions between proto-generated types and d-engine-core native types. +//! +//! # Why free functions instead of From impls +//! +//! Both proto types (d-engine-proto) and core types (d-engine-core) are external +//! to this crate. Rust's orphan rule forbids `impl From for ExternalB` +//! in a third crate. Free functions sidestep this without any ergonomics or +//! performance cost — callers are explicit about the conversion, and `#[inline]` +//! still works across crate boundaries. +//! +//! # Performance +//! +//! - All functions are `#[inline]` +//! - `Bytes` fields are moved (refcount unchanged, zero allocation) +//! - `ErrorCode` conversion uses exhaustive match — LLVM optimises to identity; +//! a missing variant is a compile error (correctness guarantee) + +use d_engine_core::client::{ + ClientReadRequest, ClientResponse, ClientResponsePayload, ClientWriteRequest, ErrorCode, + KvEntry, LeaderHint, ReadResults, WriteOperation, WriteResult, +}; +use d_engine_core::config::ReadConsistencyPolicy; +use d_engine_proto::client::write_command::Operation; +use d_engine_proto::client::{ + self as proto_client, ClientResult, ReadResults as ProtoReadResults, + WriteResult as ProtoWriteResult, +}; +use d_engine_proto::error::{ErrorCode as ProtoErrorCode, ErrorMetadata}; + +// ─── proto → core ───────────────────────────────────────────────────────────── + +/// Convert a proto `WriteCommand` to a core `WriteOperation`. +/// +/// Callers **must** validate that the command is non-empty before calling this +/// (i.e. `wc.operation.is_some()`). An empty operation is a server-side bug, +/// not a client error, and will panic. +#[inline] +pub(crate) fn write_command_to_op(wc: proto_client::WriteCommand) -> WriteOperation { + match wc.operation { + Some(Operation::Insert(i)) => WriteOperation::Insert { + key: i.key, + value: i.value, + // Proto convention: 0 == no expiration → None in core + ttl_secs: if i.ttl_secs == 0 { + None + } else { + Some(i.ttl_secs) + }, + }, + Some(Operation::Delete(d)) => WriteOperation::Delete { key: d.key }, + Some(Operation::CompareAndSwap(c)) => WriteOperation::CompareAndSwap { + key: c.key, + expected: c.expected_value, + new_value: c.new_value, + }, + None => unreachable!("WriteCommand must have an operation"), + } +} + +#[inline] +pub(crate) fn to_core_write_req(req: proto_client::ClientWriteRequest) -> ClientWriteRequest { + ClientWriteRequest { + client_id: req.client_id, + command: req.command.map(write_command_to_op), + } +} + +#[inline] +pub(crate) fn to_core_read_req(req: proto_client::ClientReadRequest) -> ClientReadRequest { + ClientReadRequest { + client_id: req.client_id, + keys: req.keys, // Vec moved, O(1) + consistency_policy: req + .consistency_policy + .and_then(|i| proto_client::ReadConsistencyPolicy::try_from(i).ok()) + .map(ReadConsistencyPolicy::from), + } +} + +#[allow(dead_code)] +#[inline] +pub(crate) fn proto_error_to_core(e: ProtoErrorCode) -> ErrorCode { + match e { + ProtoErrorCode::Success => ErrorCode::Success, + ProtoErrorCode::ConnectionTimeout => ErrorCode::ConnectionTimeout, + ProtoErrorCode::InvalidAddress => ErrorCode::InvalidAddress, + ProtoErrorCode::LeaderChanged => ErrorCode::LeaderChanged, + ProtoErrorCode::JoinError => ErrorCode::JoinError, + ProtoErrorCode::InvalidResponse => ErrorCode::InvalidResponse, + ProtoErrorCode::VersionMismatch => ErrorCode::VersionMismatch, + ProtoErrorCode::DiskFull => ErrorCode::DiskFull, + ProtoErrorCode::DataCorruption => ErrorCode::DataCorruption, + ProtoErrorCode::StorageIoError => ErrorCode::StorageIoError, + ProtoErrorCode::StoragePermissionDenied => ErrorCode::StoragePermissionDenied, + ProtoErrorCode::KeyNotExist => ErrorCode::KeyNotExist, + ProtoErrorCode::NotLeader => ErrorCode::NotLeader, + ProtoErrorCode::StaleOperation => ErrorCode::StaleOperation, + ProtoErrorCode::InvalidRequest => ErrorCode::InvalidRequest, + ProtoErrorCode::RateLimited => ErrorCode::RateLimited, + ProtoErrorCode::ClusterUnavailable => ErrorCode::ClusterUnavailable, + ProtoErrorCode::ProposeFailed => ErrorCode::ProposeFailed, + ProtoErrorCode::TermOutdated => ErrorCode::TermOutdated, + ProtoErrorCode::RetryRequired => ErrorCode::RetryRequired, + ProtoErrorCode::WatchBufferOverflow => ErrorCode::WatchBufferOverflow, + ProtoErrorCode::General => ErrorCode::General, + ProtoErrorCode::Uncategorized => ErrorCode::Uncategorized, + } +} + +#[allow(dead_code)] +#[inline] +pub(crate) fn to_core_response(r: proto_client::ClientResponse) -> ClientResponse { + let error = ProtoErrorCode::try_from(r.error) + .map(proto_error_to_core) + .unwrap_or(ErrorCode::Uncategorized); + + let (leader_hint, retry_after_ms) = match r.metadata { + Some(meta) => { + let hint = parse_leader_hint(&meta); + (hint, meta.retry_after_ms) + } + None => (None, None), + }; + + let result = r.success_result.map(|sr| match sr { + proto_client::client_response::SuccessResult::WriteResult(w) => { + ClientResponsePayload::Write(WriteResult { + succeeded: w.succeeded, + }) + } + proto_client::client_response::SuccessResult::ReadData(rd) => { + ClientResponsePayload::Read(ReadResults { + entries: rd + .results + .into_iter() + .map(|e| KvEntry { + key: e.key, + value: e.value, + }) + .collect(), + }) + } + }); + + ClientResponse { + error, + leader_hint, + retry_after_ms, + result, + } +} + +// ─── core → proto ───────────────────────────────────────────────────────────── + +#[inline] +pub(crate) fn core_error_to_proto(e: ErrorCode) -> ProtoErrorCode { + match e { + ErrorCode::Success => ProtoErrorCode::Success, + ErrorCode::ConnectionTimeout => ProtoErrorCode::ConnectionTimeout, + ErrorCode::InvalidAddress => ProtoErrorCode::InvalidAddress, + ErrorCode::LeaderChanged => ProtoErrorCode::LeaderChanged, + ErrorCode::JoinError => ProtoErrorCode::JoinError, + ErrorCode::InvalidResponse => ProtoErrorCode::InvalidResponse, + ErrorCode::VersionMismatch => ProtoErrorCode::VersionMismatch, + ErrorCode::DiskFull => ProtoErrorCode::DiskFull, + ErrorCode::DataCorruption => ProtoErrorCode::DataCorruption, + ErrorCode::StorageIoError => ProtoErrorCode::StorageIoError, + ErrorCode::StoragePermissionDenied => ProtoErrorCode::StoragePermissionDenied, + ErrorCode::KeyNotExist => ProtoErrorCode::KeyNotExist, + ErrorCode::NotLeader => ProtoErrorCode::NotLeader, + ErrorCode::StaleOperation => ProtoErrorCode::StaleOperation, + ErrorCode::InvalidRequest => ProtoErrorCode::InvalidRequest, + ErrorCode::RateLimited => ProtoErrorCode::RateLimited, + ErrorCode::ClusterUnavailable => ProtoErrorCode::ClusterUnavailable, + ErrorCode::ProposeFailed => ProtoErrorCode::ProposeFailed, + ErrorCode::TermOutdated => ProtoErrorCode::TermOutdated, + ErrorCode::RetryRequired => ProtoErrorCode::RetryRequired, + ErrorCode::WatchBufferOverflow => ProtoErrorCode::WatchBufferOverflow, + ErrorCode::General => ProtoErrorCode::General, + ErrorCode::Uncategorized => ProtoErrorCode::Uncategorized, + } +} + +#[inline] +pub(crate) fn to_proto_response(r: ClientResponse) -> proto_client::ClientResponse { + let error_code = core_error_to_proto(r.error); + let metadata = build_error_metadata(r.leader_hint, r.retry_after_ms); + let success_result = r.result.map(|payload| match payload { + ClientResponsePayload::Write(w) => { + proto_client::client_response::SuccessResult::WriteResult(ProtoWriteResult { + succeeded: w.succeeded, + }) + } + ClientResponsePayload::Read(rd) => { + proto_client::client_response::SuccessResult::ReadData(ProtoReadResults { + results: rd + .entries + .into_iter() + .map(|e| ClientResult { + key: e.key, + value: e.value, + }) + .collect(), + }) + } + }); + proto_client::ClientResponse { + error: error_code as i32, + metadata, + success_result, + } +} + +// ─── Helpers ────────────────────────────────────────────────────────────────── + +#[inline] +fn build_error_metadata( + leader_hint: Option, + retry_after_ms: Option, +) -> Option { + if leader_hint.is_none() && retry_after_ms.is_none() { + return None; + } + Some(ErrorMetadata { + retry_after_ms, + leader_id: leader_hint.as_ref().map(|h| h.leader_id.to_string()), + leader_address: leader_hint.map(|h| h.address), + debug_message: None, + }) +} + +#[allow(dead_code)] +#[inline] +fn parse_leader_hint(meta: &ErrorMetadata) -> Option { + let leader_id = meta.leader_id.as_ref()?.parse::().ok()?; + let address = meta.leader_address.clone()?; + Some(LeaderHint { leader_id, address }) +} diff --git a/d-engine-server/src/proto_convert_test.rs b/d-engine-server/src/proto_convert_test.rs new file mode 100644 index 00000000..aa6e3d67 --- /dev/null +++ b/d-engine-server/src/proto_convert_test.rs @@ -0,0 +1,390 @@ +use bytes::Bytes; +use d_engine_core::client::{ + ClientReadRequest, ClientResponse, ClientResponsePayload, ClientWriteRequest, ErrorCode, + KvEntry, LeaderHint, ReadResults, WriteOperation, WriteResult, +}; +use d_engine_core::config::ReadConsistencyPolicy; +use d_engine_proto::client as proto_client; +use d_engine_proto::client::write_command::{CompareAndSwap, Delete, Insert, Operation}; +use d_engine_proto::error::ErrorCode as ProtoErrorCode; + +use crate::proto_convert::{ + core_error_to_proto, proto_error_to_core, to_core_read_req, to_core_response, + to_core_write_req, to_proto_response, write_command_to_op, +}; + +// ─── write_command_to_op ────────────────────────────────────────────────────── + +#[test] +fn test_write_command_insert_with_ttl_converts_correctly() { + let wc = proto_client::WriteCommand { + operation: Some(Operation::Insert(Insert { + key: Bytes::from("k"), + value: Bytes::from("v"), + ttl_secs: 60, + })), + }; + let op = write_command_to_op(wc); + assert_eq!( + op, + WriteOperation::Insert { + key: Bytes::from("k"), + value: Bytes::from("v"), + ttl_secs: Some(60), + } + ); +} + +/// Proto convention: ttl_secs == 0 means "no expiration". Must become None in core. +#[test] +fn test_write_command_insert_ttl_zero_becomes_none() { + let wc = proto_client::WriteCommand { + operation: Some(Operation::Insert(Insert { + key: Bytes::from("k"), + value: Bytes::from("v"), + ttl_secs: 0, + })), + }; + let op = write_command_to_op(wc); + assert!(matches!(op, WriteOperation::Insert { ttl_secs: None, .. })); +} + +#[test] +fn test_write_command_delete_converts_correctly() { + let wc = proto_client::WriteCommand { + operation: Some(Operation::Delete(Delete { + key: Bytes::from("del"), + })), + }; + let op = write_command_to_op(wc); + assert_eq!( + op, + WriteOperation::Delete { + key: Bytes::from("del") + } + ); +} + +#[test] +fn test_write_command_cas_with_expected_value_converts_correctly() { + let wc = proto_client::WriteCommand { + operation: Some(Operation::CompareAndSwap(CompareAndSwap { + key: Bytes::from("k"), + expected_value: Some(Bytes::from("old")), + new_value: Bytes::from("new"), + })), + }; + let op = write_command_to_op(wc); + assert_eq!( + op, + WriteOperation::CompareAndSwap { + key: Bytes::from("k"), + expected: Some(Bytes::from("old")), + new_value: Bytes::from("new"), + } + ); +} + +/// CAS with expected_value=None means "key must not exist" — distinct semantic. +#[test] +fn test_write_command_cas_key_must_not_exist_when_expected_none() { + let wc = proto_client::WriteCommand { + operation: Some(Operation::CompareAndSwap(CompareAndSwap { + key: Bytes::from("k"), + expected_value: None, + new_value: Bytes::from("new"), + })), + }; + let op = write_command_to_op(wc); + assert!(matches!( + op, + WriteOperation::CompareAndSwap { expected: None, .. } + )); +} + +// ─── to_core_write_req ──────────────────────────────────────────────────────── + +#[test] +fn test_to_core_write_req_insert_roundtrip() { + let proto_req = proto_client::ClientWriteRequest { + client_id: 7, + command: Some(proto_client::WriteCommand { + operation: Some(Operation::Insert(Insert { + key: Bytes::from("key"), + value: Bytes::from("val"), + ttl_secs: 0, + })), + }), + }; + let core_req = to_core_write_req(proto_req); + assert_eq!(core_req.client_id, 7); + assert!(matches!( + core_req.command, + Some(WriteOperation::Insert { ttl_secs: None, .. }) + )); +} + +#[test] +fn test_to_core_write_req_empty_command_becomes_none() { + let proto_req = proto_client::ClientWriteRequest { + client_id: 1, + command: None, + }; + let core_req: ClientWriteRequest = to_core_write_req(proto_req); + assert!(core_req.command.is_none()); +} + +// ─── to_core_read_req ───────────────────────────────────────────────────────── + +#[test] +fn test_to_core_read_req_keys_moved() { + let keys = vec![Bytes::from("k1"), Bytes::from("k2")]; + let proto_req = proto_client::ClientReadRequest { + client_id: 3, + keys: keys.clone(), + consistency_policy: None, + }; + let core_req: ClientReadRequest = to_core_read_req(proto_req); + assert_eq!(core_req.client_id, 3); + assert_eq!(core_req.keys, keys); +} + +#[test] +fn test_to_core_read_req_no_policy_becomes_none() { + let proto_req = proto_client::ClientReadRequest { + client_id: 1, + keys: vec![], + consistency_policy: None, + }; + let core_req: ClientReadRequest = to_core_read_req(proto_req); + // None → use cluster default; must NOT silently assume LeaseRead + assert!(core_req.consistency_policy.is_none()); +} + +#[test] +fn test_to_core_read_req_lease_read_policy_converts() { + let proto_req = proto_client::ClientReadRequest { + client_id: 1, + keys: vec![], + consistency_policy: Some(proto_client::ReadConsistencyPolicy::LeaseRead as i32), + }; + let core_req: ClientReadRequest = to_core_read_req(proto_req); + assert_eq!( + core_req.consistency_policy, + Some(ReadConsistencyPolicy::LeaseRead) + ); +} + +#[test] +fn test_to_core_read_req_linearizable_policy_converts() { + let proto_req = proto_client::ClientReadRequest { + client_id: 1, + keys: vec![], + consistency_policy: Some(proto_client::ReadConsistencyPolicy::LinearizableRead as i32), + }; + let core_req: ClientReadRequest = to_core_read_req(proto_req); + assert_eq!( + core_req.consistency_policy, + Some(ReadConsistencyPolicy::LinearizableRead) + ); +} + +// ─── ErrorCode bidirectional ────────────────────────────────────────────────── + +/// All 23 core::ErrorCode variants must survive a core→proto→core roundtrip. +/// This test catches any variant added to one side without updating the other. +#[test] +fn test_error_code_core_to_proto_to_core_roundtrip() { + let all_core = [ + ErrorCode::Success, + ErrorCode::ConnectionTimeout, + ErrorCode::InvalidAddress, + ErrorCode::LeaderChanged, + ErrorCode::JoinError, + ErrorCode::InvalidResponse, + ErrorCode::VersionMismatch, + ErrorCode::DiskFull, + ErrorCode::DataCorruption, + ErrorCode::StorageIoError, + ErrorCode::StoragePermissionDenied, + ErrorCode::KeyNotExist, + ErrorCode::NotLeader, + ErrorCode::StaleOperation, + ErrorCode::InvalidRequest, + ErrorCode::RateLimited, + ErrorCode::ClusterUnavailable, + ErrorCode::ProposeFailed, + ErrorCode::TermOutdated, + ErrorCode::RetryRequired, + ErrorCode::WatchBufferOverflow, + ErrorCode::General, + ErrorCode::Uncategorized, + ]; + + for code in all_core { + let proto = core_error_to_proto(code); + let back = proto_error_to_core(proto); + assert_eq!(back, code, "Roundtrip failed for {code:?}"); + } +} + +// ─── to_proto_response ──────────────────────────────────────────────────────── + +#[test] +fn test_to_proto_response_success_write() { + let core_resp = ClientResponse { + error: ErrorCode::Success, + leader_hint: None, + retry_after_ms: None, + result: Some(ClientResponsePayload::Write(WriteResult { + succeeded: true, + })), + }; + let proto_resp = to_proto_response(core_resp); + + assert_eq!(proto_resp.error, ProtoErrorCode::Success as i32); + assert!(proto_resp.metadata.is_none()); + assert!(matches!( + proto_resp.success_result, + Some(proto_client::client_response::SuccessResult::WriteResult( + proto_client::WriteResult { succeeded: true } + )) + )); +} + +#[test] +fn test_to_proto_response_success_read() { + let core_resp = ClientResponse { + error: ErrorCode::Success, + leader_hint: None, + retry_after_ms: None, + result: Some(ClientResponsePayload::Read(ReadResults { + entries: vec![KvEntry { + key: Bytes::from("k"), + value: Bytes::from("v"), + }], + })), + }; + let proto_resp = to_proto_response(core_resp); + + assert_eq!(proto_resp.error, ProtoErrorCode::Success as i32); + match proto_resp.success_result { + Some(proto_client::client_response::SuccessResult::ReadData(rd)) => { + assert_eq!(rd.results.len(), 1); + assert_eq!(rd.results[0].key, Bytes::from("k")); + assert_eq!(rd.results[0].value, Bytes::from("v")); + } + _ => panic!("expected ReadData"), + } +} + +#[test] +fn test_to_proto_response_not_leader_with_hint() { + let core_resp = ClientResponse { + error: ErrorCode::NotLeader, + leader_hint: Some(LeaderHint { + leader_id: 2, + address: "10.0.0.2:5002".into(), + }), + retry_after_ms: Some(100), + result: None, + }; + let proto_resp = to_proto_response(core_resp); + + assert_eq!(proto_resp.error, ProtoErrorCode::NotLeader as i32); + let meta = proto_resp.metadata.unwrap(); + assert_eq!(meta.retry_after_ms, Some(100)); + assert_eq!(meta.leader_id.as_deref(), Some("2")); + assert_eq!(meta.leader_address.as_deref(), Some("10.0.0.2:5002")); + assert!(proto_resp.success_result.is_none()); +} + +#[test] +fn test_to_proto_response_error_no_hint_has_no_metadata() { + let core_resp = ClientResponse { + error: ErrorCode::ClusterUnavailable, + leader_hint: None, + retry_after_ms: None, + result: None, + }; + let proto_resp = to_proto_response(core_resp); + assert!(proto_resp.metadata.is_none()); +} + +#[test] +fn test_to_proto_response_retry_without_hint_has_metadata() { + let core_resp = ClientResponse { + error: ErrorCode::RateLimited, + leader_hint: None, + retry_after_ms: Some(500), + result: None, + }; + let proto_resp = to_proto_response(core_resp); + let meta = proto_resp.metadata.unwrap(); + assert_eq!(meta.retry_after_ms, Some(500)); + assert!(meta.leader_id.is_none()); +} + +// ─── to_core_response ───────────────────────────────────────────────────────── + +#[test] +fn test_to_core_response_success_write() { + let proto_resp = proto_client::ClientResponse { + error: ProtoErrorCode::Success as i32, + metadata: None, + success_result: Some(proto_client::client_response::SuccessResult::WriteResult( + proto_client::WriteResult { succeeded: true }, + )), + }; + let core_resp = to_core_response(proto_resp); + assert_eq!(core_resp.error, ErrorCode::Success); + assert!(core_resp.leader_hint.is_none()); + assert!(matches!( + core_resp.result, + Some(ClientResponsePayload::Write(WriteResult { + succeeded: true + })) + )); +} + +#[test] +fn test_to_core_response_not_leader_with_metadata() { + use d_engine_proto::error::ErrorMetadata; + + let proto_resp = proto_client::ClientResponse { + error: ProtoErrorCode::NotLeader as i32, + metadata: Some(ErrorMetadata { + retry_after_ms: Some(200), + leader_id: Some("3".into()), + leader_address: Some("10.0.0.3:5003".into()), + debug_message: None, + }), + success_result: None, + }; + let core_resp = to_core_response(proto_resp); + assert_eq!(core_resp.error, ErrorCode::NotLeader); + assert_eq!(core_resp.retry_after_ms, Some(200)); + let hint = core_resp.leader_hint.unwrap(); + assert_eq!(hint.leader_id, 3); + assert_eq!(hint.address, "10.0.0.3:5003"); +} + +/// Unparseable leader_id string → LeaderHint is None (not a panic). +#[test] +fn test_to_core_response_bad_leader_id_yields_no_hint() { + use d_engine_proto::error::ErrorMetadata; + + let proto_resp = proto_client::ClientResponse { + error: ProtoErrorCode::NotLeader as i32, + metadata: Some(ErrorMetadata { + retry_after_ms: None, + leader_id: Some("not-a-number".into()), + leader_address: Some("10.0.0.3:5003".into()), + debug_message: None, + }), + success_result: None, + }; + let core_resp = to_core_response(proto_resp); + // Graceful degradation: bad leader_id string → no hint, no panic + assert!(core_resp.leader_hint.is_none()); +} diff --git a/d-engine-server/tests/cas_operations/leader_failover_cas_standalone.rs b/d-engine-server/tests/cas_operations/leader_failover_cas_standalone.rs index d06fe5c7..7258d9d2 100644 --- a/d-engine-server/tests/cas_operations/leader_failover_cas_standalone.rs +++ b/d-engine-server/tests/cas_operations/leader_failover_cas_standalone.rs @@ -1,7 +1,7 @@ use d_engine_client::Client; use d_engine_core::ClientApi; use d_engine_core::ClientApiError; -use d_engine_proto::error::ErrorCode; +use d_engine_core::client::ErrorCode; use std::time::Duration; use tempfile::TempDir; use tracing::info; diff --git a/d-engine-server/tests/client_manager/mod.rs b/d-engine-server/tests/client_manager/mod.rs index a941e790..7bd370fc 100644 --- a/d-engine-server/tests/client_manager/mod.rs +++ b/d-engine-server/tests/client_manager/mod.rs @@ -8,10 +8,10 @@ use d_engine_client::Client; use d_engine_client::ClientBuilder; use d_engine_core::ClientApi; use d_engine_core::ClientApiError; +use d_engine_core::client::ErrorCode; +use d_engine_core::config::ReadConsistencyPolicy; use d_engine_core::convert::safe_kv_bytes; use d_engine_core::convert::safe_vk; -use d_engine_proto::client::ReadConsistencyPolicy; -use d_engine_proto::error::ErrorCode; use d_engine_proto::server::cluster::NodeMeta; use tokio::time::sleep; use tracing::debug; diff --git a/d-engine-server/tests/common/mod.rs b/d-engine-server/tests/common/mod.rs index d761fac5..8de104ec 100644 --- a/d-engine-server/tests/common/mod.rs +++ b/d-engine-server/tests/common/mod.rs @@ -8,6 +8,7 @@ use d_engine_core::ClientApi; use d_engine_core::ClientApiError; use d_engine_core::alias::SMOF; use d_engine_core::alias::SOF; +use d_engine_core::client::ErrorCode; use d_engine_core::config::BackoffPolicy; use d_engine_core::config::ElectionConfig; use d_engine_core::config::FlushPolicy; @@ -20,7 +21,6 @@ use d_engine_core::convert::safe_kv_bytes; use d_engine_proto::client::WriteCommand; use d_engine_proto::common::Entry; use d_engine_proto::common::EntryPayload; -use d_engine_proto::error::ErrorCode; use d_engine_proto::server::election::VotedFor; use d_engine_server::FileStateMachine; use d_engine_server::FileStorageEngine; diff --git a/d-engine-server/tests/embedded_client/embedded_client_operations.rs b/d-engine-server/tests/embedded_client/embedded_client_operations.rs index 76779fa3..813d71c8 100644 --- a/d-engine-server/tests/embedded_client/embedded_client_operations.rs +++ b/d-engine-server/tests/embedded_client/embedded_client_operations.rs @@ -546,7 +546,7 @@ async fn test_put_with_ttl_readable_immediately() { /// LeaseRead should behave identically to LinearizableRead. #[tokio::test] async fn test_get_with_consistency_lease_read() { - use d_engine_proto::client::ReadConsistencyPolicy; + use d_engine_core::config::ReadConsistencyPolicy; let (engine, _temp_dir) = create_test_engine("lease_read").await; let client = engine.client(); diff --git a/d-engine-server/tests/readonly_and_learner_mode/learner_readonly_sync_standalone.rs b/d-engine-server/tests/readonly_and_learner_mode/learner_readonly_sync_standalone.rs index 6b53d102..93c3b556 100644 --- a/d-engine-server/tests/readonly_and_learner_mode/learner_readonly_sync_standalone.rs +++ b/d-engine-server/tests/readonly_and_learner_mode/learner_readonly_sync_standalone.rs @@ -14,7 +14,7 @@ use std::time::Duration; use d_engine_core::ClientApiError; -use d_engine_proto::client::ReadConsistencyPolicy; +use d_engine_core::config::ReadConsistencyPolicy; use tracing_test::traced_test; use crate::client_manager::ClientManager; diff --git a/d-engine-server/tests/watch_and_subscriptions/watch_events_grpc_standalone.rs b/d-engine-server/tests/watch_and_subscriptions/watch_events_grpc_standalone.rs index 5a3358bf..4dd0161a 100644 --- a/d-engine-server/tests/watch_and_subscriptions/watch_events_grpc_standalone.rs +++ b/d-engine-server/tests/watch_and_subscriptions/watch_events_grpc_standalone.rs @@ -12,8 +12,8 @@ use crate::common::node_config; use crate::common::start_node; use d_engine_client::ClientBuilder; use d_engine_core::ClientApi; +use d_engine_core::client::ErrorCode; use d_engine_proto::client::WatchResponse; -use d_engine_proto::error::ErrorCode; use d_engine_server::FileStateMachine; use d_engine_server::FileStorageEngine; use futures::StreamExt; diff --git a/examples/client-usage-standalone/src/main.rs b/examples/client-usage-standalone/src/main.rs index 9dbfe162..2c42d56d 100644 --- a/examples/client-usage-standalone/src/main.rs +++ b/examples/client-usage-standalone/src/main.rs @@ -3,7 +3,7 @@ use clap::Parser; use clap::Subcommand; use d_engine::{ Client, ClientApi, ClientApiError, ClientBuilder, - protocol::{ClientResult, ReadConsistencyPolicy}, + protocol::{KvEntry, ReadConsistencyPolicy}, }; #[derive(Parser)] @@ -93,7 +93,7 @@ async fn handle_read( .map_err(|e: ClientApiError| anyhow::anyhow!("Read error: {e:?}"))?; match result { - Some(ClientResult { key: _, value }) => { + Some(KvEntry { key: _, value }) => { let value = safe_vk(&value).unwrap(); println!("{value:?}"); } From fe1c3d21f9028f2b48434d0f85bc97446902c1c5 Mon Sep 17 00:00:00 2001 From: Joshua Chi <112539+JoshuaChi@users.noreply.github.com> Date: Fri, 22 May 2026 19:04:49 +0800 Subject: [PATCH 2/3] fix #260: address CodeRabbit review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue 1 (Minor) — types_test.rs: Add compile-time exhaustiveness guard for ErrorCode variants. The previous length-based assertion could be bypassed by adding a variant without updating the array; a wildcard-free match function now guarantees a compile error if any variant is unhandled. Issue 4 (Critical) — grpc_raft_service.rs: Validate the nested WriteCommand.operation field at the gRPC boundary. Previously only command.is_some() was checked; a request with WriteCommand { operation: None } passed validation and triggered an unreachable!() panic in proto_convert -- externally exploitable. Now a single check covers both the outer command and inner operation. Not addressed (pre-existing, separate tickets): - embedded_client.rs: retry_after_ms dropped in map_error_response - embedded_client.rs: non-read payloads silently coerced to None - grpc_raft_service.rs: invalid consistency_policy int -- intentional proto3 degradation to server default, not a bug --- d-engine-core/src/client/types_test.rs | 35 ++++++++++++++++--- .../src/network/grpc/grpc_raft_service.rs | 8 +++-- d-engine-server/src/proto_convert.rs | 6 ++-- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/d-engine-core/src/client/types_test.rs b/d-engine-core/src/client/types_test.rs index 80564390..149e8e62 100644 --- a/d-engine-core/src/client/types_test.rs +++ b/d-engine-core/src/client/types_test.rs @@ -4,9 +4,37 @@ use super::types::*; // ─── ErrorCode ──────────────────────────────────────────────────────────────── -/// Every ErrorCode variant must be reachable via an exhaustive match. -/// If a variant is added to proto without being added here, this test catches it -/// (because the proto ↔ core conversion will fail to compile). +/// Compile-time exhaustiveness guard: adding a new ErrorCode variant without +/// updating this match is a compile error, not a runtime surprise. +#[allow(dead_code)] +fn _assert_error_code_match_is_exhaustive(code: ErrorCode) { + match code { + ErrorCode::Success => {} + ErrorCode::ConnectionTimeout => {} + ErrorCode::InvalidAddress => {} + ErrorCode::LeaderChanged => {} + ErrorCode::JoinError => {} + ErrorCode::InvalidResponse => {} + ErrorCode::VersionMismatch => {} + ErrorCode::DiskFull => {} + ErrorCode::DataCorruption => {} + ErrorCode::StorageIoError => {} + ErrorCode::StoragePermissionDenied => {} + ErrorCode::KeyNotExist => {} + ErrorCode::NotLeader => {} + ErrorCode::StaleOperation => {} + ErrorCode::InvalidRequest => {} + ErrorCode::RateLimited => {} + ErrorCode::ClusterUnavailable => {} + ErrorCode::ProposeFailed => {} + ErrorCode::TermOutdated => {} + ErrorCode::RetryRequired => {} + ErrorCode::WatchBufferOverflow => {} + ErrorCode::General => {} + ErrorCode::Uncategorized => {} + } +} + #[test] fn test_error_code_all_variants_are_exhaustive() { let codes = [ @@ -34,7 +62,6 @@ fn test_error_code_all_variants_are_exhaustive() { ErrorCode::General, ErrorCode::Uncategorized, ]; - // 23 variants total — if the count changes, update this assertion and the proto .proto file assert_eq!(codes.len(), 23); } diff --git a/d-engine-server/src/network/grpc/grpc_raft_service.rs b/d-engine-server/src/network/grpc/grpc_raft_service.rs index b3439215..8d5887b8 100644 --- a/d-engine-server/src/network/grpc/grpc_raft_service.rs +++ b/d-engine-server/src/network/grpc/grpc_raft_service.rs @@ -412,8 +412,12 @@ where let request_future = async move { let proto_req: ClientWriteRequest = request.into_inner(); - if proto_req.command.is_none() { - return Err(Status::invalid_argument("Command cannot be empty")); + let operation_present = + proto_req.command.as_ref().and_then(|c| c.operation.as_ref()).is_some(); + if !operation_present { + return Err(Status::invalid_argument( + "WriteCommand must contain an operation", + )); } let core_req = proto_convert::to_core_write_req(proto_req); diff --git a/d-engine-server/src/proto_convert.rs b/d-engine-server/src/proto_convert.rs index 5583c079..a2d675f6 100644 --- a/d-engine-server/src/proto_convert.rs +++ b/d-engine-server/src/proto_convert.rs @@ -31,9 +31,9 @@ use d_engine_proto::error::{ErrorCode as ProtoErrorCode, ErrorMetadata}; /// Convert a proto `WriteCommand` to a core `WriteOperation`. /// -/// Callers **must** validate that the command is non-empty before calling this -/// (i.e. `wc.operation.is_some()`). An empty operation is a server-side bug, -/// not a client error, and will panic. +/// Panics if `wc.operation` is `None`. Callers must validate the nested +/// operation field before calling — the gRPC handler does this at the network +/// boundary via an explicit `invalid_argument` check. #[inline] pub(crate) fn write_command_to_op(wc: proto_client::WriteCommand) -> WriteOperation { match wc.operation { From b41dfa166312ef54cd9ff88c1808bed09e4fed30 Mon Sep 17 00:00:00 2001 From: Joshua Chi <112539+JoshuaChi@users.noreply.github.com> Date: Fri, 22 May 2026 22:47:13 +0800 Subject: [PATCH 3/3] fix(embedded-client) #389: forward server retry_after_ms, reject unexpected read payloads; warn on unknown consistency_policy --- d-engine-server/src/api/embedded_client.rs | 234 ++++++++++++++++-- .../src/network/grpc/grpc_raft_service.rs | 11 +- 2 files changed, 223 insertions(+), 22 deletions(-) diff --git a/d-engine-server/src/api/embedded_client.rs b/d-engine-server/src/api/embedded_client.rs index 892bd391..88efe686 100644 --- a/d-engine-server/src/api/embedded_client.rs +++ b/d-engine-server/src/api/embedded_client.rs @@ -27,7 +27,7 @@ use d_engine_core::RaftOneshot; use d_engine_core::ScanResult; use d_engine_core::client::{ ClientApi, ClientApiError, ClientApiResult, ClientReadRequest, ClientResponsePayload, - ClientWriteRequest, ErrorCode, LeaderHint, WriteOperation, + ClientWriteRequest, ErrorCode, LeaderHint, ReadResults, WriteOperation, }; use d_engine_core::config::ReadConsistencyPolicy; use tokio::sync::mpsc; @@ -60,6 +60,7 @@ fn timeout_error(duration: Duration) -> ClientApiError { fn not_leader_error( leader_id: Option, leader_address: Option, + retry_after_ms: Option, ) -> ClientApiError { let message = match (&leader_address, &leader_id) { (Some(addr), _) => format!("Not leader, try leader at: {addr}"), @@ -78,7 +79,7 @@ fn not_leader_error( ClientApiError::Network { code: ErrorCode::NotLeader, message, - retry_after_ms: Some(100), + retry_after_ms: retry_after_ms.or(Some(100)), leader_hint, } } @@ -91,6 +92,28 @@ fn server_error(msg: String) -> ClientApiError { } } +/// Unwrap a `ClientResponsePayload` as a `ReadResults`, returning a +/// `Protocol { InvalidResponse }` error for any other variant. +/// +/// Centralises the match so both `get_with_consistency` and +/// `get_multi_with_consistency` share identical error semantics, and so +/// the logic can be unit-tested without standing up a full Raft channel. +fn extract_read_payload(result: Option) -> ClientApiResult { + match result { + Some(ClientResponsePayload::Read(r)) => Ok(r), + Some(ClientResponsePayload::Write(_)) => Err(ClientApiError::Protocol { + code: ErrorCode::InvalidResponse, + message: "expected ReadData payload, got WriteResult".to_string(), + supported_versions: None, + }), + None => Err(ClientApiError::Protocol { + code: ErrorCode::InvalidResponse, + message: "expected ReadData payload, got None".to_string(), + supported_versions: None, + }), + } +} + /// Zero-overhead KV client for embedded mode. /// /// Directly calls Raft core without gRPC overhead. @@ -135,6 +158,7 @@ impl EmbeddedClient { fn map_error_response( error: ErrorCode, leader_hint: Option, + retry_after_ms: Option, ) -> ClientApiError { match error { ErrorCode::NotLeader => { @@ -143,7 +167,7 @@ impl EmbeddedClient { } else { (None, None) }; - not_leader_error(leader_id, leader_address) + not_leader_error(leader_id, leader_address, retry_after_ms) } _ => server_error(format!("Error code: {error:?}")), } @@ -187,6 +211,7 @@ impl EmbeddedClient { return Err(Self::map_error_response( response.error, response.leader_hint, + response.retry_after_ms, )); } @@ -291,15 +316,12 @@ impl EmbeddedClient { return Err(Self::map_error_response( response.error, response.leader_hint, + response.retry_after_ms, )); } - match response.result { - Some(ClientResponsePayload::Read(read_results)) => { - Ok(read_results.entries.first().map(|e| e.value.clone())) - } - _ => Ok(None), - } + let read_results = extract_read_payload(response.result)?; + Ok(read_results.entries.first().map(|e| e.value.clone())) } /// Get multiple keys with linearizable consistency. @@ -367,21 +389,17 @@ impl EmbeddedClient { return Err(Self::map_error_response( response.error, response.leader_hint, + response.retry_after_ms, )); } - match response.result { - Some(ClientResponsePayload::Read(read_results)) => { - // Reconstruct result vector in requested key order. - // Server only returns results for keys that exist, so we must - // map by key to preserve positional correspondence with input. - let results_by_key: std::collections::HashMap<_, _> = - read_results.entries.into_iter().map(|e| (e.key, e.value)).collect(); - - Ok(keys.iter().map(|k| results_by_key.get(k).cloned()).collect()) - } - _ => Ok(vec![None; keys.len()]), - } + let read_results = extract_read_payload(response.result)?; + // Reconstruct result vector in requested key order. + // Server only returns results for keys that exist, so we must + // map by key to preserve positional correspondence with input. + let results_by_key: std::collections::HashMap<_, _> = + read_results.entries.into_iter().map(|e| (e.key, e.value)).collect(); + Ok(keys.iter().map(|k| results_by_key.get(k).cloned()).collect()) } /// Delete a key-value pair with strong consistency. @@ -419,6 +437,7 @@ impl EmbeddedClient { return Err(Self::map_error_response( response.error, response.leader_hint, + response.retry_after_ms, )); } @@ -712,6 +731,7 @@ impl ClientApi for EmbeddedClient { return Err(Self::map_error_response( response.error, response.leader_hint, + response.retry_after_ms, )); } @@ -773,6 +793,7 @@ impl ClientApi for EmbeddedClient { return Err(Self::map_error_response( response.error, response.leader_hint, + response.retry_after_ms, )); } @@ -835,3 +856,174 @@ impl ClientApi for EmbeddedClient { self.scan_prefix(prefix).await } } + +#[cfg(test)] +mod error_helper_tests { + use d_engine_core::client::KvEntry; + + use super::*; + + // ─── not_leader_error ──────────────────────────────────────────────────── + + #[test] + fn test_not_leader_uses_server_retry_after_ms_when_provided() { + let err = not_leader_error( + Some("1".to_string()), + Some("127.0.0.1:5001".to_string()), + Some(500), + ); + match err { + ClientApiError::Network { retry_after_ms, .. } => { + assert_eq!(retry_after_ms, Some(500)); + } + _ => panic!("expected Network error"), + } + } + + #[test] + fn test_not_leader_falls_back_to_100ms_when_server_provides_none() { + let err = not_leader_error(None, None, None); + match err { + ClientApiError::Network { retry_after_ms, .. } => { + assert_eq!(retry_after_ms, Some(100)); + } + _ => panic!("expected Network error"), + } + } + + #[test] + fn test_not_leader_zero_is_not_treated_as_none() { + // Some(0) is an explicit server instruction, not absent — must be preserved + let err = not_leader_error(None, None, Some(0)); + match err { + ClientApiError::Network { retry_after_ms, .. } => { + assert_eq!(retry_after_ms, Some(0)); + } + _ => panic!("expected Network error"), + } + } + + // ─── map_error_response ────────────────────────────────────────────────── + + #[test] + fn test_map_error_response_not_leader_forwards_retry_after_ms() { + let err = EmbeddedClient::map_error_response( + ErrorCode::NotLeader, + Some(LeaderHint { + leader_id: 2, + address: "10.0.0.2:5002".into(), + }), + Some(250), + ); + match err { + ClientApiError::Network { + code, + retry_after_ms, + leader_hint, + .. + } => { + assert_eq!(code, ErrorCode::NotLeader); + assert_eq!(retry_after_ms, Some(250)); + let h = leader_hint.unwrap(); + assert_eq!(h.leader_id, 2); + } + _ => panic!("expected Network error"), + } + } + + #[test] + fn test_map_error_response_not_leader_falls_back_to_100ms_when_none() { + let err = EmbeddedClient::map_error_response(ErrorCode::NotLeader, None, None); + match err { + ClientApiError::Network { retry_after_ms, .. } => { + assert_eq!(retry_after_ms, Some(100)); + } + _ => panic!("expected Network error"), + } + } + + // ─── extract_read_payload ──────────────────────────────────────────────── + // + // These tests call `extract_read_payload` directly — the actual function + // used by both `get_with_consistency` and `get_multi_with_consistency`. + // This ensures the error behaviour is tested at the implementation site, + // not via a copy of the match logic that could silently drift. + + #[test] + fn test_extract_read_payload_returns_read_results_on_success() { + // Happy path: a well-formed Read payload must be unwrapped without error. + let entries = vec![KvEntry { + key: Bytes::from("k"), + value: Bytes::from("v"), + }]; + let payload = Some(ClientResponsePayload::Read(ReadResults { + entries: entries.clone(), + })); + + let result = extract_read_payload(payload).unwrap(); + assert_eq!(result.entries.len(), 1); + assert_eq!(result.entries[0].key, Bytes::from("k")); + assert_eq!(result.entries[0].value, Bytes::from("v")); + } + + #[test] + fn test_extract_read_payload_rejects_write_result_payload() { + // A WriteResult inside a read response is a server protocol violation. + // It must surface as InvalidResponse, not silently become "key not found". + let payload = Some(ClientResponsePayload::Write( + d_engine_core::client::WriteResult { succeeded: true }, + )); + + let err = extract_read_payload(payload).unwrap_err(); + assert_eq!(err.code(), ErrorCode::InvalidResponse); + assert!( + err.message().contains("WriteResult"), + "error message should identify the unexpected variant; got: {}", + err.message() + ); + } + + #[test] + fn test_extract_read_payload_rejects_none_payload() { + // A Success response with no payload is a protocol violation. + // It must surface as InvalidResponse, not silently become "key not found". + let err = extract_read_payload(None).unwrap_err(); + assert_eq!(err.code(), ErrorCode::InvalidResponse); + assert!( + err.message().contains("None"), + "error message should identify missing payload; got: {}", + err.message() + ); + } + + #[test] + fn test_extract_read_payload_empty_entries_is_valid() { + // An empty ReadResults is a legitimate response (no keys matched). + // It must not be treated as an error. + let payload = Some(ClientResponsePayload::Read(ReadResults { entries: vec![] })); + + let result = extract_read_payload(payload).unwrap(); + assert!(result.entries.is_empty()); + } + + #[test] + fn test_extract_read_payload_multiple_entries_are_preserved() { + // All entries in the ReadResults must be passed through unchanged. + let payload = Some(ClientResponsePayload::Read(ReadResults { + entries: vec![ + KvEntry { + key: Bytes::from("k1"), + value: Bytes::from("v1"), + }, + KvEntry { + key: Bytes::from("k2"), + value: Bytes::from("v2"), + }, + ], + })); + + let result = extract_read_payload(payload).unwrap(); + assert_eq!(result.entries.len(), 2); + assert_eq!(result.entries[1].key, Bytes::from("k2")); + } +} diff --git a/d-engine-server/src/network/grpc/grpc_raft_service.rs b/d-engine-server/src/network/grpc/grpc_raft_service.rs index 8d5887b8..467d11c3 100644 --- a/d-engine-server/src/network/grpc/grpc_raft_service.rs +++ b/d-engine-server/src/network/grpc/grpc_raft_service.rs @@ -458,7 +458,16 @@ where return Err(Status::unavailable("Service is not ready")); } - let core_req = proto_convert::to_core_read_req(request.into_inner()); + let proto_req = request.into_inner(); + if let Some(raw) = proto_req.consistency_policy + && d_engine_proto::client::ReadConsistencyPolicy::try_from(raw).is_err() + { + warn!( + raw_value = raw, + "Unknown consistency_policy value received, degrading to cluster default" + ); + } + let core_req = proto_convert::to_core_read_req(proto_req); let (resp_tx, resp_rx) = MaybeCloneOneshot::new(); self.cmd_tx .send(d_engine_core::ClientCmd::Read(core_req, resp_tx))