Repository navigation
fix #315: isolate snapshot disk I/O from Raft event loop via spawn_blocking - #362
Conversation
…ocking RocksDB snapshot generation (create_dir_all, flush_cf_opt, export_column_family) was executing on tokio async worker threads, competing directly with the Raft event loop for thread pool capacity. Under concurrent write load this caused AppendEntries acks to be delayed past the proposal timeout, producing intermittent 4006 ProposeFailed errors in test_learner_snapshot_concurrent_replication. Move all blocking snapshot I/O into tokio::task::spawn_blocking so it runs on the dedicated blocking thread pool, freeing async workers for Raft protocol processing. Use load_full() to obtain Arc<DB> (Send) for safe transfer into the closure. Also document the minimum 2-core CPU recommendation in CHANGELOG, as single-core machines conflate all thread pools onto one CPU and exhibit the same starvation pattern regardless of spawn_blocking.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 23 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded operational notes to CHANGELOG recommending >=2 CPU cores per node; refactored Changes
Sequence DiagramsequenceDiagram
participant Async as Async Context
participant Tokio as Tokio Runtime
participant Block as Blocking Thread Pool
participant RocksDB as RocksDB + Filesystem
participant Err as Error Mapper
Async->>Tokio: call generate_snapshot_data()
Tokio->>Tokio: prepare cloned dir & DB handle
Tokio->>Block: spawn_blocking(snapshot task)
Block->>RocksDB: create directories
Block->>RocksDB: flush column families
Block->>RocksDB: export checkpoints (sm, sm_meta)
Block->>RocksDB: write export metadata
Block-->>Tokio: return Result<(), io/db error>
Tokio->>Err: await join, map panic/cancel -> StorageError
Err-->>Async: return Result<(), StorageError>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs`:
- Around line 911-912: The current map_err treats all JoinError as "panicked";
update the closure that handles the .await result for the snapshot blocking task
to distinguish cancellation vs panic by inspecting the JoinError (use
e.is_cancelled() and e.is_panic() on the JoinError), returning a
StorageError::DbError with a clear "snapshot blocking task cancelled" message
for cancelled tasks and preserving the "panicked" message (including error
details) for actual panics; replace the single map_err(|e|
StorageError::DbError(format!("snapshot blocking task panicked: {e}")))?? with a
match/if that checks e.is_cancelled() first, then e.is_panic(), and otherwise
includes the JoinError info.
🪄 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: c3a598d2-5521-4867-aef3-6ef7e5e9b995
⛔ Files ignored due to path filters (3)
examples/client-usage-standalone/Cargo.lockis excluded by!**/*.lockexamples/single-node-expansion/Cargo.lockis excluded by!**/*.lockexamples/three-nodes-embedded/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
CHANGELOG.mdd-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
55d9a71 to
59e74bf
Compare
1 participant
What Does This PR Do?
Moves all blocking RocksDB snapshot I/O (
create_dir_all,flush_cf_opt,export_column_family) intotokio::task::spawn_blocking, preventing snapshotgeneration from starving the Raft event loop's async worker threads and causing
intermittent 4006 ProposeFailed errors under concurrent write load.
Type:
Why Is This Needed?
create_snapshot()inRocksDBStateMachineperformed blocking disk I/O directlyon tokio async worker threads — the same pool that drives the Raft event loop.
Under concurrent write load (e.g. parallel test runs), these blocking operations
consumed async workers long enough to delay AppendEntries ack processing past the
proposal timeout, producing intermittent
Error code: 4006(ProposeFailed) intest_learner_snapshot_concurrent_replication.The fix wraps all blocking ops in
spawn_blocking, which offloads them to tokio'sdedicated blocking thread pool, keeping async workers free for Raft protocol
processing.
load_full()is used to obtainArc<DB>(Send) for safe transferinto the closure.
Also adds a minimum 2-core CPU recommendation to CHANGELOG: on single-core machines
all thread pools share one CPU and exhibit the same starvation regardless of
spawn_blocking.Checklist
Required:
make testpassesTesting
How tested:
test_learner_snapshot_concurrent_replication— previouslyfailed intermittently with 4006 under
make test(parallel run); now passesreliably because snapshot I/O no longer competes with the Raft event loop.
For bug fixes:
test_learner_snapshot_concurrent_replication)Does This Follow d-engine's Principles?
Reviewer Notes
The change is a mechanical wrapping of an existing synchronous block inside
spawn_blocking. The only subtle point isdb.load_full()vsdb.load()—load_full()returnsArc<DB>which isSendand can be moved into the closure,while
load()returns a guard that is not.Estimated review complexity:
Summary by CodeRabbit
Documentation
Refactor