Conversation
…y log entry. feat #66: Refactor the Errors to distinguish between protocol logic errors and system-level errors. * feat #43: When a Raft leader is elected, it should first send an empty log entry. 1. implement verify_leadership_in_new_term 2. refactor verify_leadership_in_new_term and enforce_quorum_consensus * feat #66: Refactor the Errors to distinguish between protocol logic errors and system-level errors.
…r-friendly StorageEngine and StateMachine * feat #59: Refactor state_machine_commit_listener into a separate function to improve visibility across all newly spawned threads. * feat #79: Add snapshot feature * fix #90: Refactor: Decouple Client Command Protocol from Raft Internal Log Payload Type * feat #89: Add auto-discovery support for new learner nodes * feat #101: Revised learner join process with promotion/fail semantics * feat #45: Implemented first/last index for term to resolve replication conflicts * fix #106: Retry leadership noop confirmation until timeout * feat #102: Automatic node removal * feat #109: Enable RPC connection cache * fix #107: 1. Add back rpc connection params 2. Change parking lot sync lock to Tokio async lock in RaftMembership 3. optimize MembershipGuard for performance. Remove read locker. * feat #110: 1. add new events SnapshotCreated and LogPurgeCompleted 2. decouple Leader snapshot create to several events 3. refactor raft log index generation from pre_allocate_raft_logs_next_index to pre_allocate_id_range 4. optimize LocalLogBatch structure - based on performance analysis * feat #119: Make the StorageEngine trait more developer-friendly * feat #120: Make the StateMachine trait more developer-friendly * fix #121: Election: higher term vote request should be updated immediately and reset voted_for * feat #122: Refactor compress and decompress snapshot logic from StateMachine to StateMachineHandler * fix #123: Replace global tracing log with trace_test crate * feat #125: Make rocksdb adaptor as feature. Move sled implementation as example
…tart when path is already locked by another node
* refs #138: optimize to use long lived peer tasks for append entries request only * refs #138: try to fix race issue - Multiple threads can pass the initial check and all create new appenders * feat #138: 1. introduce crossbeam 2. optimize buffered_raft_log by creating a flush worker pool with configurable number of workers to handle persistence operations * feat #138: optimize flush_workers as configure * feat #138: optimize channel_capacity into configure * feat #139: work on a reliable monitoring metric for resource utilization * feat #140: Optimize proto bytes fields to use bytes::Bytes * feat #140: switch Into<Bytes> to AsRef<[u8]> * fix #141: Optimize RocksDB write path and Raft log loop for lower latency * feat 142: Add Read Consistency Policy Support and Lease-Based Read Optimization * feat #142: update v0.1.4 bench report * feat #142: add client doc guide * feat #143: Refactor gRPC compression configuration for Raft transport (performance optimization) - step1: disable server side client rpc compress response * feat #143: Refactor gRPC compression configuration for Raft transport (performance optimization) * feat #143: add performance diagram * feat #138: ready to publish v0.1.4 * feat #138: add ut * fix #146: buffered_raft_log.rs Code Reviews * fix #146: update bench report * fix #145: Bug: Undefined Behavior from Incorrect Lifetime Conversion in Mmap Zero-Copy Path * fix #148: fix rocksdb storage engine flush bug * fix #147: fix file state machine bug
- Add check_cluster_is_ready() after node restart in failover_test - Root cause: client connected before node fully initialized - Connection refused error due to missing readiness check - Update overview.md Quick Start examples - Add EmbeddedEngine::with_rocksdb() as recommended approach - Fix FileStateMachine::new() async call (add .await) - Clarify API usage: EmbeddedEngine (simple) vs NodeBuilder (flexible)
Problem: Single node cannot exit with Ctrl+C during startup when peers are unavailable. Process hangs with "Connection refused" errors during cluster ready check and connection warmup phases. Solution: 1. Add shutdown_signal (watch::Receiver) to Node struct 2. Use tokio::select! in run() to monitor shutdown during: - check_cluster_is_ready() - pre_warm_connections() 3. Implement Drop for GrpcTransport to abort peer appender tasks 4. Add abort_all_tasks() method for immediate task termination Impact: - Node responds to SIGINT immediately during startup - No connection retry spam after Ctrl+C - Clean shutdown in both single-node and cluster modes
Problem: 1. lib.rs contained broken .html links (e.g., raft-role.html) - Actual paths use underscores and index.html (raft_role/index.html) 2. All workspace crates generated docs, including internal ones - d-engine-core and d-engine-proto are implementation details 3. External references in quick-start-5min.md broke in generated docs 4. Missing error-handling.md caused broken references Solution: 1. Fixed all .html links in lib.rs to match rustdoc output format - Changed hyphenated names to underscore_names/index.html 2. Updated Makefile docs target to exclude internal crates - Added --exclude d-engine-core --exclude d-engine-proto - publish = false ensures they won't be released to crates.io 3. Removed external product-design references - Replaced with inline content in quick-start-5min.md 4. Created error-handling.md with error categories and retry strategies - Updated go-client.md to reference new doc instead of GitHub links Files Changed: - d-engine-docs/src/lib.rs: Fixed 12+ .html link paths - d-engine-docs/src/docs/client_guide/error-handling.md: New file - d-engine-docs/src/docs/client_guide/mod.rs: Added error_handling module - d-engine-docs/src/docs/client_guide/go-client.md: Updated error reference - d-engine-docs/src/docs/client_guide/service-discovery-pattern.md: Fixed scale-to-cluster link - d-engine-docs/src/docs/server_guide/watch-feature.md: Removed broken config/performance links - d-engine-docs/src/docs/quick-start-5min.md: Removed external reference - Makefile: Updated docs target to exclude internal crates Impact: - All documentation links now work correctly - Cleaner workspace docs (only public crates: client, server, docs) - Better user experience with complete error handling guide - No broken external dependencies in generated documentation
## Feature Simplification - Remove `full` feature (confusing, no real use case) - Add `default = ["server", "rocksdb"]` for zero-config start - Move `KvClient` trait export to `client` feature (from `full`) ## Documentation Enhancements - README: Add "When to Use" section explaining 3 integration modes - Embedded Mode (Rust): Zero-overhead, single binary - Standalone Mode (Go/Python/Java): Language-agnostic gRPC - Custom Storage: Advanced use cases (Sled, memory-only) - Add use-cases.md: Detailed scenarios for control plane, DNS, orchestration - Include objective performance claims (51% vs etcd, with disclaimers) ## Developer Experience - Simplify dependency declaration: `d-engine = "0.2"` (no features needed) - Examples updated to use default feature (7 examples simplified) - Add ❌ "Don't use" guidance to prevent common mistakes ## Testing Infrastructure - Fix `make test-examples`: Now properly verifies all examples compile - Add `test-examples` to `make test-all` (pre-release checklist)
Changes: - Remove commented legacy package config from workspace Cargo.toml - Update README benchmark reference to v0.2.0 (dengine_comparison_v0.2.0.png) - Upgrade rustfmt edition from 2021 to 2024 - Clean up outdated v0.2.0 benchmark reports (report_20251205/09/10.md) - Add final v0.2.0 benchmark report and comparison chart - Sync version references across all example Cargo.toml files
- Fix typo: rename backgroup → background in snapshot transfer - Remove obsolete storage_test.rs - Update client error handling and KV operations - Fix network module exports - Update candidate state and state machine handler - Fix single-node cluster examples documentation - Update benchmark workflow configuration
Cache cluster metadata in LeaderState to eliminate 3 async calls per write.
Performance: +2-3% throughput, -13-21% p99 latency vs v0.2.0
Changes:
- Add ClusterMetadata { is_single_node, total_voters } cache
- Initialize on leader election, update on membership changes
- Pass to replication handler to avoid repeated queries
- Fix 6 test failures due to missing metadata initialization
…ale cache Bug: update_cluster_metadata() only updated total_voters, missing is_single_node. Caused test_embedded_node_rejoin to hang intermittently. Fix: - Update BOTH is_single_node and total_voters in update_cluster_metadata() - Call update_cluster_metadata() after learner promotion - Add unit test to catch this bug
Race condition: gRPC servers start async, election RPCs timeout before ready. Solution: Increase test election timeout 100ms→2000ms, retries 3→5. Only affects tests/common/mod.rs test configuration.
…tations - Add Project Status section to README (pre-1.0 + compatibility promise) - Simplify MIGRATION_GUIDE (remove NodeBuilder API, keep WAL format only) - Streamline CHANGELOG v0.2.0 highlights (code examples + concise features) - Update benchmark reports with actual test data (Dec 13, 2025) - Restructure performance report (TL;DR first, detailed results collapsed) Focus: Lower cognitive load for new users while maintaining transparency
…ments This PR consolidates architecture review improvements and critical bug fixes for v0.2.0 release. ## 🎯 Critical Fixes - **#212**: Fix learner promotion stuck (voter count + role transition bugs) - **#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) ## ✨ Feature Additions - **#213**: Implement READ_ONLY Learner nodes for permanent analytics - **#219**: Pre-generate Go protobuf code for zero-config experience ## 🔧 Refactoring & Performance - **#223**: Optimize check_learner_progress() lock contention - Extract 5 helper methods (SRP), add 11 unit tests - **#211**: Remove Arc::get_mut anti-pattern from lease injection - **#210**: Simplify watch architecture (tokio::broadcast, 90% code sharing) - **#217**: Refactor Node::run() with strategy pattern - **#209**: Consolidate Raft unit tests (migrate 27 tests from server to core) ## 📚 Documentation - Restructure quick-start docs (embedded + standalone examples) - Add integration-modes.md and use-cases.md - Delete 911 lines of internal architecture docs (20/80 principle) - Fix Go client example with pre-generated protobufs ## 🧪 Testing & Validation - 14+ new unit tests across components - Fix flaky tests and timing issues (#204, #209) - Optimize test suite with nextest - All 430 core + 305 server + 292 integration tests passing ## 📊 Performance - Benchmarks stable (±3% variance) - CI optimized: removed benchmark compile check (saves 2-3 min/run) --- **Closes:** #209, #210, #211, #212, #213, #217, #218, #219, #222, #223 **Migration Notes:** - Config: `raft.watch.enabled` removed (breaking) - API: `StateMachine::start()` changed to async (breaking) - NodeStatus enum refactored: PROMOTABLE/READ_ONLY/ACTIVE (non-breaking) **Files Changed:** 100+ files, ~5000 insertions, ~1500 deletions
…ion for v0.2.0 ## Overview Prepare d-engine v0.2.0 for multi-crate publishing with comprehensive documentation, improved developer experience, and production-ready examples. ## Key Changes ### 📚 Documentation Restructuring (#225) **Adopted OpenRaft documentation pattern:** - Moved docs from standalone crate to `d-engine/src/docs/` (preserves git history) - Unified documentation structure with role-based guides - All crates now have clear positioning and usage guidelines **Multi-crate README strategy:** - Added READMEs for all workspace crates with consistent structure - Clear guidance on when to use each crate vs. main `d-engine` package - Architecture diagrams showing crate relationships - Verified all crates ready for publishing (`cargo publish --dry-run`) **Link fixes after refactoring:** - Updated 14 files with cross-crate doc links to docs.rs format - Fixed intra-doc links in `d-engine/src/docs/mod.rs` - Added tested Go/Python code generation examples in `d-engine-proto` ### 🔧 Developer Experience Improvements (#226) **Reduced startup noise:** - Eliminated misleading connection errors during cluster startup - Downgraded expected connection failures from `error!` to `debug!` level - Added startup banner with config path and node prefixes `[Node1]` `[Node2]` `[Node3]` - Improved log readability with emoji indicators **Examples cleanup:** - Removed duplicate `rocksdb-cluster` example (superseded by `three-nodes-cluster`) - Fixed `quick-start-embedded` release mode config error (added `CONFIG_PATH`) - Updated all examples to match current API (`EmbeddedEngine`, `StandaloneServer`) - Clarified service-discovery examples focus on **Watch API** demonstration ### 🐛 Bug Fixes **Compilation fixes:** - Fixed `Result` type conflicts in `quick-start-embedded` (use `std::result::Result`) - Updated API imports in `sled-cluster` after module reorganization - Fixed clippy `uninlined_format_args` warning in `three-nodes-cluster` **Updated references:** - All `rocksdb-cluster` references point to `three-nodes-cluster` - Updated `include_str!` paths in `d-engine-core` after docs relocation ## Testing - ✅ All examples compile and pass clippy checks - ✅ All crates ready for publishing (dry-run verified) - ✅ Proto generation commands tested (Go/Python/Java) - ✅ No performance regressions (±5% variance within normal range) ## Files Changed - **8** crate lib.rs/README.md files - **6** example README files - **3** example source files (compilation fixes) - **5** network/logging files (startup experience) - **1** docs reorganization (mod.rs/overview.md) --- **Ready for review and merge to `develop`** 🚀
- Fix protoc working directory in d-engine-proto README (should be in d-engine-proto, not proto/ subdirectory) - Update Rust version requirement from 1.70+ to 1.85+ to match edition 2024 - Fix inconsistent imports in sled-cluster test (use d_engine_proto::client consistently across codebase)
## Problem **Critical bug**: `put()` → `get_linearizable()` returned `None` immediately after write - **Root cause**: Leader commits log but doesn't wait for state machine to apply - **Impact**: Violates linearizability guarantee - reads don't reflect committed writes - **Reproducibility**: Only on first startup (no existing data) due to timing race ## Solution Following industry best practices (inspired by OpenRaft), implemented **wait-for-apply mechanism**: 1. **Watch channel for apply notifications** - `tokio::sync::watch` notifies when `last_applied` advances - `ensure_state_machine_upto_commit_index()` waits until target index applied 2. **Configurable timeout** - `state_machine_sync_timeout_ms` (default: 10ms for local SSD) - Tests use 100ms for CI environment reliability 3. **Enhanced error diagnostics** - Timeout errors now include `current_applied` index - Helps diagnose: stuck state machine, slow apply, or no progress ## Test Coverage - **Integration tests**: Multi-node linearizable read consistency (Test 5b) - **No-sleep tests**: `test_put_get_no_sleep()` validates immediate reads - **Concurrent test fixes**: Isolated mock addresses, tempfile for test data - **Examples updated**: Use `get_linearizable()` in quick-start/service-discovery ## Performance Impact | Read Type | Latency Change | Notes | |------------------|----------------|--------------------------------| | Linearizable | +0.8% | ✅ No regression | | LeaseRead | +9.5% | ✅ Unexpected improvement | | Eventual | -6.1% |⚠️ Will optimize in v0.3.0 | | Write | -3.0% | ✅ Acceptable for correctness | **Trade-off**: -3~6% performance for **critical data consistency fix**. Linearizable read (primary use case) unaffected. ## Documentation - README example simplified (30→13 lines) with links to detailed guides - Added references to `examples/quick-start-embedded`, `service-discovery-embedded`, `quick-start-standalone` ## Breaking Changes None - API unchanged, behavior now matches documented guarantee.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces state-machine synchronization for linearizable reads: a configurable timeout in Raft read consistency, an async wait_applied API with watch-based notifications in the state machine handler, conversion of leader-side ensure_state_machine_upto_commit_index to async, and updates to tests, examples, and README to use linearizable reads. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Leader
participant SMH as StateMachineHandler
participant SM as StateMachine
Client->>Leader: get_linearizable(key)
activate Leader
rect rgb(235,245,255)
Note over Leader,SMH: Leader ensures state machine reached commit index
Leader->>SMH: wait_applied(commit_index, timeout)
activate SMH
alt already applied
SMH-->>Leader: Ok
else behind
SMH->>SM: subscribe/watch applied index
SM-->>SMH: send applied index updates
SMH-->>Leader: Ok when target reached
end
deactivate SMH
end
Leader->>SM: read(key)
SM-->>Leader: value
Leader-->>Client: value
deactivate Leader
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #228 where get_linearizable() incorrectly returned None immediately after a write operation. The fix implements a wait mechanism that ensures linearizable reads always reflect all committed writes, per Raft protocol guarantees.
Key Changes:
- Added
wait_applied()method to the state machine handler to block until entries are applied - Implemented watch-based notification using tokio's watch channel for efficient waiting
- Made
ensure_state_machine_upto_commit_index()async to properly wait for state machine synchronization - Added configurable timeout (
state_machine_sync_timeout_ms, default 10ms) for state machine sync operations
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| d-engine-core/src/state_machine_handler/mod.rs | Added wait_applied() trait method for blocking until target index is applied |
| d-engine-core/src/state_machine_handler/default_state_machine_handler.rs | Implemented watch-based wait mechanism with timeout and notifications |
| d-engine-core/src/raft_role/leader_state.rs | Made ensure_state_machine_upto_commit_index() async and added wait call |
| d-engine-core/src/config/raft.rs | Added state_machine_sync_timeout_ms configuration option with 10ms default |
| config/base/raft.toml | Added configuration for state machine sync timeout |
| d-engine-server/tests/embedded/local_kv_client_integration_test.rs | Added comprehensive single-node linearizable read tests |
| d-engine-server/tests/cluster_start_stop/cluster_integration_test.rs | Added multi-node linearizable read consistency test |
| d-engine-server/tests/components/raft_role/leader_state_test.rs | Updated unit tests to mock wait_applied() method |
| d-engine-server/tests/common/mod.rs | Added test_put_get_no_sleep() helper for linearizable read testing |
| d-engine-server/tests/election/election_case1.rs | Refactored to use temporary directory for test isolation |
| d-engine-server/src/network/connection_cache_test.rs | Fixed test to use independent mock addresses |
| examples/service-discovery-embedded/server.rs | Added verification of linearizable reads after writes |
| examples/quick-start-embedded/src/main.rs | Changed from get_eventual() to get_linearizable() and added failure detection |
| README.md | Updated documentation to simplify Quick Start section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d-engine-server/tests/embedded/local_kv_client_integration_test.rs
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
d-engine-server/tests/embedded/local_kv_client_integration_test.rs (1)
13-18: Fix mismatch between timeout value and comment
state_machine_sync_timeout_msis set to1000, but the comment says// 100ms for tests:let mut raft_config = RaftConfig::default(); raft_config.read_consistency.state_machine_sync_timeout_ms = 1000; // 100ms for testsEither drop/adjust the comment or change the value to
100so the code and documentation stay in sync.Also applies to: 62-70
🧹 Nitpick comments (7)
d-engine-server/tests/election/election_case1.rs (1)
47-50: Tempdir-based storage/log paths look correct; consider simplifying reset usageUsing
tempfile::tempdir()and derivingdb_root_dir/log_dirkeeps this test self-contained and avoids persistent artifacts; theprepare_storage_engineandcreate_node_configpaths are wired consistently.You might eventually drop the
reset(ELECTION_CASE1_DIR)/ELECTION_CASE1_DIRconstant for this test (since all on-disk state now lives under the tempdir), but it's harmless as-is.Also applies to: 56-64, 72-79
d-engine-server/tests/common/mod.rs (1)
437-452: No-sleep helper matches intent; consider propagatingverify_readerrorsThe helper correctly exercises the "PUT then immediate linearizable read" path and reuses existing
ClientManagerAPIs.If
verify_readreturns aResult, you may want to apply?there as well to surface read failures through the test helper, rather than relying solely on internal assertions.d-engine-server/tests/cluster_start_stop/cluster_integration_test.rs (1)
19-20: Multi-node linearizable read test is well-structured and aligned with the bugThe new
test_multi_node_linearizable_read_consistencycorrectly:
- Reuses the common start-node/ready/ClientManager machinery.
- Uses its own case name/DB/LOG roots to avoid interfering with existing tests.
- Validates both single PUT→immediate read and sequential overwrites via
test_put_get_no_sleep.I don’t see correctness issues; this should give good coverage for the regression described in #228.
Also applies to: 261-341
d-engine-core/src/config/raft.rs (1)
953-985: Newstate_machine_sync_timeout_msfield is wired correctlyThe new
state_machine_sync_timeout_ms:
- Is annotated with
#[serde(default = "default_state_machine_sync_timeout_ms")]for backward compatibility.- Is initialized in
Defaultand has a sensible 10ms default.If you intend to forbid
0as a timeout, you could extendReadConsistencyConfig::validate()to enforcestate_machine_sync_timeout_ms > 0, but as-is this is structurally sound.d-engine-server/tests/components/raft_role/leader_state_test.rs (2)
894-909: Linearizable read path now properly exercises state-machine synchronizationIn
test_handle_raft_event_case6_2, theMockStateMachineHandlerexpectations:
update_pending()once,wait_applied()once,read_from_state_machine()returningSome(vec![]),combined with wiring via
.with_state_machine_handler, ensure the leader’s ClientReadRequest path for linearizable reads waits for the state machine before serving.If
read_from_state_machinein production expects one value per key, you might want the mock to return a vector with matching length (e.g.,Some(vec![Bytes::new()])) to mirror real behavior more closely.
4926-4940: Unspecified-policy client read test now exercises linearizable path end-to-endThe updated
test_handle_client_read_unspecified_policy_leader:
- Injects a
MockStateMachineHandlerwithupdate_pending,wait_applied, andread_from_state_machine.- Ensures that the default (LinearizableRead) policy drives the same wait-and-read behavior as explicit linearizable reads.
Same note as above: if the real implementation requires value-count matching keys, consider returning one value per key in the mock.
d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)
246-247: Consider adding a comment explaining why send result is ignored.The notification is correctly placed after the successful apply. The
let _ =pattern at line 247 is appropriate sincewatch::Sender::send()returns the previous value (which we don't need), and send errors are not possible with watch channels.However, for future maintainability, consider adding a brief comment:
🔎 Optional clarification comment
// Notify waiters that last_applied has advanced + // Note: send() returns the previous value; we ignore it as watch channels cannot fail let _ = self.applied_notify_tx.send(idx);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.mdconfig/base/raft.tomld-engine-core/src/config/raft.rsd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/state_machine_handler/default_state_machine_handler.rsd-engine-core/src/state_machine_handler/mod.rsd-engine-server/src/network/connection_cache_test.rsd-engine-server/tests/cluster_start_stop/cluster_integration_test.rsd-engine-server/tests/common/mod.rsd-engine-server/tests/components/raft_role/leader_state_test.rsd-engine-server/tests/election/election_case1.rsd-engine-server/tests/embedded/local_kv_client_integration_test.rsexamples/quick-start-embedded/src/main.rsexamples/service-discovery-embedded/server.rs
🧰 Additional context used
🧬 Code graph analysis (6)
d-engine-core/src/state_machine_handler/mod.rs (2)
d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)
wait_applied(163-189)d-engine-server/src/node/client/local_kv.rs (1)
timeout(429-431)
d-engine-server/tests/election/election_case1.rs (1)
d-engine-server/tests/common/mod.rs (4)
get_available_ports(604-620)prepare_storage_engine(365-373)manipulate_log(383-401)init_hard_state(403-417)
d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)
d-engine-core/src/state_machine_handler/mod.rs (1)
wait_applied(87-91)
d-engine-core/src/raft_role/leader_state.rs (2)
d-engine-core/src/raft_role/mod.rs (1)
commit_index(326-328)d-engine-core/src/raft_role/role_state.rs (1)
commit_index(242-244)
d-engine-server/tests/cluster_start_stop/cluster_integration_test.rs (3)
d-engine-server/tests/common/mod.rs (6)
test_put_get_no_sleep(441-452)reset(470-490)get_available_ports(604-620)start_node(267-287)node_config(170-244)create_bootstrap_urls(455-457)d-engine-core/src/raft_role/leader_state.rs (2)
new(107-115)new(2123-2165)d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)
new(791-825)
d-engine-server/tests/embedded/local_kv_client_integration_test.rs (2)
d-engine-server/src/node/builder.rs (4)
raft_config(210-216)from_cluster_config(154-161)storage_engine(183-189)state_machine(192-198)d-engine-core/src/config/raft.rs (13)
default(107-125)default(197-204)default(266-273)default(325-333)default(378-384)default(518-540)default(646-650)default(709-714)default(734-739)default(886-894)default(963-970)default(1085-1093)default(1195-1201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Agent
- GitHub Check: audit
- GitHub Check: Lint and Format Check
- GitHub Check: Analyze (rust)
- GitHub Check: Lint and Format Check
- GitHub Check: audit
🔇 Additional comments (16)
d-engine-server/src/network/connection_cache_test.rs (1)
112-120: LGTM! Independent mock addresses improve test isolation.The change from reusing a single address to three independent mock addresses makes the test more robust and realistic. Each connection now has its own endpoint, better simulating production scenarios where different nodes and connection types may use distinct addresses.
README.md (3)
67-101: No changes needed. The benchmark path exists and the 45% performance claim is accurate and well-documented in the comprehensive report atbenches/d-engine-bench/reports/v0.2.0/report_v0.2.0_final.md. The hardware context (M2 Mac single machine vs GCE 3-node etcd setup) is correctly stated in both the README and the report, and the methodology is reproducible.
45-61: Example code and file references are correct.The embedded mode example (lines 45-61) correctly uses the
get_linearizable()API fromLocalKvClient, which returnsResult<Option<Bytes>>. The double.unwrap()pattern—first unwrapping theResultfrom the async call, then unwrapping theOption—is appropriate for a quick-start demo. All referenced example files exist and are consistent with the documented paths.
102-114: The external documentation links and example are valid. The d-engine crate documentation on docs.rs confirms support for implementing custom storage engines and state machines. The referenced example atexamples/sled-clusterexists in the repository.config/base/raft.toml (1)
86-89: LGTM: Well-documented configuration for linearizable read synchronization.The 10ms default timeout is reasonable given the <1ms typical apply latency on local SSD. The comments clearly explain the purpose and usage context.
examples/service-discovery-embedded/server.rs (1)
88-98: Good defensive verification, but consider the user experience.The panic demonstrates that the bug is fixed, which is valuable for testing. However, for a user-facing example, you might consider whether returning a
Resultwith a descriptive error would be more instructive than a panic.That said, if the goal is to assert correctness as part of the demo flow (similar to a self-test), the current approach is acceptable.
d-engine-core/src/state_machine_handler/mod.rs (1)
81-91: LGTM: Clean async API for state machine synchronization.The new
wait_appliedmethod is well-documented and serves a clear purpose for ensuring linearizable reads. The signature is appropriate, and the documentation explains the rationale effectively.examples/quick-start-embedded/src/main.rs (1)
63-94: LGTM: Example updated appropriately for linearizable reads.The change from
get_eventual()toget_linearizable()demonstrates the stronger consistency guarantee, and the error handling witheprintln!is more user-friendly than a panic. The message clearly communicates the expectation.d-engine-core/src/raft_role/leader_state.rs (2)
1848-1872: LGTM: Correct implementation of state machine synchronization for linearizable reads.The async conversion properly waits for the state machine to apply committed entries before serving reads. The logic flow is correct:
- Check if synchronization is needed
- Signal pending work to the state machine handler
- Wait with configurable timeout
- Propagate errors appropriately
The use of the configured timeout value provides flexibility for different deployment environments.
914-926: Good error handling for state machine sync failures.The error from
ensure_state_machine_upto_commit_indexis properly awaited and wrapped in aStatus::failed_preconditionwith context. This provides clear feedback when linearizable reads cannot be satisfied due to state machine lag.d-engine-server/tests/embedded/local_kv_client_integration_test.rs (1)
383-487: New linearizable-read tests cover the critical scenarios wellBoth
test_linearizable_read_after_write_no_sleepandtest_linearizable_read_sees_latest_valueexercise:
- Immediate
get_linearizable()after a successfulput.- Sequential overwrites and ensuring each read sees the latest value.
The assertions use
Bytesconsistently and run against the embedded single-node stack, which is ideal for catching regressions like #228. No issues from a correctness perspective.d-engine-server/tests/components/raft_role/leader_state_test.rs (2)
411-416: Asyncensure_state_machine_upto_commit_indexbehavior is properly validatedThe test now:
- Expects exactly one
update_pendingand onewait_appliedwhenlast_applied < commit_index.- Awaits
ensure_state_machine_upto_commit_index(..).await.This accurately reflects the new async contract between
LeaderStateand the state machine handler.
441-446: Case 2 correctly asserts no state-machine sync when already caught upFor
last_applied >= commit_index, assertingupdate_pending()andwait_applied()are both called zero times, while still awaitingensure_state_machine_upto_commit_index, is the right negative test to guard against unnecessary waits.d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (3)
105-108: LGTM! Watch channel is the right choice for state machine synchronization.The
tokio::sync::watchchannel is well-suited for this use case where multiple waiters need to observe progress oflast_applied. The documentation clearly explains the purpose for linearizable reads.
802-803: LGTM! Initialization correctly sets up the watch channel.The watch channel is properly initialized with
last_applied_index, ensuring consistency with the persisted state. Storing both the sender and receiver allows the struct to notify waiters (viaapplied_notify_tx) and provide cloneable receivers for independent waiters (viaapplied_notify_rx.clone()).Also applies to: 820-821
163-189: The timeout and error handling are appropriate for the use case.The implementation correctly uses
Fatalerror on timeout. The default 10ms timeout is explicitly designed with a 10x safety margin over typical <1ms apply latency on local SSDs, as documented in the config. The timeout is configurable per deployment (tests use 1000ms for slower CI environments), allowing operators to adjust for their specific workload characteristics.Returning
Fatalon timeout is correct because a timeout at the default 10ms indicates a systemic problem—the state machine cannot keep up with commits, which makes it impossible to serve linearizable reads. This is not a transient condition amenable to graceful degradation; it's a consistency violation that should fail loudly. The error message includes diagnostic context (timeout duration, current_applied, target_index) to aid operators in debugging.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
## Performance Optimization - Add fast path check in wait_applied to avoid timeout future overhead - Immediately return if target_index already applied (0-cost early exit) - Expected to reduce linearizable read latency by 20-30% for common case ## Code Quality - Add debug_assert in apply_chunk for channel send failure detection - Add test_simulate_apply() helper method for unit tests ## Test Coverage (P0 + P1 + P2) Unit tests (6 new): - Fast path: already applied scenario - Slow path: wait for notification - Timeout: state machine stuck with diagnostics - Concurrent waiters: multiple readers waiting - Sequential waits: incremental apply progress - Exact match: target equals last_applied Integration test (1 new): - Concurrent write+read: 100 parallel operations validate no stale reads ## Verification - All 6 unit tests pass - All 7 linearizable integration tests pass - Clippy clean
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
d-engine-server/tests/embedded/local_kv_client_integration_test.rs (1)
62-64: Fix the comment to match the actual timeout value.The comment states "100ms for tests" but the actual value is 1000ms (1 second).
Proposed fix
let mut raft_config = RaftConfig::default(); - raft_config.read_consistency.state_machine_sync_timeout_ms = 1000; // 100ms for tests + raft_config.read_consistency.state_machine_sync_timeout_ms = 1000; // 1000ms (1s) for tests
🧹 Nitpick comments (3)
d-engine-core/src/state_machine_handler/wait_applied_test.rs (2)
28-45: Consider relaxing timing assertions for CI environment variability.The fast path tests use strict timing bounds (< 5ms) which could be flaky on resource-constrained CI systems. While the comment acknowledges "CI system scheduling delays", systems under heavy load might exceed 5ms even for a fast-path operation.
Consider increasing the threshold
// Fast path should be very fast (< 5ms allows for CI system scheduling delays) assert!( - elapsed < Duration::from_millis(5), + elapsed < Duration::from_millis(10), "Fast path should return quickly, actual: {elapsed:?}" );Also applies to: 144-161
47-72: Timing assertion may be flaky on slow CI systems.The test expects the operation to complete between 15-50ms, but CI environments can experience significant scheduling delays. The lower bound (15ms) might not be met if the spawned task executes faster than expected, and the upper bound (50ms) might be exceeded on heavily loaded systems.
Consider adjusting timing bounds
// Should take approximately 20ms (waiting for apply) assert!( - elapsed >= Duration::from_millis(15) && elapsed < Duration::from_millis(50), + elapsed >= Duration::from_millis(10) && elapsed < Duration::from_millis(100), "Should wait for notification, actual: {elapsed:?}" );d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)
256-259: Consider relaxing the debug_assert for normal receiver drop scenarios.The
debug_assert!will fire if all receivers are dropped, which could happen in test scenarios or during graceful shutdown. While unlikely in production, a debug assertion failure might be too strict sincesend()failing due to no receivers isn't necessarily a bug.Alternative approach
self.last_applied.store(idx, Ordering::Release); // Notify waiters that last_applied has advanced - if let Err(e) = self.applied_notify_tx.send(idx) { - debug_assert!(false, "apply notify send failed: {e:?}"); - } + // Ignore send errors (no receivers is acceptable) + let _ = self.applied_notify_tx.send(idx);Rationale: If there are no active linearizable readers, the send failing is benign.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
d-engine-core/src/state_machine_handler/default_state_machine_handler.rsd-engine-core/src/state_machine_handler/mod.rsd-engine-core/src/state_machine_handler/wait_applied_test.rsd-engine-server/tests/embedded/local_kv_client_integration_test.rs
🧰 Additional context used
🧬 Code graph analysis (4)
d-engine-server/tests/embedded/local_kv_client_integration_test.rs (2)
d-engine-server/src/node/builder.rs (4)
raft_config(210-216)from_cluster_config(154-161)storage_engine(183-189)state_machine(192-198)d-engine-core/src/config/raft.rs (15)
default(107-125)default(197-204)default(266-273)default(325-333)default(378-384)default(518-540)default(646-650)default(709-714)default(734-739)default(886-894)default(963-970)default(1085-1093)default(1195-1201)from(1000-1010)from(1014-1026)
d-engine-core/src/state_machine_handler/mod.rs (1)
d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)
wait_applied(163-199)
d-engine-core/src/state_machine_handler/wait_applied_test.rs (2)
d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (3)
last_applied(128-130)new(803-837)new_without_watch(843-860)d-engine-core/src/state_machine_handler/mod.rs (1)
last_applied(75-75)
d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (1)
d-engine-core/src/state_machine_handler/mod.rs (1)
wait_applied(89-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: audit
- GitHub Check: Lint and Format Check
- GitHub Check: Analyze (rust)
- GitHub Check: Lint and Format Check
- GitHub Check: audit
🔇 Additional comments (13)
d-engine-core/src/state_machine_handler/mod.rs (2)
48-49: LGTM!The test module addition follows the existing pattern in the file.
83-93: Well-documented async trait method.The
wait_appliedmethod signature is clear, and the documentation effectively explains its purpose for ensuring linearizable reads in Raft. The timeout parameter provides flexibility for different deployment scenarios.d-engine-core/src/state_machine_handler/wait_applied_test.rs (4)
1-26: LGTM!The test helper function is well-structured and provides a clean way to instantiate test handlers with specific applied indices.
74-106: Excellent timeout diagnostics verification.The test correctly verifies that the error message contains useful debugging information (timeout reason, current_applied, target_index). This will help operators diagnose linearizable read issues in production.
108-138: Excellent concurrent waiters test.This test validates that multiple concurrent readers can all wait for the same state machine index and all succeed when it's applied. This is critical for real-world scenarios where many linearizable reads might be queued.
163-189: Good sequential progression test.This test validates that wait_applied works correctly across multiple sequential applies, which mirrors the real pattern of incremental log application.
d-engine-server/tests/embedded/local_kv_client_integration_test.rs (3)
383-433: Excellent test for the core linearizable read guarantee.This test directly validates the P0 bug fix mentioned in PR #228: ensuring
get_linearizable()doesn't return None immediately after a write. The comprehensive documentation explains the Raft linearizability guarantee clearly, making this a valuable test case for future maintenance.
435-487: Thorough sequential write/read test.This test validates that linearizable reads always see the most recent committed value across multiple rapid overwrites, ensuring no stale reads occur. The documentation clearly explains why this matters for correctness.
489-533: Excellent concurrent load test.This test stresses the
wait_appliedimplementation with 100 concurrent write-then-linearizable-read operations, validating both the fast-path optimization and concurrent waiter handling under realistic load.d-engine-core/src/state_machine_handler/default_state_machine_handler.rs (4)
105-108: Well-documented watch channel fields.The comments clearly explain the purpose of the watch channel for linearizable read synchronization.
163-199: Excellent fast-path optimization and comprehensive error diagnostics.The implementation effectively optimizes the common case where the target index is already applied (avoiding timeout future allocation). The timeout error message includes valuable diagnostics (target_index, timeout, current_applied) that will aid in production debugging.
The use of
tokio::sync::watchis appropriate here since multiple readers need to observe state machine progress.
814-815: LGTM!The watch channel initialization correctly starts with the initial
last_applied_index, ensuring that fast-path checks work correctly from the start.Also applies to: 832-833
1250-1258: Useful test helper for simulating state machine progress.This helper cleanly simulates the state machine applying entries, allowing unit tests to trigger notifications without complex setup. The
#[cfg(test)]guard ensures it's only available in test builds.
Type
Related Issues
Checklist
Summary by CodeRabbit
Documentation
New Features
Examples
Tests
✏️ Tip: You can customize this high-level summary in your review settings.