feat: add replication factor support and chunk location registration#1
feat: add replication factor support and chunk location registration#1
Conversation
murka
commented
Jan 16, 2026
murka
commented
- Add replication_factor parameter to allocate_chunks and Put command
- Implement register_chunk_location method in client
- Update chunk download to use hex format for CID
- Add replication_factor parameter to allocate_chunks and Put command - Implement register_chunk_location method in client - Update chunk download to use hex format for CID
Add replication factor to
|
There was a problem hiding this comment.
Pull request overview
This pull request adds replication factor support to the distributed storage system, enabling users to specify how many copies of each chunk should be stored across nodes. It also implements chunk location registration to track where chunks are stored and updates chunk operations to use consistent hex formatting for CIDs.
Changes:
- Added
replication_factorCLI parameter to the Put command with a default value of 1 - Implemented
register_chunk_locationmethod to register chunk locations with the gateway after upload - Updated chunk download operations to use hex format consistently for CID references
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/main.rs | Added replication_factor CLI argument to Put command with short flag -r and default value of 1 |
| src/commands.rs | Threaded replication_factor parameter through put function to allocate_chunks; changed CID references from to_string() to to_hex() for chunk downloads |
| src/client.rs | Added replication_factor parameter to allocate_chunks request; implemented register_chunk_location method; integrated location registration into upload_chunk workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use crate::client::{MonoceClient, NamespaceEntry}; | ||
|
|
||
| pub async fn put(endpoint: &str, path: &str, name: Option<&str>) -> Result<()> { | ||
| pub async fn put(endpoint: &str, path: &str, name: Option<&str>, replication_factor: u32) -> Result<()> { |
There was a problem hiding this comment.
The replication_factor parameter should be validated to ensure it's not zero. A replication factor of 0 would be meaningless and could cause issues in the distributed storage system. Consider adding validation to ensure the value is at least 1 before making the API call.
| path: String, | ||
| #[arg(long, help = "Target namespace path")] | ||
| name: Option<String>, | ||
| #[arg(short = 'r', long, default_value = "1", help = "Replication factor")] |
There was a problem hiding this comment.
Consider adding validation to reject values above a reasonable maximum (e.g., 10 or 100) to prevent accidentally setting an extremely high replication factor that could overwhelm the storage cluster. Use clap's value_parser with a range check.
| #[arg(short = 'r', long, default_value = "1", help = "Replication factor")] | |
| #[arg( | |
| short = 'r', | |
| long, | |
| default_value_t = 1u32, | |
| value_parser = clap::value_parser!(u32).range(1..=100), | |
| help = "Replication factor (1-100)" | |
| )] |
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn register_chunk_location(&self, cid_hex: &str, node_addr: &str) -> Result<()> { |
There was a problem hiding this comment.
The node_addr parameter is not validated before use. If an empty string is passed, this would create an invalid URL "http://". Consider adding validation to ensure node_addr is not empty and has a valid format before processing.
|
|
||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
The new public method register_chunk_location lacks documentation. Consider adding a doc comment explaining its purpose, parameters, and expected behavior, especially since it's a public API that could be used by other parts of the codebase.
| /// Register the location of a chunk with the gateway. | |
| /// | |
| /// * `cid_hex` - Hex-encoded CID of the chunk whose location is being registered. | |
| /// * `node_addr` - Address of the node storing the chunk. May be either a full | |
| /// `http://` or `https://` URL or a bare host[:port], which will be normalized | |
| /// to an `http://` URL before being sent to the gateway. | |
| /// | |
| /// Returns an error if the underlying HTTP request fails or the gateway responds | |
| /// with a non-success status code. |
| let node_address = if node_addr.starts_with("http://") || node_addr.starts_with("https://") { | ||
| node_addr.to_string() | ||
| } else { | ||
| format!("http://{}", node_addr) | ||
| }; |
There was a problem hiding this comment.
The URL normalization logic (checking for http:// or https:// prefix and adding http:// if needed) is duplicated between upload_chunk and register_chunk_location. Consider extracting this into a helper method to reduce code duplication and ensure consistency.
1 participant
