add bidirectional kvs #2898
add bidirectional kvs #2898
Conversation
📝 WalkthroughWalkthroughThis pull request reorders workspace member entries in the root Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BidiMap as KVS_BidiMap
Client->>BidiMap: new(path)
Note right of BidiMap: Initialize bidirectional mapping buckets
Client->>BidiMap: set(key, value, timestamp)
Note right of BidiMap: Entry added to both mapping buckets
Client->>BidiMap: get(key)
BidiMap-->>Client: (value, timestamp)
Client->>BidiMap: get_inverse(value)
BidiMap-->>Client: (key, timestamp)
Client->>BidiMap: delete(key)
BidiMap-->>Client: confirmation
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[CHATOPS:HELP] ChatOps commands.
|
![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
🧹 Nitpick comments (4)
rust/libs/kvs/src/lib.rs (4)
21-30: Consider adopting custom errors.The methods in your
BidiMaptrait return generalizedResult<_, anyhow::Error>. While it works, using a specialized error type (e.g., an enum) could enhance maintainability and clarity.
49-55: Validate store configuration and path handling.While this function creates a new store at the given
path, consider logging or validating path correctness, ensuring that any creation, permission, or concurrency issues are handled gracefully.
57-66: Enhance “key not found” error context.Returning a generic “key not found” message could be expanded to include the missing key’s value or more context. This helps with debugging, especially in production.
105-113: Offer an iterator-based approach for range.Using
rangewith a closure is fine, but returning an iterator could be more flexible for callers. Iterators compose well with Rust’s standard library patterns (e.g., map, filter).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rust/Cargo.toml(1 hunks)rust/libs/kvs/Cargo.toml(1 hunks)rust/libs/kvs/src/lib.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: check-format-diff
- GitHub Check: Run tests for Rust
- GitHub Check: build / build
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
rust/Cargo.toml (1)
24-26: Reordered workspace members successfully.No functional changes are introduced by rearranging the workspace members, and the updated sequence appears consistent with the addition of the
libs/kvsentry. All references remain intact, so this reordering is acceptable.rust/libs/kvs/Cargo.toml (1)
1-28: Review package and dependency declarations.
- Your pinned versions (e.g.,
anyhow = "1.0.97",tokio = "1.44.1") look stable, which is beneficial for reproducible builds. Regularly check for security patches and updates.- The crate name
"kvs"is descriptive but may collide with potential external crates; consider a more specific name if you plan to publish.- Overall, this file is well-structured, and the dependencies align with typical usage for error handling, serialization, and asynchronous programming.
rust/libs/kvs/src/lib.rs (5)
32-35: Ensure correct lifetime management of buckets.The
Bidi<'a>struct holds bucket references with'a. Verify that the referenced store remains in scope for'a; otherwise, dangling references might arise if the store is closed or dropped.
115-120: Good integrity check between ou and uo buckets.Ensuring both buckets have the same length avoids silent data mismatches. The
ensure!macro is a clear solution for quick validation. This is a robust touch.
127-180: Tests: Single-thread functional coverage.
- Cleanup strategy with
teardownis solid; ephemeral directories help keep test artifacts isolated.- Basic CRUD operations are thoroughly tested, ensuring correctness and verifying that store files persist on disk.
182-213: Tests: Range coverage.Iterating through inserted items is a reliable approach for verifying that both keys and metadata (timestamps) are correct. This check ensures items are properly stored and retrieved.
215-313: Tests: Concurrent read/write coverage.
- The multi-threaded test thoroughly exercises read and write operations across multiple tasks, adding confidence in concurrency safety.
- Consider verifying partial-failure scenarios under concurrency (e.g., one thread fails to write) if you anticipate real-world usage with potential I/O errors.
| fn set(&mut self, key: &str, value: u32, timestamp: u128) -> Result<()> { | ||
| self.uo.set(&key, &Bincode(ValueUo{ | ||
| value, | ||
| timestamp, | ||
| }))?; | ||
| self.ou.set(&Integer::from(value), &Bincode(ValueOu { | ||
| value: key.to_owned(), | ||
| timestamp, | ||
| }))?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle partial failure for set operations.
If the first self.uo.set(...) call succeeds but the second self.ou.set(...) fails, the buckets become inconsistent. Consider a rollback or transactional approach to revert the first write if the second fails.
kmrmt
requested review from
a team,
kpango and
vankichi
and removed request for
a team
Description
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
Chores
New Features