Feature/172 ttl support - #176
Conversation
…&mut dyn Any;" coding style"
…shutdown - Remove TTL persist from apply_chunk hot path (zero overhead) - Add TTL persist to graceful shutdown and snapshot - Isolate benchmark measurements for accurate profiling - Simplify benchmark workflow (remove third-party action) - Improve Arc::get_mut error handling in builder Performance improvements: - Apply_chunk: eliminated TTL persist I/O overhead - Benchmark accuracy: 99.997% improvement (isolated cleanup logic)
- Add explicit stable Rust toolchain installation - Use --locked flag for cargo-deny to avoid version conflicts - Fixes: smol_str@0.3.4 requires rustc 1.89 error
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. WalkthroughThis PR introduces TTL/lease-based expiration support across the storage layer, refactors the node startup sequence into a unified Changes
Sequence DiagramsequenceDiagram
actor User
participant NodeBuilder
participant StateMachine
participant Lease
participant RaftNode
User->>NodeBuilder: build()
rect rgba(0, 100, 150, 0.1)
Note over NodeBuilder: Lease Injection & Init
NodeBuilder->>StateMachine: try_inject_lease(config)
StateMachine->>Lease: new(config)
NodeBuilder->>StateMachine: start()
NodeBuilder->>StateMachine: post_start_init()
StateMachine->>Lease: load_lease_data()
end
NodeBuilder->>RaftNode: Create
NodeBuilder-->>User: Ok(Self)
rect rgba(0, 150, 100, 0.1)
Note over User: Application calls start_server()
User->>NodeBuilder: start_server()
NodeBuilder->>NodeBuilder: build().await
NodeBuilder->>RaftNode: start_rpc_server()
NodeBuilder->>RaftNode: ready()
RaftNode-->>NodeBuilder: Ok(())
end
NodeBuilder-->>User: Ok(Arc<Node>)
rect rgba(150, 100, 0, 0.1)
Note over RaftNode: Lease-aware Operations
User->>RaftNode: put_with_ttl(key, value, ttl)
RaftNode->>StateMachine: apply_chunk(Insert{ttl_secs})
StateMachine->>Lease: register(key, ttl_secs)
User->>RaftNode: get(key)
RaftNode->>StateMachine: get(key)
StateMachine->>Lease: is_expired(key)
alt Expired
StateMachine->>Lease: unregister(key)
StateMachine-->>RaftNode: None
else Valid
StateMachine-->>RaftNode: Some(value)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements TTL (time-to-live) support for d-engine, adding automatic key expiration through a lease-based lifecycle management system. The implementation includes a simplified NodeBuilder API, comprehensive testing infrastructure, and performance benchmarking.
- Adds TTL/Lease functionality with multiple cleanup strategies (disabled, passive, piggyback, background)
- Simplifies NodeBuilder API:
.build().start_rpc_server().await.ready()→.start_server().await - Implements lease injection framework with
try_inject_lease()andpost_start_init()hooks - Adds comprehensive integration tests and performance benchmarks
Reviewed Changes
Copilot reviewed 47 out of 51 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| d-engine-proto/proto/client/client_api.proto | Adds optional ttl_secs field to Insert message |
| d-engine-core/src/storage/lease.rs | Defines Lease trait for TTL management |
| d-engine-server/src/storage/lease.rs | Implements DefaultLease with dual-index architecture |
| d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs | Adds TTL support to RocksDB state machine |
| d-engine-server/src/storage/adaptors/file/file_state_machine.rs | Adds TTL support to File state machine |
| d-engine-server/src/node/builder.rs | Implements unified start_server() API |
| d-engine-core/src/config/raft.rs | Adds LeaseConfig with cleanup strategies |
| d-engine-server/benches/ttl.rs | TTL-specific performance benchmarks |
| d-engine-server/benches/state_machine.rs | State machine performance benchmarks |
| d-engine-client/src/kv.rs | Adds put_with_ttl() client method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn open_db<P: AsRef<Path>>(path: P) -> Result<DB, Error> { | ||
| // Same options as new() | ||
| let mut opts = Options::default(); | ||
| opts.create_if_missing(true); | ||
| opts.create_missing_column_families(true); | ||
|
|
||
| // Memory and write optimization | ||
| opts.set_max_write_buffer_number(4); | ||
| opts.set_min_write_buffer_number_to_merge(2); | ||
| opts.set_write_buffer_size(128 * 1024 * 1024); | ||
|
|
||
| // Compression optimization | ||
| opts.set_compression_type(rocksdb::DBCompressionType::Lz4); | ||
| opts.set_bottommost_compression_type(rocksdb::DBCompressionType::Zstd); | ||
| opts.set_compression_options(-14, 0, 0, 0); | ||
|
|
||
| // WAL-related optimizations | ||
| opts.set_wal_bytes_per_sync(1024 * 1024); | ||
| opts.set_manual_wal_flush(true); | ||
| opts.set_use_fsync(false); | ||
|
|
||
| // Performance Tuning | ||
| opts.set_max_background_jobs(4); | ||
| opts.set_max_open_files(5000); | ||
| opts.set_use_direct_io_for_flush_and_compaction(true); | ||
| opts.set_use_direct_reads(true); | ||
|
|
||
| // Leveled Compaction Configuration | ||
| opts.set_level_compaction_dynamic_level_bytes(true); | ||
| opts.set_target_file_size_base(64 * 1024 * 1024); | ||
| opts.set_max_bytes_for_level_base(256 * 1024 * 1024); | ||
|
|
||
| // Block cache configuration | ||
| let cache = Cache::new_lru_cache(128 * 1024 * 1024); | ||
| opts.set_row_cache(&cache); | ||
|
|
||
| let cfs = vec![STATE_MACHINE_CF, STATE_MACHINE_META_CF]; | ||
| DB::open_cf(&opts, path, cfs).map_err(|e| StorageError::DbError(e.to_string()).into()) | ||
| } |
There was a problem hiding this comment.
The open_db() function duplicates all RocksDB configuration logic from new(). Consider extracting the options configuration into a shared helper function (e.g., configure_db_options()) to maintain DRY principle and ensure consistency between both initialization paths.
| let ttl_secs = if pos + 4 <= buffer.len() { | ||
| let ttl = u32::from_be_bytes(buffer[pos..pos + 4].try_into().unwrap()); | ||
| pos += 4; | ||
| if ttl > 0 { Some(ttl) } else { None } | ||
| } else { | ||
| // Backward compatibility: if no TTL field, assume no TTL | ||
| debug!( | ||
| "No TTL field at position {}, assuming no TTL (backward compatibility)", | ||
| pos | ||
| ); | ||
| None | ||
| }; |
There was a problem hiding this comment.
The backward compatibility logic for WAL entries without TTL fields is critical for upgrades. Consider adding a comment explaining the migration path: how existing WAL entries (written before this PR) are handled during replay after upgrading to this version.
| if !(10..=10000).contains(&self.piggyback_frequency) { | ||
| return Err(Error::Config(ConfigError::Message(format!( | ||
| "piggyback_frequency must be between 10 and 10000, got {}", | ||
| self.piggyback_frequency | ||
| )))); | ||
| } |
There was a problem hiding this comment.
The piggyback_frequency validation range (10-10000) lacks justification in the validation logic. Consider documenting why these specific bounds were chosen (e.g., reference to benchmark results or design decisions) either in code comments or configuration documentation to help users understand the constraints.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
d-engine-server/Cargo.toml (1)
4-4: Invalid Rust edition specified.Rust edition "2024" does not exist. The latest stable edition is 2021. This will cause
cargo buildand all other cargo commands to fail immediately.Apply this diff to fix the edition:
-edition = "2024" +edition = "2021"d-engine-core/Cargo.toml (1)
4-4: Invalid Rust edition "2024".Rust supports editions
2015,2018, and2021. Edition2024does not exist and will cause the build to fail.Apply this diff to fix the edition:
-edition = "2024" +edition = "2021"examples/sled-cluster/src/sled_state_machine.rs (1)
229-244: TTL inserts are silently treated as non-expiring
ttl_secsis now part ofInsert, but this branch just discards it and writes the value permanently. Any client using the Sled-backed example with TTL will never see keys expire, undermining the new TTL/lease feature and leaking data. Please either integrate this path with the lease/TTL machinery (so the expiry is enforced) or reject TTL writes explicitly until support is landed.d-engine-server/src/node/builder.rs (1)
13-28: Update the builder docs to reflect the new async API.
build()is nowasync fn build(...) -> Result<Self>, yet the snippet still chains.build().start_rpc_server().await.ready()without awaiting or handling theResult. Anyone copying this will get a compiler error. Please update the example to either awaitbuild()(handling the error) or switch the sample entirely to the newstart_server().await?flow advertised in the bullet above so the docs stay consistent.d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs (1)
523-544: Ensure TTL removals clear existing leasesRe-inserting a key without a TTL (or with
ttl_secs = 0) should cancel any previous lease. The current logic only registers TTL whenttl_secsisSome(ttl > 0)and otherwise leaves the old lease in place, causing keys that were explicitly converted to non-TTL entries to still expire later. Please unregister the lease whenever no positive TTL is provided.Some(Operation::Insert(Insert { key, value, ttl_secs, })) => { batch.put_cf(&cf, &key, &value); - // Register TTL if specified - if let Some(ttl) = ttl_secs { - if ttl > 0 { - if let Some(ref lease) = self.lease { - lease.register(key.clone(), ttl); - } - } - } + if let Some(ref lease) = self.lease { + match ttl_secs { + Some(ttl) if ttl > 0 => lease.register(key.clone(), ttl), + _ => lease.unregister(&key), + } + }d-engine-server/src/storage/adaptors/file/file_state_machine.rs (1)
503-533: TTL metadata is dropped during WAL replay.
FileStateMachine::newcallsload_from_disk()→replay_wal()beforeset_lease()/try_inject_lease()ever runs, soself.leaseis guaranteed to beNonein this block. After a crash, any INSERT that was only present in the WAL (because we hadn’t persistedttl_state.binyet) replays here without re-registering its TTL and the key becomes permanent. Please inject the lease prior to replaying the WAL (e.g., construct the machine with anArc<DefaultLease>already set) or defer WAL replay until after the lease is installed so thoseregister/unregistercalls actually run.
🧹 Nitpick comments (4)
d-engine-core/Cargo.toml (1)
36-37: Clarify the stale comment reference.Line 36 references "Dashset" but the dependency is "dashmap". Clarify or update the comment for clarity.
-# using Dashset +# Concurrent hash map for lease trackingd-engine-server/benches/README.md (1)
1-172: LGTM!Excellent benchmark documentation that covers all aspects from overview to implementation details. The performance targets table (lines 81-86) is particularly helpful, and the example benchmark code provides a clear template for contributors.
You could optionally add language specifiers to the fenced code blocks at lines 67 and 106 to satisfy the markdown linter:
-``` +```text target/criterion/report/index.htmland ```diff -``` +```text https://[your-org].github.io/d-engine/dev/bench/This is purely cosmetic and doesn't affect functionality. </blockquote></details> <details> <summary>d-engine-server/benches/state_machine.rs (1)</summary><blockquote> `99-129`: **Consider state machine reuse for more accurate performance measurements.** The `bench_apply_without_ttl` and `bench_apply_with_ttl` benchmarks create a new `FileStateMachine` on every iteration, which includes filesystem operations and initialization overhead that may not reflect typical runtime performance where a state machine is long-lived. Consider moving the state machine creation outside the iteration loop, similar to the read benchmarks (lines 137-142): ```diff fn bench_apply_without_ttl(c: &mut Criterion) { let runtime = tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap(); + + // Setup state machine once before benchmark + let (sm, _temp_dir) = runtime.block_on(async { + create_test_state_machine().await + }); c.bench_function("apply_without_ttl", |b| { b.to_async(&runtime).iter(|| async { - let (sm, _temp_dir) = create_test_state_machine().await; - let entries = create_entries_without_ttl(1, 1); + // Use incrementing indices to avoid key conflicts + let entries = create_entries_without_ttl(1, black_box(1)); // Measure pure apply performance sm.apply_chunk(entries).await.unwrap(); black_box(()); }); }); }However, if the intent is to measure cold-start performance including initialization, the current approach is correct. Consider adding a comment clarifying the intent.
d-engine-server/benches/ttl.rs (1)
26-46: Async function doesn't require async operations before await.The
create_test_state_machinefunction is markedasync, but all operations before theFileStateMachine::newcall are synchronous. While functionally correct, this is a minor style inconsistency.This is acceptable as-is since the function is only called from async contexts, but you could simplify slightly:
-async fn create_test_state_machine() -> (FileStateMachine, TempDir) { +fn create_test_state_machine() -> impl std::future::Future<Output = (FileStateMachine, TempDir)> { + async move { use d_engine_server::storage::DefaultLease; let temp_dir = TempDir::new().expect("Failed to create temp dir"); // ... + } }Or keep as-is for simplicity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockd-engine-proto/src/generated/d_engine.client.rsis excluded by!**/generated/**examples/rocksdb-cluster/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
.github/workflows/benchmark.yml(1 hunks).github/workflows/ci.yml(1 hunks).github/workflows/dependency-audit.yml(2 hunks).gitignore(1 hunks)CHANGELOG.md(1 hunks)MIGRATION_GUIDE.md(1 hunks)Makefile(8 hunks)README.md(1 hunks)d-engine-client/src/kv.rs(1 hunks)d-engine-client/src/kv_test.rs(1 hunks)d-engine-core/Cargo.toml(1 hunks)d-engine-core/src/config/raft.rs(4 hunks)d-engine-core/src/errors.rs(1 hunks)d-engine-core/src/storage/lease.rs(1 hunks)d-engine-core/src/storage/mod.rs(1 hunks)d-engine-core/src/storage/state_machine.rs(2 hunks)d-engine-core/src/storage/state_machine_test.rs(4 hunks)d-engine-core/src/storage/storage_engine_test.rs(1 hunks)d-engine-core/src/test_utils/mock/mock_raft_builder.rs(1 hunks)d-engine-docs/src/docs/overview.md(1 hunks)d-engine-docs/src/docs/server_guide/customize-state-machine.md(3 hunks)d-engine-docs/src/docs/server_guide/customize-storage-engine.md(1 hunks)d-engine-proto/proto/client/client_api.proto(1 hunks)d-engine-proto/src/exts/client_ext.rs(1 hunks)d-engine-server/Cargo.toml(2 hunks)d-engine-server/README.md(1 hunks)d-engine-server/benches/README.md(1 hunks)d-engine-server/benches/state_machine.rs(1 hunks)d-engine-server/benches/ttl.rs(1 hunks)d-engine-server/src/node/builder.rs(4 hunks)d-engine-server/src/node/builder_test.rs(1 hunks)d-engine-server/src/storage/adaptors/file/file_state_machine.rs(25 hunks)d-engine-server/src/storage/adaptors/file/file_state_machine_test.rs(4 hunks)d-engine-server/src/storage/adaptors/rocksdb/rocksdb_engine_test.rs(1 hunks)d-engine-server/src/storage/adaptors/rocksdb/rocksdb_state_machine.rs(15 hunks)d-engine-server/src/storage/lease.rs(1 hunks)d-engine-server/src/storage/lease_integration_test.rs(1 hunks)d-engine-server/src/storage/mod.rs(1 hunks)d-engine-server/src/test_utils/mock/mock_node_builder.rs(1 hunks)d-engine-server/tests/components/replication/replication_handler_test.rs(3 hunks)d-engine/src/lib.rs(1 hunks)examples/client_usage/src/main.rs(1 hunks)examples/rocksdb-cluster/src/main.rs(2 hunks)examples/sled-cluster/Cargo.toml(1 hunks)examples/sled-cluster/src/main.rs(1 hunks)examples/sled-cluster/src/sled_engine_test.rs(2 hunks)examples/sled-cluster/src/sled_state_machine.rs(1 hunks)examples/three-nodes-cluster/src/main.rs(1 hunks)
🧰 Additional context used
🪛 LanguageTool
d-engine-server/benches/README.md
[grammar] ~32-~32: Ensure spelling is correct
Context: ...h varying expired key counts (target: < 1ms for 100 keys) - ttl_registration - ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~37-~37: Ensure spelling is correct
Context: ... Performance with different TTL values (1min, 1hr, 1day) - *worst_case_all_expired...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~37-~37: Ensure spelling is correct
Context: ...rmance with different TTL values (1min, 1hr, 1day) - worst_case_all_expired - A...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~146-~146: Try using a synonym here to strengthen your wording.
Context: ...bench_<what_you_test>4. Include clear comments about what you're measuring 5. Runcar...
(COMMENT_REMARK)
🪛 markdownlint-cli2 (0.18.1)
d-engine-server/benches/README.md
67-67: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
d-engine-server/README.md
18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Lint and Format Check
- GitHub Check: Lint and Format Check
🔇 Additional comments (38)
.github/workflows/dependency-audit.yml (2)
15-16: Good: Explicit Rust toolchain setup improves reproducibility.Adding the dtolnay/rust-toolchain@stable step ensures consistent Rust versions across CI runs and is a best practice for GitHub Actions workflows.
32-32: Good:--lockedflag ensures reproducible dependency installation.Using the
--lockedflag when installing cargo-deny ensures the exact versions specified in Cargo.lock are used, which improves reproducibility and consistency across CI runs.d-engine-server/Cargo.toml (3)
69-69: Benchmark tooling configuration looks good.Criterion is the standard Rust benchmarking framework and the feature selection (html_reports, async_tokio) is appropriate for async workloads.
71-78: Benchmark configurations verified and properly set up.Both benchmark files are present and correctly located:
- ✓
state_machine.rs(8445 bytes)- ✓
ttl.rs(11440 bytes)The Cargo.toml configurations with
harness = falseare correctly set up to use criterion. No issues detected.
38-38: Dashmap 6.1 upgrade is API-compatible; TTL assumption in original comment is unfounded.All existing dashmap usage in the codebase (DashMap, DashSet, and entry API) is compatible with version 6.1. However, dashmap 6.1 does not include built-in TTL or automatic-expiration support, so the bump is not motivated by TTL requirements. Breaking changes between 5.5.3 and 6.1 are internal only and do not affect the public API surface used in this codebase. The upgrade is safe to proceed.
d-engine-core/Cargo.toml (1)
37-37: No issues found—DashMap 6.1 upgrade is safe.DashMap v6.0.0 contains no breaking API changes, only performance optimizations and QoL improvements. v6.1.0 introduces no explicit breaking changes, and MSRV is 1.65. The codebase's Entry API usage (Occupied/Vacant patterns at lines 604-609 in grpc_transport.rs and elsewhere) remains stable and compatible across all 10 DashMap usage locations. While v6.0.0 was yanked due to a critical bug (#304), v6.1 is the stable release with that issue resolved.
.gitignore (1)
49-49: LGTM!Adding
*.bak*to ignore backup files is a sensible addition for keeping the repository clean..github/workflows/ci.yml (1)
69-73: LGTM!The benchmark compilation check is well-placed and ensures that benchmark code stays in sync with the main codebase without the overhead of running full benchmarks on every CI run.
examples/client_usage/src/main.rs (1)
114-114: LGTM!Removing the
.into()conversion simplifies the code. Thepolicyvariable is already of typeReadConsistencyPolicy, so the explicit conversion was unnecessary.CHANGELOG.md (1)
7-40: LGTM!The v0.2.0 release notes are comprehensive and well-structured. Breaking changes are clearly documented with references to the migration guide, and the feature descriptions align with the PR objectives.
d-engine-server/README.md (1)
1-244: LGTM!This is comprehensive and well-structured documentation that clearly explains the server architecture, provides practical examples, and demonstrates the new
start_server()API consistently throughout. The quick-start examples and storage backend comparisons are particularly helpful.Note: The markdownlint warning about the fenced code block on line 18 is a false positive—it's an ASCII art architecture diagram, not a code block requiring a language specifier.
.github/workflows/benchmark.yml (1)
1-89: LGTM!The benchmark workflow is well-configured with appropriate triggers (weekly schedule, manual dispatch, and pushes to performance-critical paths). The separate cache key for benchmarks prevents interference with test caches, and the 30-day artifact retention provides a reasonable window for performance analysis.
d-engine-server/benches/state_machine.rs (3)
203-240: LGTM!The batch benchmarks properly test scaling characteristics across different sizes (10, 100, 1000) and use
BenchmarkId::from_parameterfor clear result labeling. Creating a fresh state machine per iteration is appropriate here to ensure consistent starting conditions for each batch size.
133-199: LGTM!The read benchmarks demonstrate good benchmark design by setting up the state machine once before iterations (lines 137-142, 159-164, 181-190), which isolates the actual read performance being measured. The expired TTL benchmark correctly waits for expiration before measuring passive deletion costs.
26-95: LGTM!The helper functions are well-factored and reusable. The entry creation logic correctly constructs protobuf messages with and without TTL values, and the use of formatting with indices ensures unique keys across benchmark iterations.
d-engine-server/benches/ttl.rs (2)
158-175: Single-threaded runtime may not reflect production workload.The batch TTL registration benchmark uses
new_current_thread()(Line 159), which doesn't reflect production environments where d-engine typically runs with a multi-threaded tokio runtime. This could mask concurrency-related performance characteristics.Consider whether multi-threaded runtime benchmarks are needed for realistic performance profiles, or document that these benchmarks intentionally isolate single-threaded performance.
94-130: Benchmark timing affected by explicit sleep.The 2-second sleep on Line 114 is included in the benchmark measurement, which distorts the actual cleanup performance metrics. Criterion measures the entire closure execution, including the sleep time.
Move the sleep and setup outside the benchmark closure:
fn bench_piggyback_cleanup(c: &mut Criterion) { use d_engine_server::storage::DefaultLease; let mut group = c.benchmark_group("piggyback_cleanup"); // Test with different numbers of expired keys for expired_count in [10, 50, 100, 500].iter() { let lease_config = d_engine_core::config::LeaseConfig { cleanup_strategy: "piggyback".to_string(), ..Default::default() }; - let lease = DefaultLease::new(lease_config); - - // Register keys with very short TTL - for i in 0..*expired_count { - let key = format!("key_ttl_{}", i); - lease.register(bytes::Bytes::from(key), 1); // 1 second TTL - } - - // Wait for expiration - std::thread::sleep(Duration::from_secs(2)); group.bench_with_input( BenchmarkId::from_parameter(expired_count), expired_count, |b, _| { + // Setup happens per iteration to ensure consistent state + let lease = DefaultLease::new(lease_config.clone()); + for i in 0..*expired_count { + let key = format!("key_ttl_{}", i); + lease.register(bytes::Bytes::from(key), 1); + } + std::thread::sleep(Duration::from_secs(2)); + b.iter(|| { // Directly measure cleanup logic without apply_chunk overhead let expired_keys = lease.on_apply(); black_box(expired_keys); }); }, ); } group.finish(); }Likely an incorrect or invalid review comment.
Makefile (3)
163-176: LGTM! Well-structured separation of excluded projects.The new
clippy-excludedtarget properly isolates linting for examples and benchmarks, preventing them from slowing down the main workspace clippy checks. The loop structure is clear and provides good error reporting.
315-321: Excellent addition of bench-compile target.This target allows CI to verify benchmark compilation without the time cost of running them. The error handling and messaging are clear and appropriate.
155-160: No action required—workspace-level Clippy scope is correctly configured.Verification confirms that the workspace contains no binary targets in any member crate, and example targets are intentionally located outside the workspace (in
examples/subdirectories) and are already covered by theclippy-excludedtarget with--all-targets. The--lib --testsscope at the workspace level is appropriate and sufficient.d-engine-docs/src/docs/server_guide/customize-storage-engine.md (1)
167-168: LGTM! Documentation properly reflects the new startup API.The update from
build()tostart_server().await?correctly documents the unified asynchronous startup flow introduced in this PR.d-engine-proto/src/exts/client_ext.rs (1)
46-85: LGTM! Clean TTL support integration.The changes properly extend the
WriteCommandAPI with TTL support:
insert()explicitly setsttl_secs: Nonefor backward compatibilityinsert_with_ttl()provides an ergonomic constructor for TTL-enabled inserts- Documentation is clear and follows existing patterns
d-engine-proto/proto/client/client_api.proto (1)
11-13: LGTM! Backward-compatible TTL field addition.The
optional uint64 ttl_secsfield is properly added with clear documentation. Using field number 3 maintains sequential ordering, and the optional nature ensures backward compatibility with existing clients.examples/sled-cluster/Cargo.toml (1)
8-8: LGTM! Required dependency for proto updates.Adding the
d-engine-protodependency allows the sled-cluster example to access the updatedInserttype with TTL support.d-engine-server/src/storage/adaptors/rocksdb/rocksdb_engine_test.rs (1)
105-109: LGTM! Test updated for proto changes.The Insert construction correctly includes
ttl_secs: None, maintaining existing test behavior while adapting to the updated proto definition.README.md (1)
63-64: LGTM! Example reflects the new startup API.The quick start example correctly demonstrates the updated
start_server()flow, replacing the previousbuild()pattern. This aligns with the unified asynchronous startup introduced in the PR.d-engine-core/src/test_utils/mock/mock_raft_builder.rs (1)
360-362: LGTM! Lease injection mock support added correctly.The mock expectation for
try_inject_leaseis properly configured and aligns with the broader lease/TTL framework being introduced.d-engine-server/src/test_utils/mock/mock_node_builder.rs (1)
525-525: LGTM! Post-start initialization mock expectation added correctly.The mock properly supports the new
post_start_initlifecycle hook.d-engine-core/src/storage/storage_engine_test.rs (1)
288-292: LGTM! Test payload updated for TTL field.The
ttl_secs: Noneaddition correctly aligns with the proto changes introducing optional TTL support.d-engine-docs/src/docs/overview.md (1)
50-52: LGTM! Documentation updated for unified startup API.The example correctly demonstrates the new
start_server()method, simplifying the startup sequence.d-engine-core/src/storage/mod.rs (2)
1-1: LGTM! Lease module added to storage layer.The new lease module declaration follows the existing module organization pattern.
7-7: LGTM! Lease types exposed through public API.The re-export makes lease-related functionality available to consumers of the storage module.
d-engine-server/src/storage/adaptors/file/file_state_machine_test.rs (4)
29-29: LGTM! Test data updated with TTL field.The Insert payload correctly includes
ttl_secs: Noneto align with proto changes.
46-46: LGTM! Consistent TTL field usage.
77-77: LGTM! Clear documentation of TTL intent.
102-130: LGTM! Performance test infrastructure added.The test properly:
- Uses
#[ignore]to prevent automatic execution- Implements
StateMachineBuilderfor test harness integration- Leverages tempfile for isolation
- Calls the shared performance test suite
d-engine/src/lib.rs (1)
69-70: LGTM! Documentation reflects simplified startup API.The example correctly demonstrates the new
start_server()method, making the embedded server setup more straightforward.d-engine-core/src/errors.rs (1)
341-343: LGTM! New error variant for lease support.The
LeaseNotSupportederror is well-documented and provides a clear message for lease-related failures.
- Use inline format arguments in ttl benchmark
…drop - Call stop() to persist TTL metadata before state machine drop - Consistent with FileStateMachine test pattern
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
… fixes BREAKING CHANGES: - WAL format changed from relative ttl_secs (u32) to absolute expire_at_secs (u64) - Enables crash-safe TTL semantics compatible with etcd lease behavior - TTL now persists correctly across restarts (no TTL reset) - See MIGRATION_GUIDE.md for upgrade instructions Features: - etcd-compatible TTL semantics with absolute expiration time - Crash-safe WAL replay skips expired entries - Snapshot restore now persists TTL metadata to RocksDB CF - WAL stores absolute expiration time for proper recovery Fixes: - Fix clippy warning: empty line after doc comments in rocksdb_state_machine.rs - Fix duplicate trace logging in BufferedRaftLog initialization - Fix log level filtering: RUST_LOG now limits to DEBUG (no TRACE spam) - Fix crash-safety bug in snapshot restore (TTL metadata persistence) - Remove high-frequency trace logs from hot paths Performance: - Optimize TTL benchmarks: 10x faster execution (213s → 20s) - Use iter_batched to separate setup from measurement - Reduce sample size to 10 for tests with sleep operations - worst_case_all_expired: 10.6x faster - mixed_ttl_workload: 10x faster - piggyback_high_frequency: 10x faster Internal: - Refactor RocksDB options into configure_db_options() helper (DRY) - Update RUST_LOG_LEVEL format for proper module-level filtering - Add comprehensive WAL migration guide Docs: - Update CHANGELOG.md with breaking changes and migration notes - Add WAL format migration section to MIGRATION_GUIDE.md - Document TTL behavior changes and upgrade paths
- Remove trace! macro calls from BufferedRaftLog (command processor, batch processor, drop) - Remove trace! macro calls from FileStateMachine (apply_chunk, lease cleanup) - Remove unused 'use tracing::trace' imports - Tests now show only DEBUG and higher log levels - Significantly improves test output readability This completes the log level filtering fix started in previous commit.
6df3853 to
0b67527
Compare
Add three integration tests to verify WAL replay behavior: - test_wal_replay_handles_incomplete_entries: Verifies incomplete WAL entries (missing expire_at field) are treated as permanent keys, not crashed - test_wal_replay_all_expired_empty_state: Verifies all-expired WAL entries result in empty state after replay (expired keys are skipped) - test_wal_replay_mixed_expired_and_valid: Verifies mixed expired/valid/permanent entries are correctly filtered during replay These tests ensure crash-safe TTL semantics per MIGRATION_GUIDE.md requirements.
Add 16 unit tests covering core Raft leader election protocol: - Vote request handling with valid conditions (granted/denied) - Term advancement and vote reset behavior - Log recency checks (term precedence, index comparison) - Already-voted scenarios (same/different candidates) - Empty log edge cases - Protocol compliance (term 0, large term numbers) - Legal vote request validation Tests use MockRaftLog for RaftLog dependency to keep tests focused and fast. No #[traced_test] used - following production best practices. All 16 tests passing.
Add 15 unit tests for ElectionTimer and ReplicationTimer: - ElectionTimer initialization and randomness verification - Timeout bounds checking and distribution analysis - Reset behavior and expiration detection - ReplicationTimer dual-timeout coordination - Independent reset verification (batch vs replication) - Deadline calculation and minimum selection Tests verify: - Random timeouts fall within [min, max] range - Distribution is not clustered at range edges - Reset updates deadlines appropriately - Expiration detection works correctly - Timer independence when resetting separately All 15 tests passing.
…te file - Moved tests to dedicated lease_unit_test.rs file (best practice) - Tests cover 22 code paths including: - Basic registration, unregister, expiration checks - Key updates and multi-key scenarios - Snapshot serialization/deserialization - Error handling (invalid data) - Piggyback cleanup with different configurations - Edge cases (empty leases, non-existent keys, etc) - may_have_expired_keys() optimization paths - on_apply() frequency-based cleanup - All 22 tests passing - Improves coverage of DefaultLease methods
Coverage Improvement Results: - Before: 77.96% line coverage (Codecov diff hit) - After: 85.84% line coverage (24,432/28,461 lines) - Target: 85.61% ✅ EXCEEDED Tests Added: - 22 unit tests for DefaultLease in lease_unit_test.rs - Fixed 6 Clippy warnings in timer_test.rs - 1,006 total tests passing (0 failed) Test Coverage by Module: - storage::lease_unit_test: 22 new tests - storage::lease_integration_test: 16 tests - d-engine-core timer tests: 15 tests - d-engine-core election tests: 16 tests - d-engine-server integration: 100+ tests - buffered raft log: 600+ tests Key Improvements: - DefaultLease::register() - key update scenarios ✓ - DefaultLease::unregister() - cleanup logic ✓ - DefaultLease::is_expired() - O(1) checks ✓ - DefaultLease::get_expired_keys() - range cleanup ✓ - DefaultLease::on_apply() - piggyback frequency ✓ - DefaultLease::may_have_expired_keys() - optimization ✓ - DefaultLease::reload() - error handling ✓ Verification: - All tests pass with cargo test - Coverage verified with cargo llvm-cov nextest - lcov.info generated and ready for Codecov upload
…ypes Update ALLOWED_TYPES in commit-message-check.yml to match cliff.toml: - Add: style, test, ci, revert - These types are defined in cliff.toml but were missing from workflow validation - Ensures consistency between changelog generation and CI validation
… engine ## 🎯 Overview v0.2.0 is a major release that transforms d-engine from a Raft implementation into a **production-ready distributed coordination engine** with workspace structure, developer-friendly APIs, and comprehensive features. --- ## 🚀 Key Features ### 🏗️ **Workspace Structure (#167)** - Modular design: `d-engine-core`, `d-engine-server`, `d-engine-client`, `d-engine-proto` - Clean separation of concerns for library users - Profile-optimized build configuration ### ⏱️ **TTL & Lease Support (#172)** - Crash-safe TTL with absolute expiration timestamps - WAL format change: relative `ttl_secs` → absolute `expire_at_secs` - Piggyback cleanup mechanism (minimal overhead) - **Breaking Change**: See MIGRATION_GUIDE.md for WAL upgrade ### 👁️ **Watch API (#174, #196)** - Lock-free event notification with crossbeam-channel - gRPC streaming support for real-time key monitoring - <10ns apply-path overhead, <100μs end-to-end latency - Service discovery examples included ### 🚀 **EmbeddedEngine API (#182)** - Zero-config single-node quick start - `wait_leader()` and `leader_notifier()` for event-driven apps - Dynamic cluster expansion (1→3 nodes without downtime) - In-process `LocalKvClient` with explicit consistency levels ### 🎯 **Single-Node Support (#179)** - Configuration-based single-node detection - Automatic election/replication optimization - Production-ready for low-traffic scenarios ### 📖 **Read Consistency Policies (#142)** - LinearizableRead (strong consistency) - LeaseRead (optimized with leader lease) - EventualConsistency (fast local reads) ### 🔧 **Go Client Support (#170, #219)** - Pre-generated Go protobuf code (zero-config) - Comprehensive error handling guide - Service discovery pattern examples --- ## 🐛 Critical Fixes - **#212**: Fix learner promotion stuck (voter count + role transition) - **#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) - **#211**: Remove Arc::get_mut anti-pattern from lease injection - **#145**: Fix undefined behavior in mmap zero-copy path - **#197**: Fix integration test failures (Arc ownership, timing issues) --- ## ⚡ Performance Improvements - **#138**: Long-lived peer tasks for append entries (100K+ throughput target) - **#140**: Optimize proto bytes fields (use `bytes::Bytes`) - **#141**: Optimize RocksDB write path for lower latency - **#143**: Refactor gRPC compression configuration - **#194**: Skip protobuf decoding when no active watchers - **#208**: Eliminate redundant async calls in leader write hot path (+2-3% throughput) - **#223**: Optimize check_learner_progress() lock contention ### Benchmark Results - **LeaseRead**: 99,418 ops/s (+7.8% vs v0.1.4) - **EventualConsistency**: 126,095 ops/s (+9.2% vs v0.1.4) - **Linearizable p99**: 23.01ms (-8% vs v0.1.4) --- ## 🔧 Refactoring & Architecture - **#210**: Simplify watch architecture (tokio::broadcast, 90% code sharing) - **#217**: Refactor Node::run() with strategy pattern - **#209**: Consolidate Raft unit tests (27 tests migrated from server to core) - **#201**: Separate static membership from dynamic leader state - **#223**: Extract 5 helper methods in learner promotion (SRP + 11 unit tests) --- ## 📚 Documentation - Restructure quick-start docs (embedded + standalone examples) - Add integration-modes.md and use-cases.md - Comprehensive error handling guide - Service discovery pattern documentation - Delete 911 lines of internal architecture docs (20/80 principle) --- ## 🧪 Testing - **430** core tests + **305** server tests + **292** integration tests passing - Fix flaky tests and timing issues - Add 14+ new unit tests across components - Optimize test suite with nextest --- ##⚠️ Breaking Changes 1. **WAL Format**: Relative `ttl_secs` → absolute `expire_at_secs` (requires data migration) 2. **Config**: `raft.watch.enabled` removed (Watch always available) 3. **API**: `StateMachine::start()` changed to async 4. **NodeStatus**: Refactored enum (PROMOTABLE/READ_ONLY/ACTIVE) See **MIGRATION_GUIDE.md** for detailed upgrade instructions. --- ## 📦 Dependency Updates - tokio-stream: Fix net feature requirement - astral-tokio-tar: Replace tokio-tar (CVE-2025-62518 fix) - simd-adler32: 0.3.7 → 0.3.8 - rustls-pemfile: unmaintained warning suppressed (tonic 0.12 dep) --- ## 🎯 Closes #43, #45, #59, #66, #70, #71, #79, #89, #90, #101, #102, #106, #107, #109, #110, #119, #120, #121, #122, #123, #125, #133, #135, #138, #139, #140, #141, #142, #143, #145, #146, #147, #148, #150, #151, #152, #153, #154, #155, #156, #157, #158, #159, #161, #164, #167, #170, #172, #174, #175, #176, #178, #185, #186, #187, #192, #193, #194, #195, #196, #197, #200, #201, #203, #204, #205, #208, #209, #210, #211, #212, #213, #217, #218, #219, #222, #223, #224 --- **Files Changed:** 100+ files, ~5,000 insertions, ~1,500 deletions
Type
Description
Add TTL Support
Related Issues
Checklist
Summary by CodeRabbit
Release Notes
New Features
start_server()API for simplified server initialization.Documentation
Improvements