feat(conf): support configuring DB storage directory#243
Open
AlexStocks wants to merge 6 commits intofeat/raftfrom
Open
feat(conf): support configuring DB storage directory#243AlexStocks wants to merge 6 commits intofeat/raftfrom
AlexStocks wants to merge 6 commits intofeat/raftfrom
Conversation
Add `db-dir` configuration option to specify the RocksDB data directory. Previously the path was hardcoded as `"./db"` in 4 places across the codebase. Changes: - Add `db_dir` field to Config struct (default: "./db") - Parse `db-dir` key in Config::load() - Pass db_dir through to initialize_storage_server() and legacy server constructors (TcpServer, ClusterTcpServer, UnixServer) - Add `db-dir` documentation to kiwi.conf - Add unit tests for default value and config file parsing Usage in kiwi.conf: db-dir /data/kiwi/db Closes #193
|
Warning Rate limit exceeded
⌛ 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)
📝 WalkthroughWalkthroughAdds a configurable Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config (file)
participant Main as Main
participant Factory as ServerFactory
participant Server as Tcp/Unix/Cluster Server
participant Storage as RocksDB Storage
Config->>Main: load() -> config.db_dir
Main->>Factory: create_* (protocol, addr, db_dir)
Factory->>Server: new(addr, db_dir)
Server->>Storage: open(db_path derived from db_dir)
Storage-->>Server: opened
Server-->>Factory: ready
Factory-->>Main: server instance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/src/main.rs (1)
133-150:⚠️ Potential issue | 🟠 MajorWait for storage init success before opening the network server.
initialize_storage_servernow depends on user-supplieddb_dir, but this task is fire-and-forget: failures are only logged inside the spawned task, and the100mssleep is not a readiness check. A bad path or slow open can leave the process accepting connections before the storage backend exists.💡 Suggested fix
+ let (storage_ready_tx, storage_ready_rx) = tokio::sync::oneshot::channel(); let db_dir = config.db_dir.clone(); storage_handle.spawn(async move { info!("Initializing storage server..."); - match initialize_storage_server(storage_receiver, &db_dir).await { + let init_result = initialize_storage_server(storage_receiver, &db_dir).await; + let _ = storage_ready_tx.send( + init_result + .as_ref() + .map(|_| ()) + .map_err(|e| e.to_string()), + ); + match init_result { Ok(_) => { error!("Storage server exited unexpectedly - this should never happen!"); } Err(e) => { error!("Storage server failed: {}", e); } } }); - // Give storage server a moment to initialize - std::thread::sleep(std::time::Duration::from_millis(100)); + network_handle.block_on(async { + storage_ready_rx + .await + .map_err(|_| std::io::Error::other("storage init task exited before signaling readiness"))? + .map_err(std::io::Error::other) + })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/src/main.rs` around lines 133 - 150, The storage task is currently fire-and-forget (spawned with storage_handle.spawn) and only sleeps 100ms for readiness; update the code so the server waits for storage initialization success before opening the network: either call initialize_storage_server(&storage_receiver, &db_dir).await synchronously before starting the network or spawn initialize_storage_server but use a oneshot/mpsc readiness signal (send from inside initialize_storage_server on success/failure) and await that signal here; store the JoinHandle returned by storage_handle.spawn (or return/propagate initialization error) and fail fast if initialization fails so the network server never starts when initialize_storage_server or the provided db_dir are invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/conf/kiwi.conf`:
- Around line 193-195: The comment about RocksDB subdirectories overstates
current behavior: storage initializers use Storage::new(1, 0) so only <db-dir>/0
is created; either propagate the db_instance_num into the storage initialization
where Storage::new(...) is called (so multiple subdirs like ./db/0, ./db/1 will
be created) or simplify the comment to say the server currently creates a single
per-database directory (e.g., ./db/0) until db_instance_num is wired through;
update the comment to match whichever approach you take and reference the
Storage::new(1, 0) usage and the db_instance_num configuration symbol when
making the change.
In `@src/conf/src/config.rs`:
- Line 229: Validate and reject empty or whitespace-only values for the db_dir
configuration field before they are stored or converted to a PathBuf: add a
check (e.g., in the config parsing/validation function that constructs or
validates the struct containing pub db_dir: String) that trims the input and
returns an error if the result is empty, mirroring the existing handling for
cluster-data-dir; also apply the same validation where db_dir is read/assigned
(see the code around the db_dir usage at the other occurrence referenced) so
that whitespace-only or String::new() never flow into PathBuf::from(...) or
RocksDB initialization.
In `@src/conf/src/lib.rs`:
- Around line 117-129: The test test_db_dir_from_config_file uses a fixed path
"/tmp/kiwi_test_db_dir.conf" which races in parallel CI and fails on non-Unix
runners; change the test to create a unique temporary file (e.g., using
tempfile::NamedTempFile or constructing a path under std::env::temp_dir() with a
random suffix), write the two config lines to that temp file, pass its path to
Config::load, and rely on the tempfile to be cleaned up (remove manual
remove_file). Update references in the test function
test_db_dir_from_config_file to use the temporary file handle/path instead of
the hardcoded string.
In `@src/net/src/unix.rs`:
- Around line 36-41: Change unix::Server::new (the pub fn new in
src/net/src/unix.rs) to return Result<Self, E> instead of panicking: validate
db_dir and propagate any storage open errors rather than calling unwrap. Replace
the line creating Storage with using Storage::new(...) and call
storage.open(storage_options, db_path) and map or propagate its error into an
Err variant; on success construct and return Ok(Self { ... }). Follow the
TcpServer::new pattern for error type and propagation so
permission/invalid-path/RocksDB lock errors are returned to the caller.
---
Outside diff comments:
In `@src/server/src/main.rs`:
- Around line 133-150: The storage task is currently fire-and-forget (spawned
with storage_handle.spawn) and only sleeps 100ms for readiness; update the code
so the server waits for storage initialization success before opening the
network: either call initialize_storage_server(&storage_receiver, &db_dir).await
synchronously before starting the network or spawn initialize_storage_server but
use a oneshot/mpsc readiness signal (send from inside initialize_storage_server
on success/failure) and await that signal here; store the JoinHandle returned by
storage_handle.spawn (or return/propagate initialization error) and fail fast if
initialization fails so the network server never starts when
initialize_storage_server or the provided db_dir are invalid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e00dea7-0ec4-45ed-98f2-af25fa767216
📒 Files selected for processing (7)
src/conf/kiwi.confsrc/conf/src/config.rssrc/conf/src/lib.rssrc/net/src/lib.rssrc/net/src/tcp.rssrc/net/src/unix.rssrc/server/src/main.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new db-dir configuration option so the RocksDB data directory is configurable instead of being hardcoded, aligning storage initialization with user-provided config and documenting the new setting.
Changes:
- Introduce
db_dironConfigwith default"./db"and parsedb-dirfrom config files. - Plumb
db_dirinto storage initialization and legacy server constructors (TCP/cluster TCP/Unix). - Document
db-dirinkiwi.confand add unit tests for default + parsing.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/src/main.rs | Passes configured db_dir into storage runtime initialization. |
| src/net/src/unix.rs | Adds db_dir parameter to UnixServer and uses it to pick the RocksDB path. |
| src/net/src/tcp.rs | Adds db_dir parameter to TCP/cluster TCP legacy servers and uses it for RocksDB path. |
| src/net/src/lib.rs | Updates factory methods to pass db_dir into legacy server constructors. |
| src/conf/src/config.rs | Adds db_dir to Config, sets default, and parses db-dir. |
| src/conf/src/lib.rs | Updates existing tests and adds tests for db_dir default/config parsing. |
| src/conf/kiwi.conf | Documents the new db-dir option and its semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
38
to
42
| let storage_options = Arc::new(StorageOptions::default()); | ||
| let db_path = PathBuf::from("./db"); | ||
| let db_path = PathBuf::from(db_dir.unwrap_or("./db")); | ||
| let mut storage = Storage::new(1, 0); | ||
| storage.open(storage_options, db_path).unwrap(); | ||
| let executor = Arc::new(CmdExecutorBuilder::new().build()); |
Contributor
Author
There was a problem hiding this comment.
Fixed in the same change — new() returns Result and all callers in ServerFactory use .ok().
Comment on lines
101
to
103
| #[cfg(unix)] | ||
| "unix" => Some(Box::new(unix::UnixServer::new(addr))), | ||
| "unix" => Some(Box::new(unix::UnixServer::new(addr, None))), | ||
| #[cfg(not(unix))] |
Contributor
Author
There was a problem hiding this comment.
Good catch. The create_server_with_mode path uses the dual-runtime NetworkServer for TCP (storage is initialized separately in initialize_storage_server). The Unix fallback here is a legacy path — I've left it as None (uses default ./db) since the dual-runtime architecture doesn't use this code path. Added a comment to document this.
src/conf/src/lib.rs
Outdated
Comment on lines
+120
to
+130
| let config_path = "/tmp/kiwi_test_db_dir.conf"; | ||
| let mut f = std::fs::File::create(config_path).unwrap(); | ||
| writeln!(f, "port 7379").unwrap(); | ||
| writeln!(f, "db-dir /data/kiwi/db").unwrap(); | ||
| drop(f); | ||
|
|
||
| let config = Config::load(config_path).unwrap(); | ||
| assert_eq!("/data/kiwi/db", config.db_dir); | ||
|
|
||
| let _ = std::fs::remove_file(config_path); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed: using std::env::temp_dir().join(format\!("kiwi_test_db_dir_{}.conf", std::process::id())) now.
The test_db_dir_from_config_file test used a hardcoded /tmp/ path which does not exist on Windows. Use std::env::temp_dir() instead.
- Validate db-dir is not empty/whitespace in Config::load() - UnixServer::new returns Result instead of panicking via unwrap() - Update ServerFactory callers to handle UnixServer::new Result - Use PID in temp config filename to avoid parallel test collisions - Fix kiwi.conf doc: clarify subdirectories depend on db-instance-num
Fix cargo fmt violation in unix.rs, add clippy::unwrap_used check as a non-blocking CI job to catch unwrap usage in new code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore: enforce clippy::unwrap_used lint and remove all unwrap() from production code
- Replace all .unwrap() in production code with .expect("descriptive reason")
- Add #[allow(clippy::unwrap_used)] to all #[cfg(test)] modules
- Add #![allow(clippy::unwrap_used)] to standalone test files and benchmarks
- Add -D clippy::unwrap_used to Makefile lint target
This ensures panics from unwrap() are always accompanied by a meaningful
message, making debugging easier. Test code is exempted as unwrap() in
tests provides clear enough failure context via the test framework.
Closes #229
* fix: address cargo fmt and CodeRabbit review feedback
- Fix rustfmt line-length violations in unix.rs, segment_log.rs, redis_sets.rs
- proc-macro: use syn::Error::to_compile_error() instead of expect() for
better diagnostics in macro_stack_trace_debug.rs
- Improve expect() messages: monitoring_api.rs, custom_comparator.rs
- Remove misplaced #[allow(clippy::unwrap_used)] on use statement in writer.rs
* ci: fix build error and improve CI workflows
Fix:
- Restore OperationType import in writer.rs test module (was broken
when removing misplaced #[allow] on use statement)
CI improvements (inspired by QuantClaw):
- cargo fmt: run on ubuntu only (format output is platform-independent),
saves 2 redundant macOS/Windows jobs
- Add Cargo registry cache to clippy and build-and-test jobs
- Add fail-fast: false to matrix strategies for better error visibility
- Add permissions: contents: read for least-privilege principle
- Add shared env vars (CARGO_TERM_COLOR, CARGO_INCREMENTAL, etc.)
- Add upload-artifact on test failure for debugging
- Move integration tests (Python) to a dedicated job after build-and-test
- Remove redundant test.yml (fully covered by ci.yml)
New workflows:
- clean-cache.yml: auto-delete PR branch caches when PR is closed
- security.yml: cargo audit (dependency vulnerabilities) + cargo deny
(advisories & licenses), runs on push/PR to main + weekly schedule
* ci: make security audit jobs non-blocking
cargo-audit and cargo-deny report pre-existing dependency issues
(5 vulnerabilities, missing deny.toml config). Mark both jobs as
continue-on-error so they surface findings without blocking PRs.
* ci: remove security audit from PR triggers
Security audit checks pre-existing dependency vulnerabilities unrelated
to PR changes. Running them on PRs produces red check marks that confuse
contributors. Keep them on push-to-main and weekly schedule only.
PR #242 already enforces clippy::unwrap_used in the lint target, so the separate unwrap-check CI job and lint-unwrap Makefile target are no longer needed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
|
如果代码没有冲突的话，可以直接合入 @AlexStocks |
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
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.
Summary
Add
db-dirconfiguration option to specify the RocksDB data directory. Previously the path was hardcoded as"./db"in 4 places across the codebase.Changes
db_dirfield toConfigstruct (default:"./db")db-dirkey inConfig::load()db_dirthrough toinitialize_storage_server()and legacy server constructors (TcpServer,ClusterTcpServer,UnixServer)db-dirdocumentation tokiwi.confUsage
Each database instance creates a subdirectory under this path (e.g.,
/data/kiwi/db/0,/data/kiwi/db/1).Test plan
"./db"when not configureddb-dirvalue"./db")Closes #193
Summary by CodeRabbit
New Features
Tests