setup the key-value db (first iteration)#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a first working iteration of an LSM-style key-value store in Rust, including an engine (memtable + WAL + SSTable flushing), a TCP server using a simple framed bincode protocol, and a unified CLI for running the server and issuing requests.
Changes:
- Implemented core engine pieces: thread-safe memtable, WAL append + recovery, SSTable flush + read path.
- Added a multi-threaded TCP server and a clap-based CLI client using a custom length-prefixed bincode protocol.
- Cleaned up repository hygiene by ignoring build output/data and removing committed build artifacts.
Reviewed changes
Copilot reviewed 40 out of 69 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main.rs | Wire server binary to call start_server with default addr/path. |
| server/src/lib.rs | Implement TCP listener, per-connection handler, request/response framing + bincode serialization. |
| server/Cargo.toml | Add serialization dependencies for server crate. |
| engine/src/db.rs | Implement Database with WAL, memtable, SSTable list, flush threshold, and read path. |
| engine/src/memtable/mod.rs | Implement thread-safe BTreeMap-backed memtable with tombstones and size tracking. |
| engine/src/wal/mod.rs | Implement WAL manager: append (length-prefixed bincode) and recovery into memtable. |
| engine/src/sstable/mod.rs | Implement basic SSTable writer/reader with length-prefixed bincode records. |
| engine/src/lib.rs | Update exposed engine modules (remove compaction/utils exports). |
| engine/Cargo.toml | Add dependencies for WAL/SSTable serialization + locking (and a skiplist dep). |
| engine/src/wal/writer.rs | Remove placeholder WAL writer module. |
| engine/src/utils.rs | Remove placeholder utils module. |
| engine/src/sstable/reader.rs | Remove placeholder SSTable reader module (replaced by sstable/mod.rs). |
| engine/src/sstable/index.rs | Remove placeholder index module. |
| engine/src/sstable/block.rs | Remove placeholder block module. |
| engine/src/memtable/skiplist.rs | Remove placeholder skiplist module. |
| engine/src/compaction/mod.rs | Remove placeholder compaction module. |
| engine/src/compaction/worker.rs | Remove placeholder compaction worker. |
| common/src/types.rs | Define shared protocol/engine types (Operation, Record, Request, Response) with serde derives. |
| common/Cargo.toml | Add serde + bincode dependencies for shared types. |
| cli/src/main.rs | Implement unified CLI (start-server/set/get/delete) and client protocol framing. |
| cli/Cargo.toml | Add clap + server dependency and bincode for client protocol. |
| README.md | Update docs to reflect current architecture, CLI usage, and protocol. |
| Cargo.lock | Lock new dependencies (serde/bincode/clap/parking_lot/etc.). |
| .gitignore | Ignore target/ build outputs and local data/ directory. |
| target/debug/deps/server-b2e516feb17b9506.d | Remove committed build artifact. |
| target/debug/deps/engine-970377fa98761e24.d | Remove committed build artifact. |
| target/debug/deps/common-d52c5250bbb2aa31.d | Remove committed build artifact. |
| target/debug/deps/cli-9792b054fead65f0.d | Remove committed build artifact. |
| target/debug/.fingerprint/server-b2e516feb17b9506/invoked.timestamp | Remove committed build artifact. |
| target/debug/.fingerprint/server-b2e516feb17b9506/bin-server.json | Remove committed build artifact. |
| target/debug/.fingerprint/server-b2e516feb17b9506/bin-server | Remove committed build artifact. |
| target/debug/.fingerprint/engine-970377fa98761e24/lib-engine.json | Remove committed build artifact. |
| target/debug/.fingerprint/engine-970377fa98761e24/lib-engine | Remove committed build artifact. |
| target/debug/.fingerprint/engine-970377fa98761e24/invoked.timestamp | Remove committed build artifact. |
| target/debug/.fingerprint/common-d52c5250bbb2aa31/lib-common.json | Remove committed build artifact. |
| target/debug/.fingerprint/common-d52c5250bbb2aa31/lib-common | Remove committed build artifact. |
| target/debug/.fingerprint/common-d52c5250bbb2aa31/invoked.timestamp | Remove committed build artifact. |
| target/debug/.fingerprint/cli-9792b054fead65f0/invoked.timestamp | Remove committed build artifact. |
| target/debug/.fingerprint/cli-9792b054fead65f0/bin-cli.json | Remove committed build artifact. |
| target/debug/.fingerprint/cli-9792b054fead65f0/bin-cli | Remove committed build artifact. |
| target/CACHEDIR.TAG | Remove committed build artifact. |
| target/.rustc_info.json | Remove committed build artifact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut len_bytes = [0u8; 4]; | ||
| if reader.read_exact(&mut len_bytes).is_err() { | ||
| break; | ||
| } | ||
| let len = u32::from_le_bytes(len_bytes) as usize; | ||
| let mut bytes = vec![0u8; len]; | ||
| reader.read_exact(&mut bytes)?; | ||
|
|
There was a problem hiding this comment.
SSTable reads also trust the length prefix and allocate based on it. A corrupted SSTable can trigger very large allocations/OOM. Add a max record size bound and fail fast on invalid/corrupt lengths rather than allocating untrusted sizes.
| for stream in listener.incoming() { | ||
| let stream = stream?; | ||
| let db = Arc::clone(&db); | ||
| std::thread::spawn(move || { | ||
| if let Err(e) = handle_client(stream, db) { | ||
| eprintln!("Error handling client: {}", e); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Spawning an unbounded OS thread per incoming connection can exhaust system resources under load (or intentional connection floods). Consider using a bounded thread pool, a connection limit, or an async runtime so concurrency is controlled.
| serde = { version = "1.0", features = ["derive"] } | ||
| bincode = "1.3" | ||
| parking_lot = "0.12" | ||
| crossbeam-skiplist = "0.1" # In case we want to try skiplist later, but BTreeMap is fine for now. |
There was a problem hiding this comment.
crossbeam-skiplist is added as a dependency but isn't used anywhere in the crate. Keeping unused dependencies increases build time and attack surface; consider removing it until there's an actual implementation that needs it.
| crossbeam-skiplist = "0.1" # In case we want to try skiplist later, but BTreeMap is fine for now. |
| if let Some(old) = map.insert(key, None) { | ||
| if let Some(old_val) = old { | ||
| *size = *size + 1 - old_val.len(); // None is effectively 1 byte or so in terms of overhead | ||
| } |
There was a problem hiding this comment.
*size = *size + 1 - old_val.len() can underflow when old_val.len() > *size + 1, leading to wraparound (release) or panic (debug). Use saturating math here (similar to set) and consider making the tombstone overhead adjustment symmetric with set (replacing None with Some should subtract the previous tombstone overhead).
| for _ in 0..count { | ||
| let mut len_bytes = [0u8; 4]; | ||
| if reader.read_exact(&mut len_bytes).is_err() { | ||
| break; | ||
| } | ||
| let len = u32::from_le_bytes(len_bytes) as usize; | ||
| let mut bytes = vec![0u8; len]; | ||
| reader.read_exact(&mut bytes)?; | ||
|
|
||
| let record: Record = bincode::deserialize(&bytes)?; | ||
| if record.key == key { | ||
| return Ok(Some(record.value)); | ||
| } | ||
| } |
There was a problem hiding this comment.
SstableReader::get linearly scans and deserializes records until it finds the key. This makes each get O(n) in the SSTable size and will degrade quickly as files grow. Consider at least a sparse index/binary search over sorted keys, or reading blocks with an index, to keep point lookups logarithmic/near-constant.
| self.wal.lock().append(&op)?; | ||
| self.memtable.set(key, value); | ||
|
|
||
| if self.memtable.size() >= FLUSH_THRESHOLD { | ||
| self.flush()?; |
There was a problem hiding this comment.
set can run concurrently with flush(), but flush() snapshots memtable.entries() and later clears the memtable and rotates the WAL. A write that happens between the snapshot and clear() can be dropped from the memtable and end up only in the rotated wal.*.old file (which is not recovered on restart), causing permanent data loss. Serialize writes/flush (e.g., a DB-level mutex or memtable-swap approach) so no new ops can land in the flushed memtable/WAL segment during rotation.
| self.wal.lock().append(&op)?; | ||
| self.memtable.delete(key); | ||
|
|
||
| if self.memtable.size() >= FLUSH_THRESHOLD { | ||
| self.flush()?; | ||
| } |
There was a problem hiding this comment.
Same concurrency issue as set: delete can race with flush() (snapshot -> WAL rotate -> memtable clear), potentially losing deletes/updates and leaving them only in the rotated WAL segment. Ensure delete participates in the same synchronization strategy as set/flush so WAL rotation + memtable clear are atomic with respect to new operations.
| if let Some(old_val) = old { | ||
| *size = (*size + val_len).saturating_sub(old_val.len()); | ||
| } else { | ||
| *size += val_len; |
There was a problem hiding this comment.
When replacing an existing tombstone (old == None) with a value, *size += val_len doesn't account for the tombstone overhead previously included in size (the + 1 used in delete). Over time this will drift and can cause premature or delayed flushes; adjust size accounting when transitioning None -> Some (and vice versa) so it stays consistent.
| *size += val_len; | |
| *size = (*size + val_len).saturating_sub(1); |
| let len = bytes.len() as u32; | ||
| self.writer.write_all(&len.to_le_bytes())?; | ||
| self.writer.write_all(&bytes)?; | ||
| self.writer.flush()?; |
There was a problem hiding this comment.
flush() on BufWriter does not guarantee durability on crash/power loss; it only pushes data to the OS. Given the WAL is used for durability/recovery, consider calling sync_data()/sync_all() on the underlying File after flushing (or at least provide a configurable durability mode) so acknowledged writes are actually persisted.
| self.writer.flush()?; | |
| self.writer.flush()?; | |
| self.writer.get_ref().sync_data()?; |
| let len = u32::from_le_bytes(len_bytes) as usize; | ||
| let mut bytes = vec![0u8; len]; | ||
| reader.read_exact(&mut bytes)?; |
There was a problem hiding this comment.
WAL recovery trusts the length prefix and allocates vec![0u8; len] without bounds. A corrupted/truncated WAL can cause huge allocations or OOM. Add a reasonable maximum record size (and/or validate len against remaining file size) and treat invalid lengths as corruption (stop recovery or return an error).
Implemented the basic LSM-based key-value database with the following components:
Engine Core:
memtable, ensuring durability and recovery on startup.
on-disk files once it reaches a size threshold (1KB for demonstration).
Server & Protocol:
requests (Get, Set, Delete) and responses.
CLI Utility:
How to use:
cargo run --bin cli -- set mykey myvalueThe database ensures persistence by replaying the WAL upon server restart and flushing data to
SSTables when the memory limit is reached.