feat #167: workspace structure - #175
Conversation
…2/ migrate test-utils into d-engine-test-utils, waiting for fix all compile errors
…ready. And make d-engine-core/test_utils as feature.
… creating integration tests folder inside runtime
… creating integration tests folder inside runtime
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request restructures the project from a single crate to a multi-crate Rust workspace with separated proto, client, core, and server implementations. It reorganizes Cargo manifests, proto definitions, updates CI toolchain and caching, expands the Makefile, promotes numerous internal APIs to public, refactors error handling with unified error codes, and establishes new test infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant CB as ClientBuilder
participant CP as ConnectionPool
participant KvC as KvClient
participant RPC as gRPC Service
Client->>CB: builder(endpoints)
CB->>CB: configure (timeout, id)
CB->>CP: build pool
CB->>KvC: create with pool
CB-->>Client: Client { kv, cluster, ... }
Note over Client: Later: read/write operation
Client->>KvC: put(key, value)
KvC->>CP: get_connection()
CP->>RPC: RaftClientService.handle_client_write
RPC-->>CP: ClientResponse
CP-->>KvC: response
KvC->>KvC: validate_error via ClientResponseExt
KvC-->>Client: Result<bool>
sequenceDiagram
participant Test as Test Code
participant Mock as MockRpcService
participant Node as MockNode
participant Server as In-Process gRPC Server
Test->>Mock: new() / with_metadata_response
Test->>Node: mock_listener(mock_service)
Node->>Node: bind TCP 127.0.0.1:0
Node->>Node: health status updates
Node->>Server: spawn tonic server + services
Node-->>Test: (port, socket_addr)
Note over Test: Test creates channel to localhost:port
Test->>Test: mock_channel_with_port(port)
Test->>Server: gRPC call (read/write/discover)
Server->>Mock: route to RaftClientService/ClusterManagementService
Mock-->>Server: return configured response or error
Server-->>Test: gRPC response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
README.md (1)
79-84: Update the RocksDB example to match the new import paths.The RocksDB storage example still uses the old
d_engineimport path, which is inconsistent with the updated import path in the basic usage example (line 44). For consistency with the workspace restructuring, this should used_engine_server.Apply this diff to update the import path:
-use d_engine::{NodeBuilder, RocksDBStorageEngine, RocksDBStateMachine}; +use d_engine_server::{NodeBuilder, RocksDBStorageEngine, RocksDBStateMachine};d-engine-core/src/storage/storage_test.rs (1)
39-59: Fix undefinedFOLLOWERandLEARNERconstants in test setup.The
FOLLOWERandLEARNERconstants used at lines 39, 45, 51, and 57 are undefined. The imports provide enum variantsFollowerandLearner, which should be converted toi32instead. Replace each occurrence with the imported variant plus.into()conversion (e.g.,role: Follower.into(),androle: Learner.into(),).d-engine-core/src/utils/file_io.rs (1)
64-86: Avoid panicking inwrite_into_file.
Now that this helper ispub, every failed create/open/write ends inunwrap()→ panic, which is a correctness regression for callers that can hit I/O errors (permissions, ENOSPC, transient FS issues). Please bubble the errors back instead of aborting the process.Apply this diff to propagate errors safely:
-#[allow(dead_code)] -pub async fn write_into_file( - path: PathBuf, - buf: Vec<u8>, -) { +#[allow(dead_code)] +pub async fn write_into_file( + path: PathBuf, + buf: Vec<u8>, +) -> Result<()> { if let Some(parent) = path.parent() { - if let Err(e) = tokio::fs::create_dir_all(parent).await { - error!("failed to crate dir with error({})", e); - } else { - debug!("created successfully: {:?}", path); - } + tokio::fs::create_dir_all(parent) + .await + .map_err(StorageError::IoError)?; + debug!("created parent directories: {:?}", parent); } let file = tokio::fs::OpenOptions::new() .create(true) .append(true) - .open(path) - .await - .unwrap(); - let mut active_file = BufWriter::new(file); - active_file.write_all(&buf).await.unwrap(); - active_file.flush().await.unwrap(); + .open(&path) + .await + .map_err(StorageError::IoError)?; + let mut active_file = BufWriter::new(file); + active_file + .write_all(&buf) + .await + .map_err(StorageError::IoError)?; + active_file.flush().await.map_err(StorageError::IoError)?; + Ok(()) }d-engine-core/src/maybe_clone_oneshot.rs (2)
61-72: Fix missing Clone bound when using broadcast sender
broadcast::Sender::sendrequiresT: Clone. Because this impl is compiled under#[cfg(any(test, feature = "test-utils"))], the current signatureimpl<T: Send>does not satisfy the bound, so this block fails to compile as soon as a non-Clonetype is used (and the compiler will complain even if you never callsend). Tighten the impl to reflect the real requirement.-#[cfg(any(test, feature = "test-utils"))] -impl<T: Send> MaybeCloneOneshotSender<T> { +#[cfg(any(test, feature = "test-utils"))] +impl<T: Send + Clone> MaybeCloneOneshotSender<T> { pub fn send( &self, value: T, ) -> Result<usize, broadcast::error::SendError<T>> { if let Some(tx) = &self.test_inner { tx.send(value) } else { // Fallback for non-cloneable types panic!("Cannot broadcast non-cloneable type in tests"); } } }
118-137: Avoid tight loop in broadcast-backed FutureCalling
try_recvin a loop and immediatelywake_by_refcauses the task to be re-polled continuously while the buffer is empty, burning CPU and starving other work. You can instead poll the asyncrecv()future directly so the broadcast channel manages the waker for you.- if let Some(rx) = &mut this.test_inner { - match rx.try_recv() { - Ok(value) => Poll::Ready(Ok(value)), - Err(broadcast::error::TryRecvError::Empty) => { - // Register a Waker to wake up the task when data arrives - cx.waker().wake_by_ref(); - Poll::Pending - } - Err(broadcast::error::TryRecvError::Closed) => { - Poll::Ready(Err(broadcast::error::RecvError::Closed)) - } - Err(broadcast::error::TryRecvError::Lagged(n)) => { - Poll::Ready(Err(broadcast::error::RecvError::Lagged(n))) - } - } - } else { + if let Some(rx) = &mut this.test_inner { + let recv = rx.recv(); + tokio::pin!(recv); + recv.poll(cx) + } else { // Fallback for non-cloneable types panic!("Cannot broadcast non-cloneable type in tests"); }d-engine-core/src/replication/mod.rs (1)
352-385: RestoreAppendEntriesResponsehelpers (compile break).
response.is_conflict()/response.is_higher_term()are still invoked here, but the inherentimpl AppendEntriesResponsethat defined those helpers was removed in this PR. The prost-generated struct does not supply them, so the file no longer compiles (and any remaining callers of the constructors likeAppendEntriesResponse::successwill fail too). Please reinstate the helper impl or rewrite the call sites to pattern-match theresultfield.+use d_engine_proto::server::replication::append_entries_response; + +impl AppendEntriesResponse { + pub fn is_success(&self) -> bool { + matches!(self.result, Some(append_entries_response::Result::Success(_))) + } + + pub fn is_conflict(&self) -> bool { + matches!(self.result, Some(append_entries_response::Result::Conflict(_))) + } + + pub fn is_higher_term(&self) -> bool { + matches!(self.result, Some(append_entries_response::Result::HigherTerm(_))) + } +}d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)
1122-1138: Fix snapshot prefix slicing
parse_snapshot_dirnamestill slices at byte offset9, so any non-defaultsnapshots_dir_prefixfails to parse and old archives are never deleted. That defeats the new configuration knob and leaks snapshot files. Please derive the remainder from the actual prefix (e.g.,strip_prefix) before splitting.- if !name.starts_with(snapshot_dir_prefix) { - return None; - } - - // Remove fixed parts - let core = &name[9..name.len()]; // "snapshot-".len() = 9, + let core = match name.strip_prefix(snapshot_dir_prefix) { + Some(core) => core, + None => return None, + };benches/d-engine-bench/src/main.rs (1)
121-133: Guard against key_size overflow in prefixed keys
10u64.pow(key_size as u32)overflows oncekey_size > 19, which now makes the bench crash in debug (panic) and silently wrap in release. The previous generator handled arbitrarily large widths, so this is a regression. Please handle the overflow case before callingpow.- if sequential { - let max_value = 10u64.pow(key_size as u32) - 1; - let value = max_value.saturating_sub(index); - format!("{:0width$}", value, width = key_size) + if sequential { + let max_value = match 10u64.checked_pow(key_size as u32) { + Some(v) => v - 1, + None => return "0".repeat(key_size), + }; + let value = max_value.saturating_sub(index); + format!("{:0width$}", value, width = key_size)d-engine-core/src/test_utils/mock/mock_rpc_service.rs (1)
34-112: Significant code duplication with d-engine-client mock implementation.The
mock_listenermethod and severalsimulate_*helpers are duplicated betweend-engine-client/src/mock_rpc_service.rsand this file, with only minor differences (additional services in core version).Consider:
- Consolidating shared mock infrastructure into a common test-utils crate
- Having the client mock compose/reuse the core mock
- Using feature flags to optionally include additional services
This would reduce maintenance burden and ensure consistency across test infrastructure.
Also applies to: 173-305
d-engine-core/src/raft_role/leader_state.rs (1)
2078-2095: Leadership loss result is dropped
verify_leadership_limited_retryreturnsOk(false)when leadership is lost, but we immediately?it and treat that as success. That clears the pending queue and prints “Promotion successful” even though nothing was committed. We need to surface thefalsecase as an error (and likely re-queue the learners) so membership does not drift.self.verify_leadership_limited_retry( vec![EntryPayload::config(change)], true, ctx, role_tx, - ) - .await?; + ).await?; + + if !self + .verify_leadership_limited_retry(vec![EntryPayload::config(change)], true, ctx, role_tx) + .await? + { + return Err(NetworkError::TaskBackoffFailed( + "Batch promotion aborted: leadership could not be confirmed".to_string(), + ) + .into()); + }
🧹 Nitpick comments (11)
d-engine-core/src/test_utils/entry_builder_test.rs (1)
115-120: Consider verifying payload content in addition to presence.The tests currently verify that
payload.is_some()for various data inputs (empty, large, binary), which confirms the entry structure is correct. For more thorough validation, consider adding assertions that check the payload content matches the input data, especially for edge cases like empty and binary data.Example enhancement for
test_entry_builder_command_empty_data:let (_, entry) = builder.command(b""); assert!(entry.payload.is_some()); // Optional: verify empty data is preserved if let Some(EntryPayload { payload: Some(d_engine_proto::common::entry_payload::Payload::Command(data)) }) = &entry.payload { assert_eq!(data.len(), 0); }Also applies to: 123-129, 166-172
d-engine-core/src/test_utils/snapshot.rs (2)
57-57: Extract hardcoded size limit to a named constant.The 1GB limit is duplicated and lacks documentation. Consider extracting to a module-level constant.
Add a constant at the top of the file:
/// Maximum message size for test snapshot streams (1GB) const TEST_SNAPSHOT_MAX_SIZE: usize = 1024 * 1024 * 1024;Then replace both occurrences:
- Some(1024 * 1024 * 1024), + Some(TEST_SNAPSHOT_MAX_SIZE),Also applies to: 133-133
175-176: Remove redundant imports.
AsyncWriteExtis already imported at the module level (line 13). Consider movingGzipEncoderto module-level imports as well since it's used in multiple functions.Apply this diff:
- use async_compression::tokio::write::GzipEncoder; - use tokio::io::AsyncWriteExt;And optionally add to module-level imports (around line 5):
use async_compression::tokio::write::GzipEncoder;d-engine-core/src/utils/file_io_test.rs (3)
168-170: Document the safety of the unsafe block.The
unsafeblock usesflockwithout explaining why this is safe. Add a safety comment documenting that the file descriptor is valid and the flock call is used correctly.Apply this diff to add safety documentation:
- // Apply file lock - unsafe { - flock(file.as_raw_fd(), LOCK_EX); - } + // Apply file lock + // SAFETY: file.as_raw_fd() returns a valid file descriptor from the open file handle, + // and flock is called with a valid lock operation (LOCK_EX). + unsafe { + flock(file.as_raw_fd(), LOCK_EX); + }
135-183: Consider clarifying the test name and behavior.The test name
test_delete_busy_filemight be misleading. On Unix systems, file locks (viaflock) don't prevent deletion—the directory entry is removed, though the file data persists until all handles close. The test correctly verifies this platform-specific behavior (success on Unix, failure on Windows), but the name "busy file" doesn't fully capture this nuance.Consider renaming to something like
test_delete_locked_file_platform_behavioror adding a comment explaining the expected platform differences.
438-438: Consider a more efficient approach for generating random test data.The current approach generates 5MB of random data element-by-element. For better test performance, consider using
rand::thread_rng().fill_bytes()with a pre-allocated buffer.Example refactor:
- // Generate 5MB of random data - let data: Vec<u8> = (0..5 * 1024 * 1024).map(|_| rand::random::<u8>()).collect(); + // Generate 5MB of random data + let mut data = vec![0u8; 5 * 1024 * 1024]; + rand::thread_rng().fill_bytes(&mut data);d-engine-proto/Cargo.toml (1)
17-23: Clarify the prost dependency configuration.The commented workspace dependency for
prost(line 18) followed by a direct version specification (line 19) is inconsistent with other dependencies that useworkspace = true. This could lead to version conflicts within the workspace.Please clarify whether the direct prost version is intentional. If not, consider using the workspace dependency:
-# prost = { workspace = true } -prost = { version = "0.13", features = ["derive"] } +prost = { workspace = true, features = ["derive"] }If the direct version is required for specific reasons (e.g., feature flags not in workspace), please document this with a comment.
d-engine-client/src/mock_rpc_service.rs (2)
43-43: Replace unwrap() calls with proper error propagation in test infrastructure.Using
unwrap()in test helper functions can lead to unclear panic messages. Use?orexpect()with descriptive messages instead.Apply this diff:
- let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let listener = TcpListener::bind("127.0.0.1:0") + .await + .map_err(|e| tonic::Status::internal(format!("Failed to bind listener: {e}")))?; let addr = listener .local_addr() .map_err(|e| tonic::Status::internal(format!("Failed to bind: {e}")))?;And:
) - .await - .unwrap(); + .await + .map_err(|e| tracing::error!("Mock server failed: {e:?}")) + .ok();Also applies to: 77-77
99-110: Extract duplicate default metadata response builder.The same default metadata response builder is duplicated across four methods. Extract it into a shared helper function.
Add a helper method to
MockNode:impl MockNode { fn default_metadata_builder() -> Box<dyn Fn(u16) -> std::result::Result<ClusterMembership, tonic::Status> + Send + Sync> { Box::new(|port: u16| { Ok(ClusterMembership { version: 1, nodes: vec![NodeMeta { id: 1, role: NodeRole::Leader.into(), address: format!("127.0.0.1:{port}"), status: NodeStatus::Active.into(), }], }) }) } }Then use it:
- let builder = metadata_response_builder.unwrap_or_else(|| { - Box::new(|port: u16| { - Ok(ClusterMembership { - version: 1, - nodes: vec![NodeMeta { - id: 1, - role: NodeRole::Leader.into(), - address: format!("127.0.0.1:{port}",), - status: NodeStatus::Active.into(), - }], - }) - }) - }); + let builder = metadata_response_builder.unwrap_or_else(Self::default_metadata_builder);Also applies to: 128-140, 159-171, 190-201
d-engine-client/src/lib.rs (1)
162-182: Unnecessary mutable reference in refresh method.The
refreshmethod takes&mut selfbut only performs atomic operations viaArcSwap. The mutable reference is unnecessary and restrictive.Change the signature to use a shared reference:
- pub async fn refresh( - &mut self, - new_endpoints: Option<Vec<String>>, - ) -> std::result::Result<(), ClientApiError> { + pub async fn refresh( + &self, + new_endpoints: Option<Vec<String>>, + ) -> std::result::Result<(), ClientApiError> {This allows multiple concurrent callers to refresh the client configuration.
d-engine-core/src/test_utils/mock/mock_rpc_service.rs (1)
123-125: Trivial wrapper function.The
tcp_addr_to_http_addrfunction is a simple string format wrapper that could be inlined at call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
Cargo.lockis excluded by!**/*.lockbenches/d-engine-bench/Cargo.lockis excluded by!**/*.lockd-engine-client/Cargo.lockis excluded by!**/*.lockd-engine-proto/src/generated/d_engine.client.rsis excluded by!**/generated/**d-engine-proto/src/generated/d_engine.common.error.rsis excluded by!**/generated/**d-engine-proto/src/generated/d_engine.common.rsis excluded by!**/generated/**d-engine-proto/src/generated/d_engine.error.rsis excluded by!**/generated/**d-engine-proto/src/generated/d_engine.server.cluster.rsis excluded by!**/generated/**d-engine-proto/src/generated/d_engine.server.election.rsis excluded by!**/generated/**d-engine-proto/src/generated/d_engine.server.replication.rsis excluded by!**/generated/**d-engine-proto/src/generated/d_engine.server.storage.rsis excluded by!**/generated/**examples/client_usage/Cargo.lockis excluded by!**/*.lockexamples/rocksdb-cluster/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
.github/workflows/ci.yml(1 hunks)Cargo.toml(1 hunks)Makefile(1 hunks)README.md(1 hunks)benches/d-engine-bench/Cargo.toml(1 hunks)benches/d-engine-bench/src/main.rs(6 hunks)clippy.toml(1 hunks)config/base/raft.toml(1 hunks)d-engine-client/Cargo.toml(1 hunks)d-engine-client/src/builder.rs(1 hunks)d-engine-client/src/cluster.rs(2 hunks)d-engine-client/src/cluster_test.rs(5 hunks)d-engine-client/src/config.rs(1 hunks)d-engine-client/src/error.rs(8 hunks)d-engine-client/src/kv.rs(3 hunks)d-engine-client/src/kv_test.rs(8 hunks)d-engine-client/src/lib.rs(1 hunks)d-engine-client/src/mock_rpc.rs(1 hunks)d-engine-client/src/mock_rpc_service.rs(1 hunks)d-engine-client/src/pool.rs(3 hunks)d-engine-client/src/pool_test.rs(4 hunks)d-engine-client/src/proto/client_ext.rs(1 hunks)d-engine-client/src/proto/mod.rs(1 hunks)d-engine-client/src/utils.rs(1 hunks)d-engine-core/Cargo.toml(1 hunks)d-engine-core/src/commit_handler/default_commit_handler.rs(3 hunks)d-engine-core/src/commit_handler/default_commit_handler_test.rs(32 hunks)d-engine-core/src/commit_handler/mod.rs(1 hunks)d-engine-core/src/config/cluster.rs(2 hunks)d-engine-core/src/config/config_test.rs(2 hunks)d-engine-core/src/config/mod.rs(1 hunks)d-engine-core/src/config/network_test.rs(1 hunks)d-engine-core/src/config/raft.rs(7 hunks)d-engine-core/src/config/tls_test.rs(2 hunks)d-engine-core/src/election/election_handler.rs(4 hunks)d-engine-core/src/election/election_handler_test.rs(3 hunks)d-engine-core/src/election/mod.rs(2 hunks)d-engine-core/src/event.rs(4 hunks)d-engine-core/src/lib.rs(2 hunks)d-engine-core/src/maybe_clone_oneshot.rs(15 hunks)d-engine-core/src/maybe_clone_oneshot_test.rs(1 hunks)d-engine-core/src/membership.rs(6 hunks)d-engine-core/src/network/backgroup_snapshot_transfer.rs(1 hunks)d-engine-core/src/network/backgroup_snapshot_transfer_test.rs(2 hunks)d-engine-core/src/network/mod.rs(7 hunks)d-engine-core/src/purge/default_executor.rs(2 hunks)d-engine-core/src/purge/default_executor_test.rs(1 hunks)d-engine-core/src/purge/mod.rs(2 hunks)d-engine-core/src/raft.rs(11 hunks)d-engine-core/src/raft_context.rs(3 hunks)d-engine-core/src/raft_role/candidate_state.rs(12 hunks)d-engine-core/src/raft_role/follower_state.rs(9 hunks)d-engine-core/src/raft_role/leader_state.rs(26 hunks)d-engine-core/src/raft_role/learner_state.rs(9 hunks)d-engine-core/src/raft_role/mod.rs(6 hunks)d-engine-core/src/raft_role/role_state.rs(3 hunks)d-engine-core/src/replication/mod.rs(2 hunks)d-engine-core/src/replication/replication_handler.rs(7 hunks)d-engine-core/src/state_machine_handler/default_state_machine_handler.rs(9 hunks)d-engine-core/src/state_machine_handler/default_state_machine_handler_test.rs(19 hunks)d-engine-core/src/state_machine_handler/mod.rs(4 hunks)d-engine-core/src/state_machine_handler/snapshot_assembler.rs(2 hunks)d-engine-core/src/state_machine_handler/snapshot_assembler_test.rs(1 hunks)d-engine-core/src/state_machine_handler/snapshot_policy/composite_test.rs(2 hunks)d-engine-core/src/state_machine_handler/snapshot_policy/log_size.rs(1 hunks)d-engine-core/src/state_machine_handler/snapshot_policy/log_size_test.rs(7 hunks)d-engine-core/src/state_machine_handler/snapshot_policy/mod.rs(2 hunks)d-engine-core/src/state_machine_handler/snapshot_policy/time_based_test.rs(4 hunks)d-engine-core/src/storage/mod.rs(1 hunks)d-engine-core/src/storage/raft_log.rs(1 hunks)d-engine-core/src/storage/snapshot_path_manager.rs(1 hunks)d-engine-core/src/storage/state_machine.rs(1 hunks)d-engine-core/src/storage/state_machine_test.rs(1 hunks)d-engine-core/src/storage/storage_engine.rs(3 hunks)d-engine-core/src/storage/storage_engine_test.rs(2 hunks)d-engine-core/src/storage/storage_test.rs(1 hunks)d-engine-core/src/test_utils/common.rs(4 hunks)d-engine-core/src/test_utils/common_test.rs(1 hunks)d-engine-core/src/test_utils/entry_builder.rs(1 hunks)d-engine-core/src/test_utils/entry_builder_test.rs(1 hunks)d-engine-core/src/test_utils/mock/mock_raft_builder.rs(4 hunks)d-engine-core/src/test_utils/mock/mock_rpc.rs(2 hunks)d-engine-core/src/test_utils/mock/mock_rpc_service.rs(13 hunks)d-engine-core/src/test_utils/mock/mock_storage_engine.rs(0 hunks)d-engine-core/src/test_utils/mock/mock_type_config.rs(1 hunks)d-engine-core/src/test_utils/mock/mod.rs(1 hunks)d-engine-core/src/test_utils/mod.rs(1 hunks)d-engine-core/src/test_utils/snapshot.rs(3 hunks)d-engine-core/src/utils/cluster.rs(1 hunks)d-engine-core/src/utils/convert.rs(2 hunks)d-engine-core/src/utils/convert_test.rs(0 hunks)d-engine-core/src/utils/file_io.rs(6 hunks)d-engine-core/src/utils/file_io_test.rs(2 hunks)d-engine-core/src/utils/mod.rs(1 hunks)d-engine-core/src/utils/scoped_timer.rs(1 hunks)d-engine-core/src/utils/stream.rs(2 hunks)d-engine-core/src/utils/time_test.rs(1 hunks)d-engine-docs/Cargo.toml(1 hunks)d-engine-docs/src/docs/architecture/single-responsibility-principle.md(4 hunks)d-engine-docs/src/docs/architecture/snapshot-module-design.md(1 hunks)d-engine-docs/src/docs/overview.md(1 hunks)d-engine-docs/src/lib.rs(1 hunks)d-engine-proto/Cargo.toml(1 hunks)d-engine-proto/build.rs(1 hunks)d-engine-proto/proto/client/client_api.proto(2 hunks)d-engine-proto/proto/common.proto(2 hunks)d-engine-proto/proto/error.proto(4 hunks)
⛔ Files not processed due to max files limit (52)
- d-engine-proto/proto/server/cluster.proto
- d-engine-proto/proto/server/election.proto
- d-engine-proto/proto/server/replication.proto
- d-engine-proto/proto/server/storage.proto
- d-engine-proto/src/exts/client_ext.rs
- d-engine-proto/src/exts/client_ext_test.rs
- d-engine-proto/src/exts/cluster_ext.rs
- d-engine-proto/src/exts/cluster_ext_test.rs
- d-engine-proto/src/exts/common_ext.rs
- d-engine-proto/src/exts/common_ext_test.rs
- d-engine-proto/src/exts/mod.rs
- d-engine-proto/src/exts/replication_ext.rs
- d-engine-proto/src/exts/replication_ext_test.rs
- d-engine-proto/src/exts/storage_ext.rs
- d-engine-proto/src/exts/storage_ext_test.rs
- d-engine-proto/src/lib.rs
- d-engine-server/Cargo.toml
- d-engine-server/src/lib.rs
- d-engine-server/src/membership/membership_guard.rs
- d-engine-server/src/membership/membership_guard_test.rs
- d-engine-server/src/membership/mod.rs
- d-engine-server/src/membership/raft_membership.rs
- d-engine-server/src/membership/raft_membership_test.rs
- d-engine-server/src/network/connection_cache.rs
- d-engine-server/src/network/connection_cache_test.rs
- d-engine-server/src/network/grpc/grpc_raft_service.rs
- d-engine-server/src/network/grpc/grpc_raft_service_test.rs
- d-engine-server/src/network/grpc/grpc_transport.rs
- d-engine-server/src/network/grpc/grpc_transport_test.rs
- d-engine-server/src/network/grpc/mod.rs
- d-engine-server/src/network/health_checker.rs
- d-engine-server/src/network/health_checker_test.rs
- d-engine-server/src/network/mod.rs
- d-engine-server/src/network/network_test.rs
- d-engine-server/src/node/builder.rs
- d-engine-server/src/node/builder_test.rs
- d-engine-server/src/node/mod.rs
- d-engine-server/src/node/node_test.rs
- d-engine-server/src/node/type_config/mod.rs
- d-engine-server/src/node/type_config/raft_type_config.rs
- d-engine-server/src/storage/adaptors/file/file_engine_test.rs
- d-engine-server/src/storage/adaptors/file/file_state_machine.rs
- d-engine-server/src/storage/adaptors/file/file_state_machine_test.rs
- d-engine-server/src/storage/adaptors/file/file_storage_engine.rs
- d-engine-server/src/storage/adaptors/file/file_storage_engine_test.rs
- d-engine-server/src/storage/adaptors/rocksdb/rocksdb_engine_test.rs
- d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs
- d-engine-server/src/storage/adaptors/rocksdb/rocksdb_storage_engine.rs
- d-engine-server/src/storage/buffered/buffered_raft_log.rs
- d-engine-server/src/storage/buffered/buffered_raft_log_test.rs
- d-engine-server/src/storage/mod.rs
- d-engine-server/src/test_utils/integration/mod.rs
💤 Files with no reviewable changes (2)
- d-engine-core/src/test_utils/mock/mock_storage_engine.rs
- d-engine-core/src/utils/convert_test.rs
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 54-54: Target body for "help" exceeds allowed length of 5 (35).
(maxbodylength)
[warning] 365-365: Target body for "audit" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 374-374: Target body for "deny" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 406-406: Target body for "troubleshoot" exceeds allowed length of 5 (34).
(maxbodylength)
[warning] 18-18: Missing required phony target "clean"
(minphony)
[warning] 115-115: Target "install" should be declared PHONY.
(phonydeclared)
[warning] 355-355: Target "doc" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (100)
d-engine-core/src/utils/time_test.rs (1)
1-7: Import organization is improved.The reordering of the
get_now_as_u128import now follows a logical pattern (ascending numeric type size: u32 → u64 → u128), which enhances readability without functional impact. The overall test module structure is clean and well-organized.d-engine-docs/src/docs/architecture/snapshot-module-design.md (1)
174-190: Verify bracket escaping aligns with documentation build tool requirements.The changes escape square brackets in component references (e.g.,
[**StateMachine**]→\[**StateMachine**\]) in the "Generating a new snapshot", "Applying a snapshot", and "Cleaning up old snapshots" sections. This is applied consistently across all affected references.However, since bracket escaping requirements depend on the specific markdown renderer or documentation build system (e.g., mdx, docusaurus, standard markdown), please verify:
- Whether this escaping is required by your documentation build tool and not an artifact of a failed conversion
- Whether similar component references elsewhere in the documentation follow the same escaping pattern for consistency
- That the rendered output displays correctly (brackets should be visible in the final output, not hidden or malformed)
If you'd like, I can generate a script to validate markdown syntax consistency across the documentation directory and verify that bracket-escaping is uniformly applied.
d-engine-core/src/config/tls_test.rs (1)
4-4: LGTM! Cosmetic formatting improvements.The import reordering and assertion reformatting are purely stylistic changes that maintain identical functionality while improving code organization and readability.
Also applies to: 91-95
d-engine-core/src/config/network_test.rs (1)
1-1: LGTM! Import consolidation improves organization.Moving the
Errorimport to the top of the file eliminates duplication and follows Rust conventions for grouping imports together.d-engine-core/src/test_utils/entry_builder.rs (1)
3-5: LGTM! Import paths correctly updated for workspace structure.The migration from local proto paths (
crate::proto::common) to the sharedd_engine_protocrate aligns with the workspace restructuring. The imported types (Entry,EntryPayload,Change) are used consistently throughout the file.d-engine-core/src/test_utils/entry_builder_test.rs (1)
1-213: Excellent test coverage for EntryBuilder!The test suite is comprehensive and well-structured, covering:
- Basic operations (command, noop, config)
- Chaining and sequencing behavior
- Edge cases (empty/large data, boundary values for index/term, binary payloads)
- Mixed operation sequences
- State progression validation
The tests correctly verify index progression, term constancy, and payload presence across all operation types.
d-engine-core/src/commit_handler/default_commit_handler_test.rs (5)
27-32: LGTM! Clean migration to workspace proto crate.The import updates consistently migrate from crate-local
crate::prototo the sharedd_engine_protocrate, aligning with the workspace structure refactoring.
316-323: LGTM! Type-safe role initialization.The migration from raw constants to
Leader.into()improves type safety and code clarity by using theNodeRoleenum.
437-460: LGTM! Consistent enum usage in tests.The test correctly uses
Leader.into()for role conversions throughout, maintaining consistency with the workspace refactoring.
486-487: LGTM! Correct leadership transition test.The test appropriately uses both
Leader.into()andFollower.into()to verify that the handler correctly handles leadership loss during batch processing.
851-863: LGTM! Consistent refactoring in batch processing tests.All tests in the
process_batch_testmodule correctly useLeader.into()for role initialization, maintaining consistency with the workspace migration.d-engine-core/src/test_utils/snapshot.rs (2)
7-7: LGTM! Import changes align with workspace restructuring.The migration from internal proto paths to the
d_engine_protocrate is consistent with the PR's workspace refactoring objectives.Also applies to: 11-11, 21-23
138-160: LGTM! Promoting test utility to public API.Exposing
create_test_chunkas a public helper is reasonable for external test utilities.d-engine-core/src/utils/file_io_test.rs (5)
1-20: LGTM! Import organization is clean.The reorganized imports are all properly used throughout the test module.
186-224: LGTM! Permission test is well-structured.The test correctly sets up a permission-denied scenario and properly cleans up afterwards.
226-261: LGTM! Checksum conversion tests are comprehensive.Good coverage of valid input, boundary conditions, and edge cases.
263-381: LGTM! Compressed format validation tests are well-structured.The tests cover valid files, invalid extensions, invalid magic numbers, and empty files comprehensively.
383-516: LGTM! File checksum tests provide excellent coverage.The test suite thoroughly validates checksum computation for empty files, small files, large files, non-existent files, consistency checks, and content change detection.
README.md (1)
44-44: LGTM!The import path correctly reflects the new workspace structure where server-side components are now in the
d_engine_servercrate.d-engine-core/src/storage/snapshot_path_manager.rs (1)
3-3: LGTM!The import path correctly reflects the new workspace structure where proto definitions are centralized in the
d_engine_protocrate.d-engine-core/src/purge/default_executor_test.rs (1)
8-8: LGTM!The simplified import path indicates that test utilities are now more accessible, likely re-exported at the crate root for easier use across tests.
d-engine-client/src/builder.rs (1)
83-84: LGTM!The documentation correctly reflects the new workspace structure where client components are now in the dedicated
d_engine_clientcrate.d-engine-core/src/state_machine_handler/snapshot_policy/log_size.rs (1)
71-74: LGTM!Making the constructor public aligns with the PR objective to promote internal APIs for use across the workspace. The visibility change allows external crates to instantiate
LogSizePolicywith custom thresholds and cooldown periods.d-engine-core/src/storage/storage_test.rs (1)
4-8: LGTM!The import paths correctly reflect the workspace structure with proto types centralized in
d_engine_proto. The migration from crate-local paths to the dedicated proto crate is consistent with the overall refactoring.d-engine-core/src/utils/convert.rs (2)
1-2: LGTM!Import reordering and the addition of
prost::Messagesupport encoding functionality used in theskvfunction (line 51).
29-34: LGTM!The documentation example correctly reflects the new workspace structure where this utility is part of the
d_engine_corecrate.config/base/raft.toml (1)
55-55: Service name configuration is correct.The configured service name
d_engine.server.cluster.ClusterManagementServicematches the proto definition exactly:
- Package:
d_engine.server.cluster(fromgit/d-engine-proto/proto/server/cluster.protoline 2)- Service:
ClusterManagementService(fromgit/d-engine-proto/proto/server/cluster.protoline 120)No changes needed.
d-engine-client/src/proto/mod.rs (1)
1-2: LGTM!The module declaration and re-export pattern is idiomatic and correctly exposes the client extension API through the proto namespace.
d-engine-docs/src/docs/architecture/single-responsibility-principle.md (1)
8-151: LGTM!The formatting improvements enhance readability without changing the document's content. The code fence language change from
ignoretotextat line 122 is appropriate for the folder structure example.d-engine-core/src/purge/default_executor.rs (2)
9-10: LGTM!The import path updates correctly reflect the workspace reorganization, migrating from local proto paths to the centralized
d_engine_protocrate.
36-36: LGTM!Making the constructor public is appropriate for the workspace reorganization and allows external crates to instantiate
DefaultPurgeExecutor.d-engine-docs/src/lib.rs (1)
278-278: LGTM!The public module declaration correctly exposes the docs module for external access.
d-engine-core/src/utils/scoped_timer.rs (1)
1-23: LGTM! Clean RAII timing utility.The
ScopedTimerimplementation follows the standard RAII pattern for timing measurements. Usingtokio::time::Instantis appropriate for async contexts, and logging to the "timing" target enables easy filtering of timing data.d-engine-proto/Cargo.toml (2)
1-15: LGTM!Package metadata and docs.rs configuration are properly structured for a protocol definitions crate.
26-28: LGTM!Build dependencies are appropriate for a proto crate using tonic code generation and build metadata.
d-engine-proto/build.rs (1)
7-16: Proto file reorganization verified.All proto files exist at their new paths. The reorganization into
proto/server/andproto/client/directories with simplified naming is correct and improves separation of concerns.d-engine-docs/src/docs/overview.md (1)
27-30: Import paths are correct. All three symbols are properly exported from d_engine_server.Verification confirms that
NodeBuilder,FileStorageEngine, andFileStateMachineare all re-exported at the crate root of d_engine_server in lib.rs (lines 69, 73, and 75 respectively). The imports in the documentation are accurate.d-engine-core/src/purge/mod.rs (4)
1-1: LGTM! Documentation path updated to new workspace structure.The doc include path correctly references the new
d-engine-docslocation.
9-9: LGTM! Public re-export enables multi-crate workspace usage.Changing from
pub(crate)topubis intentional for the workspace refactor, allowing other crates to use the default executor.
14-14: LGTM! Test utilities feature gating expanded appropriately.Extending automock to
any(test, feature = "test-utils")allows other crates to use mocks when the test-utils feature is enabled.Also applies to: 28-28
19-19: LGTM! Proto import path migrated to workspace crate.The import correctly references
d_engine_proto::common::LogIdfrom the new proto crate.d-engine-core/src/network/backgroup_snapshot_transfer.rs (2)
11-11: LGTM! Import reorganization with no functional impact.The import paths have been reorganized but maintain the same functionality.
Also applies to: 16-16, 18-18
30-33: LGTM! Proto types migrated to workspace proto crate.The proto types are now correctly imported from
d_engine_proto::server::storage, aligning with the workspace structure refactor.d-engine-core/src/network/backgroup_snapshot_transfer_test.rs (2)
5-6: LGTM! Test utilities and proto imports updated.The imports correctly reference the new proto paths and test utilities from the workspace structure.
Also applies to: 19-22
24-46: LGTM! Well-structured test helper function.The
create_snapshot_streamhelper properly:
- Creates test chunks with sequential data
- Uses workspace test utilities (
create_test_chunk,crate_test_snapshot_stream)- Maps errors appropriately to
NetworkError::TonicStatusErrord-engine-core/src/utils/stream.rs (3)
13-13: LGTM! Import statement reorganized.The
ReceiverStreamimport has been repositioned with no functional impact.
49-53: LGTM! Default implementation provides good ergonomics.The
Defaulttrait implementation forGrpcStreamDecoder<T>appropriately delegates tonew(), making the API more convenient to use.
56-56: LGTM! Constructor visibility expanded for workspace usage.Making
new()public is intentional for the multi-crate workspace, allowing other crates to constructGrpcStreamDecoderinstances.d-engine-core/src/commit_handler/default_commit_handler.rs (3)
22-27: LGTM! Imports reorganized to use workspace proto crate.The imports now correctly reference
d_engine_proto::commontypes and internal aliases, aligning with the workspace structure.
117-117: LGTM! Constructor visibility expanded for workspace usage.Making the
new()constructor public is intentional for the multi-crate workspace refactor.
280-280: LGTM! Trace formatting simplified.The trace statement formatting has been consolidated to a single line with no functional impact.
d-engine-core/src/commit_handler/mod.rs (1)
58-58: LGTM! Test utilities feature gating expanded consistently.The automock configuration now applies when either
testorfeature = "test-utils"is enabled, supporting cross-crate test infrastructure.Also applies to: 64-64
.github/workflows/ci.yml (3)
41-42: LGTM! CI modernized with explicit Rust toolchain.The workflow now uses Rust 1.88.0 with explicit toolchain installation via
dtolnay/rust-toolchain, which is more maintainable than the previous approach.Also applies to: 48-52
54-55: LGTM! Cache strategy modernized.Switching to
Swatinem/rust-cacheis a better approach as it's specifically designed for Rust projects and is actively maintained.
60-63: LGTM! Cargo tools installation consolidated.The step has been renamed for clarity and still installs both
cargo-llvm-covandcargo-nextest.d-engine-core/src/storage/storage_engine.rs (3)
1-1: LGTM! Documentation path updated to workspace docs crate.The doc include path correctly references the new
d-engine-docslocation.
6-6: LGTM! Test utilities feature gating expanded for storage traits.The automock configuration for
LogStoreandMetaStorenow applies when eithertestorfeature = "test-utils"is enabled, supporting cross-crate test infrastructure.Also applies to: 36-36, 96-96
12-13: LGTM! Proto types migrated to workspace proto crate.The
EntryandLogIdtypes are now correctly imported fromd_engine_proto::common, aligning with the workspace structure refactor.d-engine-client/src/config.rs (1)
3-3: LGTM: Import path refactored.The utility function
get_now_as_u32has been relocated to theutilsmodule, which is a reasonable consolidation of utility functions.d-engine-proto/proto/common.proto (2)
2-2: LGTM: Package namespace updated.The package has been renamed to align with the new workspace structure and
d_engine_protocrate naming convention.
100-105: LGTM: Type-safe role enumeration.The new
NodeRoleenum provides a type-safe alternative to scalar role constants, improving code clarity and reducing the risk of invalid role values.d-engine-core/src/state_machine_handler/snapshot_assembler_test.rs (1)
10-11: LGTM: Proto imports migrated to external crate.The import paths have been updated to use the new
d_engine_protocrate, aligning with the workspace restructuring.d-engine-core/src/test_utils/mock/mock_type_config.rs (1)
10-10: LGTM: Explicit import improves clarity.Replacing the prelude import with an explicit import for
MockStorageEnginemakes the dependency clearer and more maintainable.benches/d-engine-bench/Cargo.toml (3)
3-3: LGTM: Version bump reflects major refactor.The version bump from
0.1.4to0.2.0appropriately reflects the significant workspace restructuring changes.
7-7: LGTM: Dependency updated to new client crate.The benchmark now depends on the new
d-engine-clientcrate, which is consistent with the workspace restructuring and provides access to the client API.
4-4: Rust edition 2024 is now stable.Rust Edition 2024 was released to stable with Rust 1.85 on February 20, 2025. The code change is valid and will not cause build failures.
Likely an incorrect or invalid review comment.
d-engine-client/src/pool.rs (2)
1-1: LGTM: Proto imports migrated to external crate.The import paths have been updated to use types from the
d_engine_protocrate, aligning with the workspace restructuring.Also applies to: 11-15
185-185: LGTM: Type-safe role comparison.The code now uses the typed
NodeRoleenum instead of a scalar constant, which improves type safety and makes the code more self-documenting. The.into()conversion properly converts the enum to the underlying protoi32representation.d-engine-core/src/state_machine_handler/snapshot_assembler.rs (1)
2-2: LGTM: Import paths refactored.The imports have been reorganized: added explicit
Arcimport, relocatedcreate_parent_dir_if_not_exist, and updatedSnapshotMetadatato use thed_engine_protocrate. These are straightforward refactoring changes with no functional impact.Also applies to: 19-20
d-engine-core/src/config/mod.rs (1)
39-39: LGTM: NodeStatus import migrated to external crate.The import path has been updated to use
d_engine_proto::common::NodeStatus, aligning with the workspace restructuring to separate proto definitions into their own crate.d-engine-proto/proto/error.proto (1)
2-2: LGTM! Package namespace migration is clean.The package rename from
raft.errortod_engine.erroraligns with the broader workspace restructuring. All error codes and metadata remain semantically unchanged.d-engine-core/src/state_machine_handler/snapshot_policy/time_based_test.rs (2)
7-10: LGTM! Import migration to proto-based types is clean.The import path updates correctly align with the new workspace structure, using
d_engine_protofor common types likeLogIdandNodeRole.
22-22: LGTM! NodeRole enum usage is correct.The conversion from the constant
LEADERtoLeader.into()properly leverages the new proto-based enum, maintaining test semantics while adopting the updated API.d-engine-core/src/state_machine_handler/snapshot_policy/log_size_test.rs (2)
8-10: LGTM! Proto import migration is consistent.The import updates for
LogIdandNodeRoletypes fromd_engine_protoalign with the workspace-wide restructuring.
35-129: LGTM! NodeRole enum conversion is applied consistently.The migration from role constants (
LEADER,FOLLOWER) to enum variants (Leader.into(),Follower.into()) is applied consistently across all test cases, maintaining test semantics.d-engine-client/src/kv_test.rs (2)
3-22: LGTM! Import migration to proto types is complete.The import updates correctly reference
d_engine_protofor all client and server types, aligning with the workspace restructure.
96-96: LGTM! Error code comparisons updated correctly.The error assertions now compare against
ErrorCodeenum variants rather than numeric values, providing better type safety and readability.Also applies to: 247-247, 427-427, 464-464, 744-744
d-engine-core/src/storage/state_machine_test.rs (1)
9-18: LGTM! Import migration is clean.The import path updates from
crate::prototod_engine_protofor Entry, EntryPayload, LogId, WriteCommand, and related types correctly align with the workspace restructuring.d-engine-core/src/config/config_test.rs (2)
9-11: LGTM! Correct usage of unsafe for environment variable removal.The
unsafeblock aroundstd::env::remove_varis necessary as this function is marked unsafe in recent Rust versions due to potential data races in concurrent environments.
217-218: LGTM! Import migration to proto types is correct.The imports for
NodeStatusandNodeMetanow correctly referenced_engine_protopaths, consistent with the workspace restructuring.d-engine-core/src/test_utils/mock/mock_rpc.rs (2)
8-32: LGTM! Mock RPC imports migrated correctly.The import paths have been systematically updated from
crate::prototod_engine_protofor all client, server, and service types, maintaining consistency with the workspace restructure.
219-221: LGTM! Syntax error fixed.The semicolon after the closing parenthesis on line 221 correctly terminates the return statement, fixing what would have been a compilation error.
d-engine-client/src/cluster.rs (2)
12-15: LGTM! Import migration to proto cluster types is clean.The imports for cluster management types now correctly reference
d_engine_proto::server::cluster, aligning with the workspace restructuring.
46-46: LGTM! Explicit return type improves API clarity.The return type is now explicitly
std::result::Result<Vec<NodeMeta>, ClientApiError>, making the public API signature clearer and more self-documenting.d-engine-core/src/maybe_clone_oneshot_test.rs (1)
1-227: Great async coverage on MaybeCloneOneshot.Appreciate the breadth of scenarios here—covering clones, futures, stress, and complex payloads gives strong confidence in the channel semantics. Nice work.
d-engine-client/src/kv.rs (1)
44-46: Docs now match ClientApiError variantsThanks for documenting Protocol and Storage outcomes; that lines up with the new ClientResponseExt handling.
d-engine-core/src/test_utils/mod.rs (1)
19-24: Handy node_config helperAppreciate having a reusable RaftNodeConfig initializer for tests.
d-engine-core/src/raft_role/role_state.rs (1)
398-400: More readable replication failure logThe reformatted error! call is easier to scan in logs when replication backoffs happen.
d-engine-core/src/test_utils/common.rs (1)
46-66: Expose common command generatorsMaking the insert/delete helpers public lets benches/tests share the encoding logic.
d-engine-core/src/utils/mod.rs (1)
1-15: Great to consolidate util modulesExplicitly exporting cluster/convert/etc. centralizes the namespace nicely.
d-engine-client/src/mock_rpc_service.rs (1)
183-211: Inconsistent visibility: method is public while similar methods are crate-private.The
simulate_mock_service_with_join_cluster_repsmethod ispubwhile othersimulate_*methods arepub(crate). Ensure visibility is intentional.Is this method intended to be public, or should it be
pub(crate)like the other simulation helpers?d-engine-core/src/raft_context.rs (2)
94-100: Good use of feature gating for test utilities.Expanding the cfg gate from
testtoany(test, feature = "test-utils")properly enables these methods for external test usage while keeping them out of production builds.Also applies to: 102-108
15-49: The review comment is incorrect and based on incomplete context.The suggestion to restrict visibility to
pub(crate)or mark with#[doc(hidden)]would break the codebase. d-engine-server (a sibling workspace crate) directly constructsRaftStorageHandlesandRaftCoreHandlersusing public field initialization in production code atd-engine-server/src/node/builder.rslines 323–331.These types are exported via
pub use raft_context::*;in lib.rs and are intentionally part of the cross-crate workspace API, not internal-only components. Making thempub(crate)would prevent d-engine-server from accessing them across crate boundaries.The codebase already provides accessor methods for read-only access where applicable, and field construction is a valid pattern for struct initialization. No changes are required.
Likely an incorrect or invalid review comment.
d-engine-core/src/raft_role/mod.rs (1)
28-30: Proto path migration and visibility expansion look good.The changes consistently migrate from
crate::prototod_engine_protopaths and appropriately expand visibility with proper feature gating for test utilities.Also applies to: 188-195, 226-253, 288-291, 342-346
d-engine-core/src/lib.rs (3)
2-56: Extensive public API surface expansion.Multiple internal modules are now publicly exposed. Ensure this aligns with the intended public API design for the workspace structure.
The expansion appears intentional for the workspace refactor, but verify that sensitive internal details aren't inadvertently exposed. Consider using
#[doc(hidden)]for workspace-internal APIs that shouldn't be in public documentation.
52-56: Appropriate use of test-utils feature gate.Good practice gating test utilities behind a feature flag to keep them out of production builds while making them available for downstream testing.
87-87: The review comment's premise is incorrect: EntryPayload constructors have not been removed.The initial script output definitively shows that
EntryPayload::command(),EntryPayload::noop(), andEntryPayload::config()constructors remain active and widely used throughout the codebase. The search found 100+ usages across production code (d-engine-core, d-engine-server) and tests, with no compilation failures. These constructors are tested in d-engine-proto/src/exts/common_ext_test.rs and used in active code paths like d-engine-core/src/raft.rs and d-engine-core/src/raft_role/leader_state.rs.No verification or fixes are necessary—no constructors were removed.
d-engine-core/src/raft.rs (1)
74-114: Test infrastructure properly gated.Good use of
cfg(any(test, feature = "test-utils"))for test-only listeners and methods.Also applies to: 336-370
d-engine-client/src/mock_rpc.rs (1)
16-47: Mock service design looks good for testing.Public fields on
MockRpcServiceare appropriate for test infrastructure, allowing flexible configuration. The builder pattern withwith_metadata_responseprovides a clean API.d-engine-core/src/test_utils/mock/mock_rpc_service.rs (1)
127-305: All simulation helpers are public - verify necessity.Unlike the client version where most
simulate_*methods arepub(crate), all helpers here arepub. Ensure this broader visibility is intentional for the test-utils module.
… engine ## 🎯 Overview v0.2.0 is a major release that transforms d-engine from a Raft implementation into a **production-ready distributed coordination engine** with workspace structure, developer-friendly APIs, and comprehensive features. --- ## 🚀 Key Features ### 🏗️ **Workspace Structure (#167)** - Modular design: `d-engine-core`, `d-engine-server`, `d-engine-client`, `d-engine-proto` - Clean separation of concerns for library users - Profile-optimized build configuration ### ⏱️ **TTL & Lease Support (#172)** - Crash-safe TTL with absolute expiration timestamps - WAL format change: relative `ttl_secs` → absolute `expire_at_secs` - Piggyback cleanup mechanism (minimal overhead) - **Breaking Change**: See MIGRATION_GUIDE.md for WAL upgrade ### 👁️ **Watch API (#174, #196)** - Lock-free event notification with crossbeam-channel - gRPC streaming support for real-time key monitoring - <10ns apply-path overhead, <100μs end-to-end latency - Service discovery examples included ### 🚀 **EmbeddedEngine API (#182)** - Zero-config single-node quick start - `wait_leader()` and `leader_notifier()` for event-driven apps - Dynamic cluster expansion (1→3 nodes without downtime) - In-process `LocalKvClient` with explicit consistency levels ### 🎯 **Single-Node Support (#179)** - Configuration-based single-node detection - Automatic election/replication optimization - Production-ready for low-traffic scenarios ### 📖 **Read Consistency Policies (#142)** - LinearizableRead (strong consistency) - LeaseRead (optimized with leader lease) - EventualConsistency (fast local reads) ### 🔧 **Go Client Support (#170, #219)** - Pre-generated Go protobuf code (zero-config) - Comprehensive error handling guide - Service discovery pattern examples --- ## 🐛 Critical Fixes - **#212**: Fix learner promotion stuck (voter count + role transition) - **#218**: Fix leader next_index initialization for new learners - **#222**: Return NOT_LEADER with leader metadata for client redirection - **#209**: Fix node restart wait_ready() timeout (leader notification race) - **#211**: Remove Arc::get_mut anti-pattern from lease injection - **#145**: Fix undefined behavior in mmap zero-copy path - **#197**: Fix integration test failures (Arc ownership, timing issues) --- ## ⚡ Performance Improvements - **#138**: Long-lived peer tasks for append entries (100K+ throughput target) - **#140**: Optimize proto bytes fields (use `bytes::Bytes`) - **#141**: Optimize RocksDB write path for lower latency - **#143**: Refactor gRPC compression configuration - **#194**: Skip protobuf decoding when no active watchers - **#208**: Eliminate redundant async calls in leader write hot path (+2-3% throughput) - **#223**: Optimize check_learner_progress() lock contention ### Benchmark Results - **LeaseRead**: 99,418 ops/s (+7.8% vs v0.1.4) - **EventualConsistency**: 126,095 ops/s (+9.2% vs v0.1.4) - **Linearizable p99**: 23.01ms (-8% vs v0.1.4) --- ## 🔧 Refactoring & Architecture - **#210**: Simplify watch architecture (tokio::broadcast, 90% code sharing) - **#217**: Refactor Node::run() with strategy pattern - **#209**: Consolidate Raft unit tests (27 tests migrated from server to core) - **#201**: Separate static membership from dynamic leader state - **#223**: Extract 5 helper methods in learner promotion (SRP + 11 unit tests) --- ## 📚 Documentation - Restructure quick-start docs (embedded + standalone examples) - Add integration-modes.md and use-cases.md - Comprehensive error handling guide - Service discovery pattern documentation - Delete 911 lines of internal architecture docs (20/80 principle) --- ## 🧪 Testing - **430** core tests + **305** server tests + **292** integration tests passing - Fix flaky tests and timing issues - Add 14+ new unit tests across components - Optimize test suite with nextest --- ##⚠️ Breaking Changes 1. **WAL Format**: Relative `ttl_secs` → absolute `expire_at_secs` (requires data migration) 2. **Config**: `raft.watch.enabled` removed (Watch always available) 3. **API**: `StateMachine::start()` changed to async 4. **NodeStatus**: Refactored enum (PROMOTABLE/READ_ONLY/ACTIVE) See **MIGRATION_GUIDE.md** for detailed upgrade instructions. --- ## 📦 Dependency Updates - tokio-stream: Fix net feature requirement - astral-tokio-tar: Replace tokio-tar (CVE-2025-62518 fix) - simd-adler32: 0.3.7 → 0.3.8 - rustls-pemfile: unmaintained warning suppressed (tonic 0.12 dep) --- ## 🎯 Closes #43, #45, #59, #66, #70, #71, #79, #89, #90, #101, #102, #106, #107, #109, #110, #119, #120, #121, #122, #123, #125, #133, #135, #138, #139, #140, #141, #142, #143, #145, #146, #147, #148, #150, #151, #152, #153, #154, #155, #156, #157, #158, #159, #161, #164, #167, #170, #172, #174, #175, #176, #178, #185, #186, #187, #192, #193, #194, #195, #196, #197, #200, #201, #203, #204, #205, #208, #209, #210, #211, #212, #213, #217, #218, #219, #222, #223, #224 --- **Files Changed:** 100+ files, ~5,000 insertions, ~1,500 deletions
1 participant
Type
Description
Workspace structure refactor
Related Issues
Checklist
Summary by CodeRabbit
New Features
Improvements
Documentation