Repository navigation
perf(read) #390: linearizable read lease fast path + fix Raft lease clock (SystemTime → Instant) - #391
Merged
Conversation
…lock (SystemTime → Instant)
…e perf numbers and MIGRATION_GUIDE
📝 WalkthroughWalkthroughThis PR releases d-engine v0.2.4: adds a lease-based linearizable-read fast path for multi-voter leaders, refactors leader lease tracking to use monotonic Instants, enforces strict lease/election timing validation, updates read dispatch logic, adds regression tests, and updates docs, changelog, versions, and benchmarks. Changesd-engine v0.2.4 Release: Linearizable Read Lease Fast Path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 90: The changelog line describing StateMachine::apply_chunk should use
American English "implementers" instead of "implementors"; update the sentence
that reads "Custom state machine implementors must update their `impl`." to
"Custom state machine implementers must update their `impl`." while keeping
references to `StateMachine::apply_chunk` and `ApplyEntry` intact.
🪄 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: cfd0e842-7ac6-42e0-bc37-e5d8e7b1530e
⛔ Files ignored due to path filters (7)
Cargo.lockis excluded by!**/*.lockbenches/reports/v0.2.4/d-engine_comparison_v0.2.4.pngis excluded by!**/*.pngbenches/reports/v0.2.4/d-engine_v0.2.3_vs_v0.2.4_embedded_mode.pngis excluded by!**/*.pngbenches/reports/v0.2.4/d-engine_v0.2.3_vs_v0.2.4_standalone_mode.pngis excluded by!**/*.pngexamples/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 (10)
CHANGELOG.mdCargo.tomlMIGRATION_GUIDE.mdREADME.mdbenches/reports/v0.2.4/bench_report_v0.2.4.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/pending_reads_test.rsd-engine/src/docs/performance/benchmarking-guide.md
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
benches/embedded-bench/Makefile (1)
4-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
clean-log-dbto.PHONYto avoid accidental no-op runs.
clean-log-dbshould be declared phony; otherwisemakemay skip it if a file/dir with that name exists. Update Line 4 to includeclean-log-db.Also applies to: 73-77
🤖 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/Makefile` at line 4, Add the missing phony declaration for the Makefile target by including "clean-log-db" in the .PHONY list (the .PHONY line that currently lists help build clean test-single-write ... all-tests) so make won't skip the target if a file/dir named clean-log-db exists; also add "clean-log-db" to the other .PHONY declaration(s) around the later block that lists targets (the block that includes test-high-conc-write test-linearizable-read test-lease-read test-eventual-read test-hot-key) to ensure all occurrences declare the target as phony.benches/reports/v0.2.4/bench_report_v0.2.4.md (1)
99-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix likely broken image filename in markdown link.
Line 99 includes a space in the asset name (
...v0.2.4_ vs...), which is likely an invalid path and will break image rendering.🤖 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/reports/v0.2.4/bench_report_v0.2.4.md` at line 99, The markdown image link contains an unintended space in the asset name ("d-engine_v0.2.4_ vs_etcd_3.2.0.png") which will break rendering; fix the filename in the image reference used in the README line (the `` entry) by removing or replacing the space (e.g., `d-engine_v0.2.4_vs_etcd_3.2.0.png`) or by URL-encoding/quoting the path so it matches the actual asset filename.
🤖 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.
Outside diff comments:
In `@benches/embedded-bench/Makefile`:
- Line 4: Add the missing phony declaration for the Makefile target by including
"clean-log-db" in the .PHONY list (the .PHONY line that currently lists help
build clean test-single-write ... all-tests) so make won't skip the target if a
file/dir named clean-log-db exists; also add "clean-log-db" to the other .PHONY
declaration(s) around the later block that lists targets (the block that
includes test-high-conc-write test-linearizable-read test-lease-read
test-eventual-read test-hot-key) to ensure all occurrences declare the target as
phony.
In `@benches/reports/v0.2.4/bench_report_v0.2.4.md`:
- Line 99: The markdown image link contains an unintended space in the asset
name ("d-engine_v0.2.4_ vs_etcd_3.2.0.png") which will break rendering; fix the
filename in the image reference used in the README line (the `` entry) by removing or
replacing the space (e.g., `d-engine_v0.2.4_vs_etcd_3.2.0.png`) or by
URL-encoding/quoting the path so it matches the actual asset filename.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8dcaf04-b292-4e79-97c7-05d609c655bc
📒 Files selected for processing (5)
README.mdbenches/embedded-bench/Makefilebenches/reports/v0.2.4/bench_report_v0.2.4.mdd-engine/src/docs/use-cases.mdexamples/three-nodes-standalone/Makefile
✅ Files skipped from review due to trivial changes (2)
- examples/three-nodes-standalone/Makefile
- README.md
Merged
10 of 12 tasks
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?
Adds a lease fast path for
LinearizableRead: when the leader holds a valid leaseand
last_applied >= read_index, reads are served without a consensus round-trip.Also fixes a correctness bug where the lease clock used
SystemTime(susceptible toNTP step-backs) instead of
Instant(always monotonic).Type:
Why Is This Needed?
Issue: #390
LinearizableReadin multi-voter clusters always required a full consensus round-trip(Raft §8 step 2). Under high concurrency this becomes the throughput bottleneck.
Raft §6.4 allows the leader to skip the round-trip when it holds a valid lease —
"valid" meaning a quorum ACK arrived within
lease_duration_ms, which proves nohigher-term leader can exist at that instant. The existing
LeaseReadpath alreadyhad the concept; this PR extends it to
LinearizableRead.Two bugs fixed in the process:
lease_timestamp: AtomicU64stored a rawSystemTimeepoch. NTP stepping theclock backward extends the lease window silently → potential stale read. Fixed to
Mutex<Option<Instant>>.warn!forlease_duration_ms >= election_timeout_min.This bound is load-bearing for safety; changed to a hard
ConfigError.Checklist
Required:
make testpassesIf changing APIs:
Testing
How tested:
pending_reads_test.rs— 199 lines covering: lease-valid fast path,lease-expired slow path, single-voter path,
last_applied < read_indexstill queues,clock monotonicity (Instant vs SystemTime)
raft_test.rs— 54 lines covering config validation (lease ≥ electiontimeout must error)
For performance improvements:
Benchmark (AWS EC2 3-node c5.2xlarge, 8 vCPU/16GB, key=8B val=256B):
Full report:
benches/reports/v0.2.4/bench_report_v0.2.4.mdDoes This Follow d-engine's Principles?
Reviewer Notes
The safety argument for the fast path rests on two invariants enforced at config load:
lease_duration_ms < election_timeout_min(now a hard error, not a warning)Instant— NTP cannot rewind itThe fast path touches only
handle_linearizable_read_requestsinleader_state.rs.is_lease_valid()is the single point of truth; it returnsfalsewhenlease_instantisNone(no quorum ACK received yet), ensuring cold-start safety.Estimated review complexity:
Summary by CodeRabbit
New Features
Bug Fixes
Changed
Documentation
Chores