♻️ Refactor: bidirectional map module structure #3218
Conversation
Deploying vald with
|
| Latest commit: |
06c12ae
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5ce38a0c.vald.pages.dev |
| Branch Preview URL: | https://refactor-rust-change-bidirec.vald.pages.dev |
📝 WalkthroughWalkthroughAdds a new map module with submodules for codec, error, and type bounds; refactors bidirectional map to new public types (BidirectionalMap/Builder), centralizes errors to a new Error enum, and moves codec abstraction to a dedicated module with a default BincodeCodec. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Builder as BidirectionalMapBuilder
participant Map as BidirectionalMap
participant Codec as Codec/BincodeCodec
participant DB as Sled (KV store)
rect rgb(245,248,255)
Note over Builder,Map: Build
Client->>Builder: configure(...)
Builder->>DB: open/init
DB-->>Builder: handle/Ok or Error
Builder-->>Client: Arc<BidirectionalMap> or Error
end
rect rgb(245,255,245)
Note over Client,DB: Set
Client->>Map: set(key, value)
Map->>Codec: encode(key), encode(value)
Codec-->>Map: bytes or Error::Codec
Map->>DB: transaction put fwd/idx
DB-->>Map: Ok or Error::Sled/::SledTransaction
Map-->>Client: Ok or Error
end
rect rgb(255,250,245)
Note over Client,DB: Get / Get Inverse
Client->>Map: get(key)
Map->>Codec: encode(key)
Codec-->>Map: bytes or Error::Codec
Map->>DB: fetch forward
DB-->>Map: bytes or NotFound
Map->>Codec: decode(value)
Codec-->>Map: value or Error::Codec
Map-->>Client: value or Error
end
rect rgb(255,245,250)
Note over Client,DB: Delete / Delete Inverse
Client->>Map: delete(key) / delete_inverse(value)
Map->>DB: transaction delete fwd+idx
DB-->>Map: Ok or Error
Map-->>Client: Ok or Error
end
rect rgb(245,245,255)
Note over Client,Map: Range / Stream
Client->>Map: range(callback) / range_stream()
Map->>DB: iterate
DB-->>Map: (k,v,ts) items or Error
Map->>Codec: decode items
Map-->>Client: callback Result / Stream<Item=Result<...>>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
|
/format |
|
[CHATOPS:HELP] ChatOps commands.
|
github-actions
bot
added
actions/backport/release/v1.7
size/L
type/refactoring
priority/low
labels
kmrmt
changed the title
|
[FORMAT] Updating license headers and formatting go codes triggered by kmrmt. |
kmrmt
requested review from
a team,
Matts966 and
datelier
and removed request for
a team
|
[FORMAT] Failed to format. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
rust/libs/kvs/src/map/codec.rs (2)
26-31: Consider loosening trait bounds for flexibility.The
Codectrait currently requires both serde and bincode traits for all operations. This creates unnecessary coupling - the trait itself should be agnostic to the specific serialization framework used by implementations.Consider this more flexible approach:
pub trait Codec: Send + Sync + 'static { /// Serializes a given value into a byte vector. - fn encode<T: Serialize + Encode + ?Sized>(&self, v: &T) -> Result<Vec<u8>, Error>; + fn encode<T>(&self, v: &T) -> Result<Vec<u8>, Error> + where T: ?Sized, Self: CodecEncode<T>; /// Deserializes a byte slice into a value of a specific type. - fn decode<T: DeserializeOwned + Decode<()>>(&self, bytes: &[u8]) -> Result<T, Error>; + fn decode<T>(&self, bytes: &[u8]) -> Result<T, Error> + where Self: CodecDecode<T>; }Then implementations can specify their own requirements. However, given that the current design works for the immediate use case, this is optional.
38-42: Consider adding context to codec errors.When encoding fails, it would be helpful to know what type was being encoded for debugging purposes.
fn encode<T: Serialize + Encode + ?Sized>(&self, v: &T) -> Result<Vec<u8>, Error> { bincode::encode_to_vec(v, bincode_standard()).map_err(|e| Error::Codec { - source: Box::new(e), + source: Box::new(e) as Box<dyn std::error::Error + Send + Sync>, }) }Consider wrapping the error with additional context about the operation, though the current implementation is acceptable.
rust/libs/kvs/src/map/types.rs (1)
49-74: Consider ifValueTypeneeds Hash and Eq bounds.While
KeyTypeclearly needsHashandEqfor indexing, values typically don't require these traits unless they're also used as keys (which they are in the bidirectional map). The current design is correct for a bidirectional map, but the trait nameValueTypemight be misleading.Consider either:
- Renaming to something like
BidiTypeto indicate it can serve as both key and value- Adding a comment explaining why values need these traits in a bidirectional context
rust/libs/kvs/src/map/bidirectional_map.rs (1)
148-161: Consider extracting common encoding logic.The encoding of payloads with timestamps appears in multiple places. Consider extracting this into a helper method to reduce duplication.
Add a helper method:
fn encode_with_timestamp(&self, data: Vec<u8>, timestamp: u128) -> Result<Vec<u8>, Error> { bincode::encode_to_vec((data, timestamp), bincode_standard()) .map_err(|e| Error::Codec { source: Box::new(e), }) }This would simplify the code in
setand potentially other methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rust/libs/kvs/src/map.rs(1 hunks)rust/libs/kvs/src/map/bidirectional_map.rs(29 hunks)rust/libs/kvs/src/map/codec.rs(1 hunks)rust/libs/kvs/src/map/error.rs(1 hunks)rust/libs/kvs/src/map/types.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Matts966
PR: vdaas/vald#2928
File: pkg/gateway/lb/handler/grpc/linear_search.go:271-333
Timestamp: 2025-04-15T05:32:26.428Z
Learning: For the vald project, refactoring PRs should be kept separate from semantic changes that alter behavior. The current PR "Divide LB Gateway handler.go" is focused solely on splitting the file into smaller, more manageable components without changing functionality.
🧬 Code graph analysis (3)
rust/libs/kvs/src/map.rs (1)
rust/libs/kvs/src/map/bidirectional_map.rs (1)
codec(68-75)
rust/libs/kvs/src/map/codec.rs (1)
rust/libs/kvs/src/map/bidirectional_map.rs (1)
new(56-63)
rust/libs/kvs/src/map/error.rs (1)
internal/net/grpc/codes/codes.go (2)
NotFound(29-29)Internal(37-37)
⏰ 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). (3)
- GitHub Check: check-format-diff
- GitHub Check: Run tests for Rust
- GitHub Check: runner / textlint
🔇 Additional comments (9)
rust/libs/kvs/src/map/error.rs (1)
19-48: LGTM! Well-structured error type with comprehensive error handling.The centralized error type properly consolidates all error variants with appropriate use of
thiserror's features. The automatic conversions via#[from]and proper source propagation through#[source]enable clean error propagation throughout the codebase.rust/libs/kvs/src/map.rs (1)
17-20: LGTM! Clean module aggregator structure.The module organization is logical and follows Rust best practices for module structure. Exposing the submodules publicly enables proper abstraction and separation of concerns.
rust/libs/kvs/src/map/types.rs (1)
22-47: LGTM! Clear trait bounds with good documentation.The marker traits effectively bundle the requirements for keys and values in the KVS system. The blanket implementations eliminate boilerplate for users.
rust/libs/kvs/src/map/bidirectional_map.rs (6)
46-52: Good use of the builder pattern for configuration.The builder struct properly encapsulates configuration options and provides a fluent API for construction.
54-63: LGTM! Clean constructor with sensible defaults.The constructor properly initializes with a default codec and enables scanning by default, which is the safe choice.
224-227: Good API improvement with the callback signature.The new signature returning
Result<bool, Error>allows callbacks to propagate errors properly, which is a significant improvement over silent error handling.
269-279: LGTM! Proper async flush implementation.The flush method correctly uses
spawn_blockingfor the I/O operation and properly propagates errors through the?operator chain.
393-571: Comprehensive test coverage!The integration tests thoroughly cover CRUD operations, concurrent access, range operations, and edge cases. The use of
TestGuardfor cleanup is a good practice.
123-129: No compatibility issues with the new borrow constraintAll existing
map.getcalls inbidirectional_map.rsuse&str, which satisfiesK: Borrow<Q>under the updated signature. No further changes are required.
Description
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit
New Features
Refactor