perf #392: ReadActor fast path for Lease/Eventual Read — fix v0.2.4 throughput regression#401
Merged
Conversation
…ReadActorConfig, and full test coverage
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements a ReadActor fast path with atomic ReadLease and monotonic clock; adds StateMachine batch read ( Changesv0.2.5 read fast-path & storage lifecycle
Estimated code review effort Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
d-engine-server/src/storage/lease.rs (1)
1-1: 💤 Low valueStale doc references to "Default lease" after rename.
The module doc (Line 1) and the struct doc (Line 44) still describe this as the "Default lease implementation", but the type is now
TtlLease. Update the wording to avoid confusion for API consumers.📝 Suggested doc wording update
-//! Default lease implementation for d-engine. +//! TTL lease implementation for d-engine.-/// Default lease implementation with single-index lock-free architecture. +/// TTL lease implementation with single-index lock-free architecture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@d-engine-server/src/storage/lease.rs` at line 1, Update the stale documentation wording that refers to "Default lease implementation" to reference the renamed type TtlLease: change the module-level doc (top of the file) and the struct doc for TtlLease (previously describing "Default lease") so they describe this as the TTL-based lease implementation (or "TtlLease") to match the exported type name and avoid confusion for API consumers; search for the text "Default lease" in this file and replace it with wording that mentions TtlLease/TTL-based lease in both the module doc and the doc comment above the TtlLease struct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@d-engine-core/src/config/raft.rs`:
- Around line 229-267: Add runtime validation for the ReadActor knobs by
implementing ReadActorConfig::validate() that rejects channel_capacity == 0
(since tokio mpsc panics on 0) and rejects max_drain == 0 (and optionally
enforce max_drain <= channel_capacity), returning an error or Result to indicate
invalid config; then call ReadActorConfig::validate() from
RaftConfig::validate() so startup fails fast with a clear error message
referencing the offending field(s) (use the struct names channel_capacity and
max_drain in the error text to aid diagnostics).
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 700-701: The code only calls self.shared_state.lease.revoke()
inside the become_follower() transition, leaving a window where ReadActor can
still serve stale LeaseRead; update the higher-term handling branches (the
VoteRequest, AppendEntries, and AppendResult branches where you detect a higher
term) to call self.shared_state.lease.revoke() immediately when leadership is
known lost, before publishing BecomeFollower() and before any early returns, so
the lease is invalidated as soon as a higher-term is observed and ReadActor
cannot serve stale reads.
In `@d-engine-core/src/raft_role/read_lease.rs`:
- Around line 55-64: The pack function currently uses debug_assert! so deadline
overflow is silently truncated in release builds; replace the debug-only check
with a runtime validation: in fn pack(term: u64, deadline_ms: u64) -> u64 check
if deadline_ms <= Self::DEADLINE_MASK and if not panic with a clear message
(e.g. assert!(... , "deadline_ms overflows 48 bits")), ensuring the invariant
established by renew() cannot be violated in release builds; update any tests or
callers expecting previous behavior if necessary.
In `@d-engine-server/src/proto_convert.rs`:
- Around line 214-215: The doc comment above the function in proto_convert.rs is
contradictory: the first sentence says "single-key fast-path read" while the
next says "batch of keys" and the function signature accepts &[bytes::Bytes]
(multiple keys); update the documentation for the function (the comment block
above the function that builds a successful ClientResponse proto/fast-path read)
to consistently describe it as building a fast-path read response for a batch of
keys (or phrase it generically for multiple keys), and remove or replace the
"single-key" wording so the comment matches the &[bytes::Bytes] parameter and
the function's actual behavior.
---
Nitpick comments:
In `@d-engine-server/src/storage/lease.rs`:
- Line 1: Update the stale documentation wording that refers to "Default lease
implementation" to reference the renamed type TtlLease: change the module-level
doc (top of the file) and the struct doc for TtlLease (previously describing
"Default lease") so they describe this as the TTL-based lease implementation (or
"TtlLease") to match the exported type name and avoid confusion for API
consumers; search for the text "Default lease" in this file and replace it with
wording that mentions TtlLease/TTL-based lease in both the module doc and the
doc comment above the TtlLease struct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d5c766c-424a-4c23-a6c0-3f5aa06acb59
📒 Files selected for processing (86)
CHANGELOG.mdMIGRATION_GUIDE.mdbenches/embedded-bench/config/n1.tomlbenches/embedded-bench/config/n2.tomlbenches/embedded-bench/config/n3.tomlbenches/embedded-bench/src/main.rsbenches/reports/v0.2.5/bench_report_v0.2.5.mdconfig/base/raft.tomld-engine-core/src/config/raft.rsd-engine-core/src/raft.rsd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/become_follower_test.rsd-engine-core/src/raft_role/leader_state_test/client_read_test.rsd-engine-core/src/raft_role/leader_state_test/lease_refresh_on_log_flushed_test.rsd-engine-core/src/raft_role/leader_state_test/mod.rsd-engine-core/src/raft_role/leader_state_test/pending_lease_reads_test.rsd-engine-core/src/raft_role/mod.rsd-engine-core/src/raft_role/read_lease.rsd-engine-core/src/raft_role/read_lease_test.rsd-engine-core/src/storage/state_machine.rsd-engine-core/src/storage/state_machine_test.rsd-engine-server/benches/lease_performance.rsd-engine-server/benches/state_machine.rsd-engine-server/benches/ttl.rsd-engine-server/benches/watch_overhead.rsd-engine-server/src/api/embedded.rsd-engine-server/src/api/embedded_client.rsd-engine-server/src/api/embedded_client_fast_path_test.rsd-engine-server/src/api/embedded_client_test.rsd-engine-server/src/api/embedded_test.rsd-engine-server/src/api/mod.rsd-engine-server/src/api/read_handle.rsd-engine-server/src/api/read_handle_test.rsd-engine-server/src/api/standalone.rsd-engine-server/src/lib.rsd-engine-server/src/membership/membership_snapshot.rsd-engine-server/src/network/grpc/grpc_raft_service.rsd-engine-server/src/network/grpc/grpc_raft_service_fast_path_test.rsd-engine-server/src/network/grpc/mod.rsd-engine-server/src/node/builder.rsd-engine-server/src/node/mod.rsd-engine-server/src/proto_convert.rsd-engine-server/src/read_actor.rsd-engine-server/src/read_actor_test.rsd-engine-server/src/storage/adaptors/file/file_state_machine.rsd-engine-server/src/storage/adaptors/file/file_state_machine_test.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine_test.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_unified_engine_test.rsd-engine-server/src/storage/lease.rsd-engine-server/src/storage/lease_integration_test.rsd-engine-server/src/storage/lease_unit_test.rsd-engine-server/src/storage/mod.rsd-engine-server/src/test_utils/mock/mock_node_builder.rsd-engine-server/tests/cas_operations/bank_transfer_invariant_embedded.rsd-engine-server/tests/cas_operations/distributed_lock_embedded.rsd-engine-server/tests/cas_operations/leader_failover_cas_embedded.rsd-engine-server/tests/cas_operations/snapshot_recovery_embedded.rsd-engine-server/tests/cluster_lifecycle/config_loading_embedded.rsd-engine-server/tests/cluster_lifecycle/scale_single_to_three_node_embedded.rsd-engine-server/tests/cluster_lifecycle/single_node_bootstrap_embedded.rsd-engine-server/tests/cluster_state_and_metadata/cluster_state_consensus_embedded.rsd-engine-server/tests/consistent_reads/lease_read_embedded.rsd-engine-server/tests/consistent_reads/linearizable_read_batching_embedded.rsd-engine-server/tests/consistent_reads/linearizable_read_consistency_embedded.rsd-engine-server/tests/drain_batching/select_fairness_embedded.rsd-engine-server/tests/embedded_client/embedded_client_operations.rsd-engine-server/tests/failover_and_recovery/leader_failover_embedded.rsd-engine-server/tests/failover_and_recovery/mod.rsd-engine-server/tests/readonly_and_learner_mode/learner_readonly_sync_embedded.rsd-engine-server/tests/readonly_and_learner_mode/learner_readonly_sync_standalone.rsd-engine-server/tests/snapshot_and_recovery/snapshot_concurrent_replication_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_concurrent_writes_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_follower_generation_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_interrupted_transfer_embedded.rsd-engine-server/tests/snapshot_and_recovery/snapshot_leader_change_during_transfer_embedded.rsd-engine-server/tests/watch_and_subscriptions/watch_events_embedded.rsd-engine-server/tests/watch_and_subscriptions/watch_membership_embedded.rsd-engine-server/tests/watch_and_subscriptions/watch_performance_gate_embedded.rsd-engine/src/lib.rsexamples/quick-start-embedded/src/main.rsexamples/service-discovery-embedded/server.rsexamples/three-nodes-embedded/src/main.rsexamples/three-nodes-standalone/config/n1.tomlexamples/three-nodes-standalone/config/n2.tomlexamples/three-nodes-standalone/config/n3.toml
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…eadActor fast path for Eventual/LeaseRead - read_handle.rs: positional alignment bug — sparse KvEntry from read_from_state_machine was not re-aligned to keys[i]; fix with HashMap to guarantee result[i] ↔ keys[i] regardless of SM response - Delete BatchRead trait (was pub(crate)); replace with StateMachine::get_multi (default sequential impl + RocksDB / FileStateMachine snapshot-isolated overrides) - Move ReadActor to d-engine-server with TypeConfig generic; wire ReadHandle for gRPC and EmbeddedClient multi-key fast path - Fix expect_get → expect_get_multi in 3 test files - Add 5 alignment tests + 11 gRPC fast path tests
…ation - leader_state: revoke lease immediately on higher-term detection (VoteRequest / AppendEntries / AppendResult) — eliminates the window between send_become_follower_event() and become_follower() where ReadActor could still serve stale LeaseRead - read_lease: promote debug_assert! to assert! so overflow guard fires in release builds - ReadActorConfig: add validate() rejecting channel_capacity=0 and max_drain=0; call from RaftConfig::validate() - ReadConsistencyConfig: move lease_duration_ms < election_timeout_min constraint into ReadConsistencyConfig::validate(election_timeout_min), making the safety invariant self-contained at the right layer - proto_convert: remove contradictory "single-key" doc comment - Tests: 3 window-period tests for become_follower, 4 ReadActorConfig validation tests, 1 ReadConsistencyConfig lease safety unit test
… path - Replace EmbeddedEngine/EmbeddedClient with DefaultEmbeddedEngine/ DefaultEmbeddedClient in all code examples (README, quick-start, integration-modes, watch-feature, ha-deployment, d-engine-server README, examples/quick-start-embedded) - read-consistency: document ReadActor fast path, fix EventualConsistency "Any node" claim, add 0 RTT explanation - consistency-tuning: fix lease_duration_ms default (500→250), fix all scenario configs that would fail validate(), add both formula bounds, remove 6 non-existent raft.xxx metrics, drop fictional Monitoring section - throughput-optimization-guide: replace architectural prose with actionable channel_capacity/max_drain tuning table (correct defaults: capacity=512, max_drain=100) - raft.rs lease_duration_ms: add full safety constraint doc including theoretical bound and clock drift guidance; fix wrong default comment
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@d-engine-server/src/api/read_handle.rs`:
- Around line 181-201: The fast-path read ignores the caller's timeout because
reply_rx.await is unbounded; wrap the oneshot receive in
tokio::time::timeout(timeout, reply_rx).await and treat a timeout as a
fallthrough to the cmd_tx path. Concretely, replace reply_rx.await match with a
timeout call and then match the timeout result: on Ok(Ok(Ok(values))) return
Ok(values); on Ok(Ok(Err(ReadActorError::SmError(e)))) return
Err(server_error(e)); on Ok(Ok(Err(ReadActorError::LeaseInvalid |
ReadActorError::SmStopped))) fall through; on Ok(Err(_)) (oneshot dropped) fall
through; on Err(_) (timeout elapsed) fall through to cmd_tx. Keep existing
send/read_tx and ReadCmd logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d09c68d-bafe-4b14-a3e5-63c068a555e0
📒 Files selected for processing (41)
README.mdd-engine-core/src/config/raft.rsd-engine-core/src/config/raft_test.rsd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/become_follower_test.rsd-engine-core/src/raft_role/read_lease.rsd-engine-core/src/storage/state_machine.rsd-engine-core/src/storage/state_machine_test.rsd-engine-server/README.mdd-engine-server/src/api/embedded.rsd-engine-server/src/api/embedded_client.rsd-engine-server/src/api/embedded_client_fast_path_test.rsd-engine-server/src/api/embedded_test.rsd-engine-server/src/api/mod.rsd-engine-server/src/api/read_handle.rsd-engine-server/src/api/read_handle_test.rsd-engine-server/src/lib.rsd-engine-server/src/membership/membership_snapshot.rsd-engine-server/src/network/grpc/grpc_raft_service.rsd-engine-server/src/network/grpc/grpc_raft_service_fast_path_test.rsd-engine-server/src/network/grpc/mod.rsd-engine-server/src/node/builder.rsd-engine-server/src/node/mod.rsd-engine-server/src/proto_convert.rsd-engine-server/src/read_actor.rsd-engine-server/src/read_actor_test.rsd-engine-server/src/storage/adaptors/file/file_state_machine.rsd-engine-server/src/storage/adaptors/file/file_state_machine_test.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine_test.rsd-engine-server/src/test_utils/mock/mock_node_builder.rsd-engine/src/docs/client_guide/read-consistency.mdd-engine/src/docs/examples/ha-deployment-load-balancing.mdd-engine/src/docs/integration-modes.mdd-engine/src/docs/overview.mdd-engine/src/docs/performance/throughput-optimization-guide.mdd-engine/src/docs/quick-start-5min.mdd-engine/src/docs/quick-start-standalone.mdd-engine/src/docs/server_guide/consistency-tuning.mdd-engine/src/docs/server_guide/watch-feature.mdexamples/quick-start-embedded/README.md
✅ Files skipped from review due to trivial changes (6)
- README.md
- examples/quick-start-embedded/README.md
- d-engine/src/docs/quick-start-standalone.md
- d-engine-server/src/network/grpc/mod.rs
- d-engine/src/docs/server_guide/watch-feature.md
- d-engine-server/src/membership/membership_snapshot.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine_test.rs
- d-engine-server/src/storage/adaptors/file/file_state_machine_test.rs
- d-engine-core/src/storage/state_machine.rs
- d-engine-server/src/proto_convert.rs
- d-engine-server/src/lib.rs
- d-engine-server/src/network/grpc/grpc_raft_service.rs
- d-engine-server/src/test_utils/mock/mock_node_builder.rs
- d-engine-server/src/api/mod.rs
- d-engine-core/src/storage/state_machine_test.rs
- d-engine-server/src/node/mod.rs
- d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs
- d-engine-server/src/node/builder.rs
- d-engine-server/src/storage/adaptors/file/file_state_machine.rs
- d-engine-server/src/api/embedded_test.rs
- d-engine-server/src/read_actor.rs
- d-engine-server/src/api/read_handle_test.rs
- d-engine-server/src/read_actor_test.rs
- d-engine-server/src/api/embedded_client.rs
- d-engine-core/src/raft_role/leader_state.rs
- d-engine-server/src/api/embedded.rs
…for embedded mode Split ReadHandle into StandaloneReadHandle (gRPC/ReadActor path) and EmbeddedReadHandle<T> (direct SM call, zero oneshot/channel overhead). EmbeddedClient now uses EmbeddedReadHandle<T>, recovering v0.2.3 latency. Add close_storage() to StateMachine trait for deterministic LOCK release on stop().
There was a problem hiding this comment.
🧹 Nitpick comments (2)
d-engine-server/src/storage/adaptors/file/file_state_machine_test.rs (1)
89-89: 💤 Low valueRemove redundant
.to_path_buf()calls.Several test setups call
.to_path_buf().to_path_buf()which unnecessarily clones thePathBuftwice. The firstto_path_buf()already returns an ownedPathBuf.♻️ Suggested simplification
For example, line 89:
- let data_dir = temp_dir.path().to_path_buf().to_path_buf(); + let data_dir = temp_dir.path().to_path_buf();Apply the same fix to the other occurrences at lines 165, 219-221, 255, 309, 356-358, 408-410, 447-449, and 538-540.
Also applies to: 165-165, 219-221, 255-255, 309-309, 356-358, 408-410, 447-449, 538-540
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@d-engine-server/src/storage/adaptors/file/file_state_machine_test.rs` at line 89, The test code creates data_dir by calling temp_dir.path().to_path_buf().to_path_buf(), which redundantly clones the PathBuf; change assignments that use temp_dir.path().to_path_buf().to_path_buf() (e.g., the data_dir variable and other occurrences noted) to a single temp_dir.path().to_path_buf() so you only convert the Path to an owned PathBuf once, updating all instances that use the same pattern (search for temp_dir.path().to_path_buf().to_path_buf() and replace with temp_dir.path().to_path_buf()).d-engine-server/src/api/embedded_read_handle.rs (1)
113-122: 💤 Low valueUnreachable comment inside return block.
The comment on line 119
// SM stopped or hard error → fall throughis placed after thereturn Ok(values);statement inside theif letblock, making it unreachable code. The logic is correct, but the comment should be placed after the closing brace of the innerif letor merged with the comment on line 121.This is purely cosmetic and doesn't affect correctness.
♻️ Proposed fix for comment placement
ReadConsistencyPolicy::LeaseRead => { if self.lease.is_valid(now_ms()) && let Ok(values) = self.sm.get_multi(keys) { return Ok(values); - - // SM stopped or hard error → fall through } - // invalid lease → fall through to cmd_tx + // invalid lease or SM stopped/error → fall through to cmd_tx }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@d-engine-server/src/api/embedded_read_handle.rs` around lines 113 - 122, The comment after the return in the ReadConsistencyPolicy::LeaseRead branch is unreachable; move the note about "SM stopped or hard error → fall through" so it sits after the closing brace of the inner if-let (the block that checks self.lease.is_valid(now_ms()) && let Ok(values) = self.sm.get_multi(keys)) or merge it with the adjacent "invalid lease → fall through to cmd_tx" comment, ensuring the comment refers to the outcome of self.sm.get_multi and not placed after the return Ok(values).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@d-engine-server/src/api/embedded_read_handle.rs`:
- Around line 113-122: The comment after the return in the
ReadConsistencyPolicy::LeaseRead branch is unreachable; move the note about "SM
stopped or hard error → fall through" so it sits after the closing brace of the
inner if-let (the block that checks self.lease.is_valid(now_ms()) && let
Ok(values) = self.sm.get_multi(keys)) or merge it with the adjacent "invalid
lease → fall through to cmd_tx" comment, ensuring the comment refers to the
outcome of self.sm.get_multi and not placed after the return Ok(values).
In `@d-engine-server/src/storage/adaptors/file/file_state_machine_test.rs`:
- Line 89: The test code creates data_dir by calling
temp_dir.path().to_path_buf().to_path_buf(), which redundantly clones the
PathBuf; change assignments that use temp_dir.path().to_path_buf().to_path_buf()
(e.g., the data_dir variable and other occurrences noted) to a single
temp_dir.path().to_path_buf() so you only convert the Path to an owned PathBuf
once, updating all instances that use the same pattern (search for
temp_dir.path().to_path_buf().to_path_buf() and replace with
temp_dir.path().to_path_buf()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95f2e5e1-6120-45ed-8235-3a601a794dc6
📒 Files selected for processing (27)
benches/embedded-bench/config/n1.tomlbenches/embedded-bench/config/n2.tomlbenches/embedded-bench/config/n3.tomld-engine-core/src/storage/state_machine.rsd-engine-server/src/api/embedded.rsd-engine-server/src/api/embedded_client.rsd-engine-server/src/api/embedded_client_test/embedded_client_test.rsd-engine-server/src/api/embedded_client_test/mod.rsd-engine-server/src/api/embedded_read_handle.rsd-engine-server/src/api/embedded_read_handle_test.rsd-engine-server/src/api/embedded_test/embedded_env_test.rsd-engine-server/src/api/embedded_test/embedded_test.rsd-engine-server/src/api/embedded_test/mod.rsd-engine-server/src/api/mod.rsd-engine-server/src/api/standalone.rsd-engine-server/src/api/standalone_read_handle.rsd-engine-server/src/api/standalone_read_handle_test.rsd-engine-server/src/network/grpc/grpc_raft_service_fast_path_test.rsd-engine-server/src/node/builder.rsd-engine-server/src/node/mod.rsd-engine-server/src/storage/adaptors/file/file_state_machine.rsd-engine-server/src/storage/adaptors/file/file_state_machine_test.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine_test.rsd-engine-server/src/test_utils/mock/mock_node_builder.rsd-engine/src/docs/server_guide/customize-state-machine.mdexamples/sled-cluster/src/sled_state_machine.rs
💤 Files with no reviewable changes (1)
- d-engine-server/src/api/embedded_test/embedded_test.rs
✅ Files skipped from review due to trivial changes (3)
- d-engine-server/src/api/embedded_test/mod.rs
- benches/embedded-bench/config/n1.toml
- d-engine-server/src/api/embedded_client_test/mod.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- benches/embedded-bench/config/n3.toml
- d-engine-server/src/node/builder.rs
- d-engine-server/src/api/standalone.rs
- d-engine-server/src/test_utils/mock/mock_node_builder.rs
- d-engine-server/src/network/grpc/grpc_raft_service_fast_path_test.rs
- d-engine-server/src/node/mod.rs
- d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine_test.rs
…mbler test - StandaloneReadHandle fast path: wrap reply_rx.await with tokio::time::timeout(timeout, ...) so a stalled ReadActor falls through to cmd_tx instead of blocking the caller indefinitely - Add test_read_actor_stall_respects_timeout to verify the timeout fires within the caller-supplied deadline - snapshot_assembler_test: call flush_to_disk() before drop in first transfer block; tokio::fs::File buffers writes internally and drop does not guarantee OS flush, causing non-deterministic stale file size on CI
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benches/reports/v0.2.5/bench_report_v0.2.5.md`:
- Around line 83-85: Update the summary sentence "v0.2.5 Standalone is stable vs
v0.2.4" to remove the blanket "all write and read scenarios within ±2%" claim
and instead scope it to exclude the Lease Read result (which is +10.8% vs
v0.2.4). Specifically, edit the line containing the phrase "v0.2.5 Standalone is
stable vs v0.2.4" so it either states "all write and read scenarios except Lease
Read are within ±2%" or otherwise calls out Lease Read separately by name,
ensuring the Lease Read +10.8% figure is not contradicted.
- Line 130: The report line incorrectly attributes the +5.7% Linearizable Read
improvement to a “Read-index optimization in ReadActor” despite the PR leaving
Linearizable reads and the Raft loop unchanged; edit the sentence referencing
ReadActor/Linearizable reads to remove causal language and present it as an
observed benchmark delta only (e.g., “Linearizable Read +5.7% vs v0.2.4 —
observed delta; no Linearizable-path code/raft loop changes in this PR”),
ensuring mentions of ReadActor, Linearizable reads, and the Raft loop remain for
context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3427a57-e52f-4c0f-9db9-78b1e7902ba0
⛔ Files ignored due to path filters (3)
benches/reports/v0.2.5/d-engine_comparison_v0.2.5.pngis excluded by!**/*.pngbenches/reports/v0.2.5/d-engine_v0.2.5_vs_v0.2.4_embedded_mode.pngis excluded by!**/*.pngbenches/reports/v0.2.5/d-engine_v0.2.5_vs_v0.2.4_standalone_mode.pngis excluded by!**/*.png
📒 Files selected for processing (2)
README.mdbenches/reports/v0.2.5/bench_report_v0.2.5.md
✅ Files skipped from review due to trivial changes (1)
- README.md
- Standalone notes: scope "%%%%%%%%%%%%" to exclude Lease Read (which shows +10.8%)within - AWS Embedded notes: remove incorrect ReadActor attribution for Linearizable Read; Lin Read path is unchanged by #392, +5.7% is an observed benchmark delta only Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replaces Arc<ArcSwap<Option<Arc<DB>>>> with ArcSwapOption<DB>. The outer Arc was unused — no caller ever cloned the field independently. Call sites become guard.as_deref() and store(None), no behavior change.
…eceive time The safety invariant lease_duration_ms < election_timeout_min was enforced by config validation, but the effective lease duration was silently extended by ~RTT/2 because update_lease_timestamp() used now_ms() at ACK *receive* time. The follower's election timer resets at heartbeat *receive* time (~RTT/2 earlier), so the old deadline = ack_ts + lease_duration_ms could equal or exceed the earliest possible election, violating the invariant in practice (Jepsen set workload confirmed: 19 elements lost under network partition). Fix: record last_heartbeat_send_ts in Phase 0 of execute_and_process_raft_rpc (before AppendEntries goes out). Multi-voter quorum ACK path now computes deadline = send_ts + lease_duration_ms, matching etcd/TiKV practice. Single-voter path unchanged (no peer RTT to account for).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@d-engine-core/src/raft_role/leader_state_test/replication_test.rs`:
- Around line 1401-1407: The fixed 1ms sleep in the test is timing-fragile;
replace it with a bounded wait that polls crate::raft_role::read_lease::now_ms()
until it returns > 0 (with a short overall timeout) before calling process_batch
so the assertion on last_heartbeat_send_ts is deterministic; update the setup in
replication_test.rs (the block that currently calls
crate::raft_role::read_lease::init_clock() and std::thread::sleep) to loop-check
now_ms() > 0 and error/fail the test if the timeout elapses.
In `@d-engine-core/src/raft_role/leader_state.rs`:
- Around line 1899-1908: The lease-renewal uses the shared mutable
last_heartbeat_send_ts (set by execute_and_process_raft_rpc) causing stale ACKs
to renew using a newer global timestamp; instead propagate a per-request send_ts
through the AppendEntries request/response path and use that per-ACK timestamp
when calling update_lease_timestamp. Concretely: when building/sending
AppendEntries capture now_ms() into a local send_ts, attach/pass that send_ts
through the RPC call/response handling (e.g., via the
execute_and_process_raft_rpc callsite and its response callback) and replace
uses of last_heartbeat_send_ts in the ACK/quorum handling with the attached
send_ts so each ACK renews the lease with the timestamp tied to that specific
dispatched AppendEntries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b98e476c-1aac-49ea-8aac-e644bbac018d
⛔ Files ignored due to path filters (3)
benches/reports/v0.2.5/d-engine_comparison_v0.2.5.pngis excluded by!**/*.pngbenches/reports/v0.2.5/d-engine_v0.2.5_vs_v0.2.4_embedded_mode.pngis excluded by!**/*.pngbenches/reports/v0.2.5/d-engine_v0.2.5_vs_v0.2.4_standalone_mode.pngis excluded by!**/*.png
📒 Files selected for processing (8)
.gitignoreREADME.mdbenches/reports/v0.2.5/bench_report_v0.2.5.mdd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/lease_send_ts_test.rsd-engine-core/src/raft_role/leader_state_test/mod.rsd-engine-core/src/raft_role/leader_state_test/replication_test.rsd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- README.md
- benches/reports/v0.2.5/bench_report_v0.2.5.md
🚧 Files skipped from review as they are similar to previous changes (2)
- d-engine-core/src/raft_role/leader_state_test/mod.rs
- d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs
…o lease_duration_ms + rtt_p99/2 < election_timeout_min
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@d-engine-core/src/config/raft.rs`:
- Around line 1080-1088: The docstring for the field/parameter
network_rtt_p99_ms is misleadingly labeled "one-way network RTT" while the code
treats it as round-trip (it is later divided by 2), which can cause operators to
under-estimate margins; update the comment near network_rtt_p99_ms in raft.rs to
state clearly that this value is the p99 round‑trip time (RTT) in milliseconds
and that the safety formula used is lease_duration_ms + (network_rtt_p99_ms / 2)
< election_timeout_min, referencing the symbols network_rtt_p99_ms,
lease_duration_ms and election_timeout_min so readers know the value is halved
when converting to one-way delay.
- Around line 1149-1156: ReadConsistencyConfig::validate() currently does
unchecked u64 addition on lease_duration_ms + rtt_half_ms which can overflow;
change the logic to use checked_add (or equivalent) to detect overflow and
return an Err(Config(ConfigError::Message(...))) when addition would overflow
instead of performing a wrap/unchecked add, then use the checked sum for the
comparison against election_timeout_min (include the same contextual message
mentioning lease_duration_ms and network_rtt_p99_ms/2 and that the sum must be
strictly less than election_timeout_min).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc85752a-0b08-4755-a40a-71a6db15bcf7
📒 Files selected for processing (3)
d-engine-core/src/config/raft.rsd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/replication_test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- d-engine-core/src/raft_role/leader_state_test/replication_test.rs
… network_rtt_p99_ms doc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This PR Do?
Introduces a dedicated
ReadActorthat servesLeaseReadandEventualConsistencyreads directly from the state machine, bypassing the Raft command channel. Also fixes a positional alignment bug inread_handle.rswhere sparseKvEntryresults were not re-aligned to the request key order.Type:
KvEntrymisalignment inread_handle.rsStateMachine::get_multiadded to traitcmd_txcontention for Lease/Eventual readsWhy Is This Needed?
Closes #392.
v0.2.4 routed all read policies through
cmd_tx(the Raft loop), causing channel contention under high concurrency.LeaseReadandEventualConsistencydo not require consensus — they only need a valid lease check or a running state machine. This PR adds a dedicatedReadActorchannel that short-circuits directly to the state machine. The Raft loop is untouched for writes andLinearizableRead.Checklist
Required:
make testpassesIf changing APIs:
Testing
ReadActor(9 tests),ReadHandlealignment (13 tests),ReadLease::revoke/is_valid(10 tests)StateMachine::get_multiRocksDB + FileStateMachine snapshot isolation (8 tests each)become_followerlease revocation — 7 tests including 3 window-period tests that verifylease.revoke()fires at higher-term detection, not only at role transitionReadActorConfig(4 tests),ReadConsistencyConfiglease safety (1 unit test directly onvalidate(election_timeout_min))Benchmark (3-node embedded, same machine):
Architecture
Reviewer Notes
Lease revocation window (safety-critical) —
revoke()is now called at the moment higher-term is detected in VoteRequest / AppendEntries / AppendResult handlers, beforesend_become_follower_event(). The previous impl only calledrevoke()insidebecome_follower(), leaving a window where ReadActor could observe a valid lease between event emission and Raft loop processing. All 3 detection paths have tests pinning this invariant.read_handle.rsalignment fix —read_from_state_machinereturns only existing keys (sparse). Public API contract requiresresult[i] ↔ keys[i]. Fix uses aHashMapat theReadHandleboundary. 5 positional tests cover all cases (first/middle/last missing, all-missing, all-present).StateMachine::get_multi— added to trait with correct-but-sequential default impl. RocksDB andFileStateMachineoverride with snapshot isolation. Zero breaking change for existing SM implementors.BatchReadtrait deleted — waspub(crate), internal only. Replaced byget_multion the public trait.ReadConsistencyConfig::validate— lease safety constraint (lease_duration_ms < election_timeout_min) moved fromRaftConfig::validate()intoReadConsistencyConfig::validate(election_timeout_min), making the invariant self-contained at the right layer.Estimated review complexity:
Summary by CodeRabbit