perf #422: fix WAL fsync serialization regression — concurrent fsync pipeline#424
Merged
Conversation
…pipeline #407 enabled Level 3 durability (per-batch flush_wal(true)), but the IO thread blocked inline on fsync. Under moderate load or high fsync latency, every entry paid the full fsync penalty individually — group commit never formed. Introduce FsyncCoordinator: - fsync dispatched to spawn_blocking; IO thread returns immediately - CAS-gated single-inflight: entries arriving during fsync coalesce into the same physical disk flush (storage-layer group commit, no artificial window) - Generation fence on reset: stale in-flight fsync results are discarded, preventing durable_index resurrection after reset - Poison mechanism: first fsync/persist failure permanently fails the log, all subsequent writes fast-fail with Fatal error - Bounded shutdown: shutdown_timeout_ms (default 5000) prevents close() from hanging on a stuck disk Raft correctness: persist hard_state before acting on term changes. Candidate now calls commit_hard_state() to durably record term increment and self-vote before broadcasting vote requests. When discovering a higher term (via election response or AppendEntries), term update is persisted before stepping down to Follower — a crash between "step down" and "persist" would otherwise resurrect the stale term on restart. Metrics: all d-engine-core metrics renamed with "core." prefix; MetricsConfig removed (always-on, recorder controls emission). New metrics: core.raft.fsync.{duration_ms,batch_entries,inflight,busy_nanos_total}. Docs: new metrics-reference.md; throughput-optimization-guide extended with metrics-based bottleneck diagnosis (USE method).
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis change centralizes Raft hard-state persistence, adds coalesced and reset-safe fsync handling with permanent storage poisoning, standardizes metrics, updates RocksDB durability and recovery behavior, and expands benchmark, Docker, monitoring, configuration, and documentation support. ChangesCore durability and observability
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RaftRole
participant BufferedRaftLog
participant FsyncCoordinator
participant StorageEngine
Client->>RaftRole: submit command
RaftRole->>BufferedRaftLog: persist entries
BufferedRaftLog->>FsyncCoordinator: submit fsync request
FsyncCoordinator->>StorageEngine: coalesced flush
StorageEngine-->>FsyncCoordinator: completion
FsyncCoordinator->>BufferedRaftLog: advance durable index or poison
BufferedRaftLog-->>RaftRole: LogFlushed or FatalError
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
d-engine-core/src/membership.rs (1)
230-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out code.
It is recommended to remove the old commented-out metric line to keep the codebase clean.
♻️ Proposed fix
- // metrics::counter!("core.cluster.unsafe_join_attempts", 1);🤖 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-core/src/membership.rs` at line 230, Remove the commented-out metrics counter line near the membership logic; do not replace it or alter surrounding code.d-engine/src/docs/performance/metrics-reference.md (1)
54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
As suggested by static analysis, providing a language identifier (such as
text) improves rendering and satisfies markdown linters.♻️ Proposed fix
-``` +```text propose_to_commit_ms + commit_to_apply_ms = propose_to_apply_ms</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/src/docs/performance/metrics-reference.mdaround lines 54 - 56,
Update the fenced code block containing the propose-to-apply metrics equation to
specify the text language, preserving the equation content unchanged.</details> <!-- cr-comment:v1:b236f201c7e7b5e6154b21bb --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>benches/embedded-bench/src/main.rs (1)</summary><blockquote> `209-230`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Include `BATCH_BUCKETS` to fully align with the standalone setup.** To ensure the benchmark metrics in embedded mode align perfectly with the standalone example and your Grafana dashboards, consider configuring the `BATCH_BUCKETS` for the `core.raft.fsync.batch_entries` metric here as well. Without this, the metric will fall back to default Prometheus buckets. <details> <summary>♻️ Proposed refactor</summary> ```diff const MS_BUCKETS: &[f64] = &[ 0.1, 0.2, 0.4, 0.8, 1.6, 3.2, 6.4, 12.8, 25.6, 51.2, 102.4, 204.8, 409.6, 819.2, 1638.4, ]; + const BATCH_BUCKETS: &[f64] = &[1.0, 2.0, 5.0, 10.0, 20.0, 50.0, 100.0, 200.0, 500.0]; metrics_exporter_prometheus::PrometheusBuilder::new() .with_http_listener(([0, 0, 0, 0], port)) .set_buckets_for_metric( metrics_exporter_prometheus::Matcher::Suffix("_ms".to_string()), MS_BUCKETS, ) .expect("failed to configure _ms buckets") + .set_buckets_for_metric( + metrics_exporter_prometheus::Matcher::Full("core.raft.fsync.batch_entries".to_string()), + BATCH_BUCKETS, + ) + .expect("failed to configure batch_entries buckets") .install() .expect("failed to start Prometheus metrics exporter");🤖 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 `@benches/embedded-bench/src/main.rs` around lines 209 - 230, Update start_metrics_server to define and configure the standalone setup’s BATCH_BUCKETS for the core.raft.fsync.batch_entries metric, alongside the existing MS_BUCKETS configuration. Use the appropriate exact metric matcher and preserve the current exporter installation flow.Source: Linked repositories
benches/standalone-bench/docker/bench-suite.sh (1)
18-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer Bash arrays for passing multiple arguments.
Using an unquoted string variable to pass multiple distinct arguments relies on shell word splitting, which can be fragile. Consider using a Bash array to properly construct and pass the endpoints.
♻️ Proposed refactor
-EP="--endpoints http://$N1 --endpoints http://$N2 --endpoints http://$N3" +EP=(--endpoints "http://$N1" --endpoints "http://$N2" --endpoints "http://$N3") echo "================================================================" echo " d-engine Benchmark Suite" echo " Endpoints: $N1, $N2, $N3" echo " conns=$CONNS clients=$CLIENTS total=$TOTAL key-size=$KEY_SIZE value-size=$VALUE_SIZE" echo "================================================================" echo "" echo "--- [1/6] Single client write (10K) ---" -standalone-bench $EP \ +standalone-bench "${EP[@]}" \ --conns 1 --clients 1 --sequential-keys --total 10000 \(Please also apply
"${EP[@]}"to the otherstandalone-benchinvocations below)🤖 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 `@benches/standalone-bench/docker/bench-suite.sh` around lines 18 - 30, Replace the string-based EP definition with a Bash array containing each endpoint argument as separate elements, then update every standalone-bench invocation in the script to expand it with "${EP[@]}". Preserve the existing endpoint values and argument ordering.Source: Linters/SAST tools
benches/standalone-bench/src/main.rs (1)
209-209: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSemaphore is now effectively a no-op. The loop at Line 224 spawns exactly
cli.clientstasks, and each task holds at most one permit at a time (Line 233). Sizing the semaphore tocli.clientsmeans total permit demand can never exceed capacity, soacquire()never blocks and the semaphore no longer limits in-flight operations. Previouslycli.connsbounded concurrency to the connection-pool size. If the intent is to cap concurrency independently of the task count, keep a distinct, smaller bound; otherwise the semaphore can be removed.🤖 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 `@benches/standalone-bench/src/main.rs` at line 209, Update the semaphore initialization and concurrency flow around the task-spawning loop and permit acquisition so the limit remains meaningful: use a distinct bound based on the intended connection-pool cap, such as cli.conns, rather than cli.clients, or remove the semaphore and its acquisition if no independent cap is required. Preserve the existing task count while ensuring permits can actually constrain in-flight operations.d-engine-core/src/raft_role/role_state.rs (1)
252-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the test-only
update_voted_forduplicate
d-engine-core/src/raft_role/role_state.rs:253— this#[cfg(test)]overload duplicatescommit_hard_state; call the shared helper directly so the persistence logic stays in one place.🤖 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-core/src/raft_role/role_state.rs` around lines 252 - 267, Remove the test-only update_voted_for method from the role state implementation. Update its test callers to invoke the existing commit_hard_state helper directly, preserving the current hard-state update and persistence behavior in that shared implementation.d-engine-server/tests/failover_and_recovery/sm_wal_disabled_crash_recovery_embedded.rs (1)
165-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comments: the code now uses graceful
stop()(Line 169), but Lines 165-168 and 171 still describemem::forget/"the forgotten engine" from a prior approach. Update the wording to avoid confusing future readers.🤖 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/tests/failover_and_recovery/sm_wal_disabled_crash_recovery_embedded.rs` around lines 165 - 172, Update the comments surrounding crashed_engine.stop() to describe the current graceful shutdown and resource-release behavior, removing references to mem::forget, forgotten engines, and obsolete leaked-task behavior. Keep the comments focused on why the stop call and subsequent wait are needed.d-engine-core/src/storage/buffered_raft_log_test/mod.rs (1)
23-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest module declarations were moved to
buffered_raft_log.rs(Lines 1433-1500) via#[path]; commenting them out here avoids double-inclusion. This leaves the file as effectively dead code (doc comment + commented decls only). Consider deleting it or itsmodreference in a follow-up to avoid confusion.🤖 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-core/src/storage/buffered_raft_log_test/mod.rs` around lines 23 - 39, Remove the now-unused `buffered_raft_log_test/mod.rs` module file or eliminate its remaining module reference, since the test declarations are already included from `buffered_raft_log.rs` via `#[path]`. Clean up the commented-out declarations so there is no dead test-module entry point.d-engine-core/src/storage/buffered_raft_log.rs (1)
718-720: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline the load here to avoid the self-recursion ambiguity.
self.is_poisoned()resolves to the inherent helper today, but it reads like recursive delegation and becomes a stack overflow if that helper ever changes.♻️ Suggested clarity fix
- self.is_poisoned() + self.poisoned.load(Ordering::Relaxed)🤖 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-core/src/storage/buffered_raft_log.rs` around lines 718 - 720, Update the is_poisoned method to inline the underlying poison-state load instead of calling self.is_poisoned(), eliminating recursive-looking delegation while preserving the existing boolean result.
🤖 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/state_machine_handler/default_state_machine_handler.rs`:
- Around line 281-283: Add the node_id label using self.node_id.to_string() to
both affected metrics in default_state_machine_handler.rs: the
core.raft.apply_index gauge at lines 281-283 and the
core.state_machine.apply.busy_nanos_total counter at lines 247-251. Preserve
their existing values and increment behavior while ensuring each metric is
scoped per node.
- Around line 264-270: Update the duration value assignment immediately before
the latency histogram in the state machine handler to convert the duration with
as_secs_f64() multiplied by 1000.0, preserving sub-millisecond precision for
core.state_machine.apply_chunk.duration_ms. Leave the adjacent batch_size
histogram unchanged.
In `@d-engine-core/src/storage/fsync_coordinator.rs`:
- Around line 79-85: Reset the `core.raft.fsync.inflight` gauge in the poisoned
early-return branch alongside `self.inflight.store(false, Ordering::Release)`.
Ensure the cleanup also occurs after fsync/persist failures transition the
coordinator to poisoned, matching the gauge reset performed by the caught-up
path.
In `@d-engine-server/src/network/grpc/grpc_raft_service.rs`:
- Around line 472-474: Preserve fractional millisecond precision in all five
latency metric sites: update
d-engine-server/src/network/grpc/grpc_raft_service.rs lines 472-474, 560-561,
and 589-590 to use elapsed.as_secs_f64() multiplied by 1000.0; update
d-engine-server/src/storage/adaptors/file/file_storage_engine.rs lines 395-396
and d-engine-server/src/storage/adaptors/rocksdb/rocksdb_storage_engine.rs lines
440-441 similarly, storing the resulting f64 and removing the redundant as f64
conversion from record().
In `@d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs`:
- Around line 928-931: Update the shutdown persistence flow around
save_hard_state(), close_db(), and Drop so the database flush completes before
hard state is persisted. Preserve the existing write batch behavior, but reorder
the calls to ensure last_applied is not saved before its corresponding
state-machine batch is durable.
In `@examples/three-nodes-standalone/docker/config/n2.toml`:
- Line 24: Update the lease_duration_ms setting in n2.toml from 100 to 500 so
the three-node standalone demo uses a consistent lease duration across n1, n2,
and n3.
In `@examples/three-nodes-standalone/Makefile`:
- Around line 23-25: Update the SNAPPY environment guard near BREW_ROCKSDB_ENV
to require SNAPPY_PREFIX to be non-empty before checking for its lib directory,
matching the existing LZ4/ZSTD pattern. Only add SNAPPY_LIB_DIR when both
conditions are satisfied, preserving the no-op behavior when Homebrew or the
library is absent.
---
Nitpick comments:
In `@benches/embedded-bench/src/main.rs`:
- Around line 209-230: Update start_metrics_server to define and configure the
standalone setup’s BATCH_BUCKETS for the core.raft.fsync.batch_entries metric,
alongside the existing MS_BUCKETS configuration. Use the appropriate exact
metric matcher and preserve the current exporter installation flow.
In `@benches/standalone-bench/docker/bench-suite.sh`:
- Around line 18-30: Replace the string-based EP definition with a Bash array
containing each endpoint argument as separate elements, then update every
standalone-bench invocation in the script to expand it with "${EP[@]}". Preserve
the existing endpoint values and argument ordering.
In `@benches/standalone-bench/src/main.rs`:
- Line 209: Update the semaphore initialization and concurrency flow around the
task-spawning loop and permit acquisition so the limit remains meaningful: use a
distinct bound based on the intended connection-pool cap, such as cli.conns,
rather than cli.clients, or remove the semaphore and its acquisition if no
independent cap is required. Preserve the existing task count while ensuring
permits can actually constrain in-flight operations.
In `@d-engine-core/src/membership.rs`:
- Line 230: Remove the commented-out metrics counter line near the membership
logic; do not replace it or alter surrounding code.
In `@d-engine-core/src/raft_role/role_state.rs`:
- Around line 252-267: Remove the test-only update_voted_for method from the
role state implementation. Update its test callers to invoke the existing
commit_hard_state helper directly, preserving the current hard-state update and
persistence behavior in that shared implementation.
In `@d-engine-core/src/storage/buffered_raft_log_test/mod.rs`:
- Around line 23-39: Remove the now-unused `buffered_raft_log_test/mod.rs`
module file or eliminate its remaining module reference, since the test
declarations are already included from `buffered_raft_log.rs` via `#[path]`.
Clean up the commented-out declarations so there is no dead test-module entry
point.
In `@d-engine-core/src/storage/buffered_raft_log.rs`:
- Around line 718-720: Update the is_poisoned method to inline the underlying
poison-state load instead of calling self.is_poisoned(), eliminating
recursive-looking delegation while preserving the existing boolean result.
In
`@d-engine-server/tests/failover_and_recovery/sm_wal_disabled_crash_recovery_embedded.rs`:
- Around line 165-172: Update the comments surrounding crashed_engine.stop() to
describe the current graceful shutdown and resource-release behavior, removing
references to mem::forget, forgotten engines, and obsolete leaked-task behavior.
Keep the comments focused on why the stop call and subsequent wait are needed.
In `@d-engine/src/docs/performance/metrics-reference.md`:
- Around line 54-56: Update the fenced code block containing the
propose-to-apply metrics equation to specify the text language, preserving the
equation content 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: eee699a7-e195-40d2-8e23-8f2766aba310
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (87)
.dockerignoreCHANGELOG.mdMakefilebenches/embedded-bench/Cargo.tomlbenches/embedded-bench/Makefilebenches/embedded-bench/src/main.rsbenches/reports/v0.2.5/bench_report_v0.2.5.mdbenches/standalone-bench/Makefilebenches/standalone-bench/docker/Dockerfilebenches/standalone-bench/docker/README.mdbenches/standalone-bench/docker/bench-suite.shbenches/standalone-bench/src/main.rsd-engine-core/Cargo.tomld-engine-core/src/config/monitoring.rsd-engine-core/src/config/raft.rsd-engine-core/src/election/election_handler.rsd-engine-core/src/membership.rsd-engine-core/src/raft.rsd-engine-core/src/raft_role/buffers/batch_buffer.rsd-engine-core/src/raft_role/buffers/batch_buffer_test.rsd-engine-core/src/raft_role/buffers/propose_batch_buffer.rsd-engine-core/src/raft_role/buffers/propose_batch_buffer_test.rsd-engine-core/src/raft_role/candidate_state.rsd-engine-core/src/raft_role/candidate_state_test.rsd-engine-core/src/raft_role/follower_state.rsd-engine-core/src/raft_role/follower_state_test.rsd-engine-core/src/raft_role/leader_state.rsd-engine-core/src/raft_role/leader_state_test/backpressure_test.rsd-engine-core/src/raft_role/leader_state_test/become_follower_test.rsd-engine-core/src/raft_role/leader_state_test/event_handling_test.rsd-engine-core/src/raft_role/leader_state_test/membership_change_test.rsd-engine-core/src/raft_role/leader_state_test/pending_lease_reads_test.rsd-engine-core/src/raft_role/leader_state_test/replication_test.rsd-engine-core/src/raft_role/leader_state_test/state_management_test.rsd-engine-core/src/raft_role/learner_state.rsd-engine-core/src/raft_role/mod.rsd-engine-core/src/raft_role/role_state.rsd-engine-core/src/raft_role/role_state_test.rsd-engine-core/src/raft_test/raft_comprehensive_tests.rsd-engine-core/src/replication/mod.rsd-engine-core/src/state_machine_handler/default_state_machine_handler.rsd-engine-core/src/storage/buffered_raft_log.rsd-engine-core/src/storage/buffered_raft_log_test/concurrent_fsync_test.rsd-engine-core/src/storage/buffered_raft_log_test/drain_fsync_test.rsd-engine-core/src/storage/buffered_raft_log_test/id_allocation_test.rsd-engine-core/src/storage/buffered_raft_log_test/mod.rsd-engine-core/src/storage/buffered_raft_log_test/performance_test.rsd-engine-core/src/storage/buffered_raft_log_test/pipeline_overlap_test.rsd-engine-core/src/storage/buffered_raft_log_test/shutdown_test.rsd-engine-core/src/storage/fsync_coordinator.rsd-engine-core/src/storage/fsync_coordinator_test.rsd-engine-core/src/storage/mod.rsd-engine-core/src/storage/raft_log.rsd-engine-core/src/storage/state_machine.rsd-engine-core/src/test_utils/buffered_raft_log_test_helpers.rsd-engine-core/src/test_utils/log_capture.rsd-engine-core/src/test_utils/mock/mock_raft_builder.rsd-engine-core/src/test_utils/mock/mock_storage_engine.rsd-engine-core/src/test_utils/mod.rsd-engine-server/src/api/embedded_client.rsd-engine-server/src/network/grpc/grpc_raft_service.rsd-engine-server/src/network/grpc/grpc_transport.rsd-engine-server/src/node/builder_test.rsd-engine-server/src/read_actor.rsd-engine-server/src/storage/adaptors/file/file_storage_engine.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_storage_engine.rsd-engine-server/src/storage/lease_integration_test.rsd-engine-server/src/test_utils/integration/mod.rsd-engine-server/tests/failover_and_recovery/mod.rsd-engine-server/tests/failover_and_recovery/sm_wal_disabled_crash_recovery_embedded.rsd-engine-server/tests/storage_buffered_raft_log/crash_recovery_test.rsd-engine-server/tests/storage_buffered_raft_log/mod.rsd-engine-server/tests/storage_buffered_raft_log/performance_test.rsd-engine/src/docs/performance/metrics-reference.mdd-engine/src/docs/performance/throughput-optimization-guide.mdexamples/three-nodes-standalone/Makefileexamples/three-nodes-standalone/docker/README.mdexamples/three-nodes-standalone/docker/config/n1.tomlexamples/three-nodes-standalone/docker/config/n2.tomlexamples/three-nodes-standalone/docker/config/n3.tomlexamples/three-nodes-standalone/docker/docker-compose.ymlexamples/three-nodes-standalone/docker/monitoring/docker-compose.ymlexamples/three-nodes-standalone/docker/monitoring/grafana/dashboards/dashboard.jsonexamples/three-nodes-standalone/docker/monitoring/prometheus/prometheus.ymlexamples/three-nodes-standalone/src/main.rs
💤 Files with no reviewable changes (4)
- d-engine-core/src/config/monitoring.rs
- d-engine-core/src/storage/state_machine.rs
- d-engine-core/src/election/election_handler.rs
- d-engine-core/src/raft_role/leader_state_test/backpressure_test.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
last_applied was persisted separately from SM data, and flush() targeted an unused CF — a crash between the two could lose flushed data while last_applied claimed it was applied. Now written in the same WriteBatch; flush() targets the correct CFs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
d-engine-server/tests/failover_and_recovery/sm_metadata_atomicity_embedded.rs (1)
12-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest honestly documents it can't regress-guard the atomicity claim — consider a white-box complement.
The docstring correctly discloses this test passes both before and after the fix because RocksDB's
CancelAllBackgroundWorkauto-flushesdisableWALmemtables on close, masking the pre-fix bug. As written, this is end-to-end coverage of the mechanism, not a regression guard for the actual invariant (last_appliedand SM data can never diverge on disk).Consider adding a lower-level unit test (in
rocksdb_state_machine_test.rs) that inspectsapply_chunk'sWriteBatchWithIndexcontents directly — asserting thelast_applied_index/termputs toSTATE_MACHINE_META_CFand the SM data puts are present in the same batch before the singlewrite_wbwi_opt()call — as a deterministic regression guard that doesn't depend on RocksDB's shutdown-flush behavior.Also applies to: 98-169
🤖 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/tests/failover_and_recovery/sm_metadata_atomicity_embedded.rs` around lines 12 - 18, Add a white-box unit test in rocksdb_state_machine_test.rs covering apply_chunk that inspects the WriteBatchWithIndex before write_wbwi_opt() executes. Assert the STATE_MACHINE_META_CF last_applied_index and term writes and the state-machine data writes are all present in the same batch, preserving the single-write atomicity invariant while leaving the existing end-to-end test unchanged.
🤖 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/tests/failover_and_recovery/sm_metadata_atomicity_embedded.rs`:
- Around line 12-18: Add a white-box unit test in rocksdb_state_machine_test.rs
covering apply_chunk that inspects the WriteBatchWithIndex before
write_wbwi_opt() executes. Assert the STATE_MACHINE_META_CF last_applied_index
and term writes and the state-machine data writes are all present in the same
batch, preserving the single-write atomicity invariant while leaving the
existing end-to-end test unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60031421-a8fa-4c72-b10f-916fd56d5b34
📒 Files selected for processing (14)
Makefilebenches/embedded-bench/Makefilebenches/standalone-bench/Makefiled-engine-core/src/state_machine_handler/default_state_machine_handler.rsd-engine-core/src/storage/fsync_coordinator.rsd-engine-server/src/network/grpc/grpc_raft_service.rsd-engine-server/src/storage/adaptors/file/file_storage_engine.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_storage_engine.rsd-engine-server/tests/failover_and_recovery/mod.rsd-engine-server/tests/failover_and_recovery/sm_metadata_atomicity_embedded.rsexamples/three-nodes-standalone/Makefileexamples/three-nodes-standalone/docker/config/n2.toml
💤 Files with no reviewable changes (1)
- d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine_test.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- d-engine-server/tests/failover_and_recovery/mod.rs
- examples/three-nodes-standalone/docker/config/n2.toml
- Makefile
- d-engine-server/src/network/grpc/grpc_raft_service.rs
- examples/three-nodes-standalone/Makefile
- d-engine-server/src/storage/adaptors/file/file_storage_engine.rs
- benches/standalone-bench/Makefile
- d-engine-core/src/state_machine_handler/default_state_machine_handler.rs
- benches/embedded-bench/Makefile
- d-engine-core/src/storage/fsync_coordinator.rs
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?
Fix the WAL fsync serialization regression introduced by #407: the IO thread blocked inline on fsync, causing every entry to pay the full fsync penalty individually. Introduces
FsyncCoordinator— a concurrent fsync pipeline with storage-layer group commit. Also fixes Raft hard_state persistence ordering (Candidate term/vote durability before broadcast).Type:
Why Is This Needed?
Problem (#422): #407 enabled per-batch
flush_wal(true)(Level 3 durability), but the IO thread called fsync inline — blocking the entire event loop. Under moderate load or high fsync latency, group commit never formed: every entry paid the full fsync penalty individually. Throughput collapsed from hundreds of ops/s to single digits.Fix:
FsyncCoordinatordispatches fsync tospawn_blocking, IO thread returns immediately. CAS-gated single-inflight ensures entries arriving during a running fsync are coalesced into the same physical disk flush — storage-layer group commit, no artificial batching window needed.Additional correctness fix: Candidate's term increment and self-vote now persist via
commit_hard_state()before broadcasting vote requests. Same for higher-term discovery before stepping down to Follower. Without this, a crash between "act on term change" and "persist it" resurrects the stale term on restart.Checklist
Required:
make testpassesIf changing APIs:
Testing
How tested:
Unit tests:
fsync_coordinator_test.rs— 15 tests: CAS behavior, fence_reset, coalescing, error propagation, generation discardconcurrent_fsync_test.rs— 10 tests: inflight coalescing, out-of-order completion, shutdown, reset-during-fsync, post-reset writescandidate_state_test.rs— 2 new tests: persist hard_state before broadcasting votes, persist hard_state on higher-term discoveryrocksdb_state_machine_test.rs— 6 tests: single-key get_multi fast pathdrain_fsync_test.rs— expanded shutdown/flush tests for new pipelineIntegration tests:
sm_wal_disabled_crash_recovery_embedded.rs— 477 lines: crash-recovery correctness with WAL disabledManual testing:
For bug fixes:
For performance improvements:
benches/reports/v0.2.5/bench_report_v0.2.5.md)Does This Follow d-engine's Principles?
FsyncCoordinatoris ~180 lines, single responsibility, no new dependenciesshutdown_timeout_ms),MetricsConfigremoved (net reduction)Reviewer Notes
Focus areas:
FsyncCoordinator::run_until_caught_up— the inflight re-arm after clearing (narrow race window between swap andinflight.store(false))fence_reset()+ generation check inrun_until_caught_up— prevents stale durable_index resurrection (perf: fix WAL fsync serialization regression introduced by #407 #422-followup-todos-0714 TODO 1)append_entries,save_hard_state,persist_pending_range,ReplaceRange,Purge)commit_hard_statecalled before broadcast/step-down in all three term-change pathsEstimated review complexity:
buffered_raft_log.rs+ 180 lines newfsync_coordinator.rs. The rest is tests, docs, bench infra, and mechanical metrics rename.Summary by CodeRabbit
New Features
shutdown_timeout_msto persistence and expanded benchmark + crash-recovery workflows (including WAL-disabled state machine scenarios).Bug Fixes
get_multifast path and added additional storage/flush timing/accuracy metrics.Documentation