From ace1d190b7c19527efe2d4d6b55dcf55942620dd Mon Sep 17 00:00:00 2001 From: Gabriel Viganotti Date: Thu, 8 Feb 2024 14:05:56 -0300 Subject: [PATCH] refactor(cli): improvements based on peer review --- README.md | 2 +- sn_cli/src/subcommands/folders.rs | 75 ++++++++++++++++--------------- sn_client/src/folders.rs | 32 ++++++++----- 3 files changed, 61 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index 9414965312..aab8bc89fb 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ The folders storage capability can be demonstrated by uploading folders to the l Upload a directory: ```bash -cargo run --bin safe --features local-discovery -- files upload +cargo run --bin safe --features local-discovery -- folders upload ``` After it finishes uploading the complete directory, with files and sub-directories, it will show the address where the main directory can be pulled from. diff --git a/sn_cli/src/subcommands/folders.rs b/sn_cli/src/subcommands/folders.rs index 23caacecbf..9d5f04d2d7 100644 --- a/sn_cli/src/subcommands/folders.rs +++ b/sn_cli/src/subcommands/folders.rs @@ -8,11 +8,9 @@ use super::files::{download_file, upload_files, ChunkManager, UploadedFile, UPLOADED_FILES}; -use sn_client::{ - Client, FilesApi, FolderEntry, FoldersApi, WalletClient, BATCH_SIZE, MAX_UPLOAD_RETRIES, -}; +use sn_client::{Client, FilesApi, FolderEntry, FoldersApi, WalletClient, BATCH_SIZE}; -use sn_protocol::storage::{Chunk, ChunkAddress, RegisterAddress}; +use sn_protocol::storage::{Chunk, ChunkAddress, RegisterAddress, RetryStrategy}; use sn_transfers::HotWallet; use clap::Parser; @@ -22,6 +20,7 @@ use color_eyre::{ }; use std::{ collections::BTreeMap, + ffi::OsString, fs::create_dir_all, path::{Path, PathBuf}, }; @@ -43,10 +42,12 @@ pub enum FoldersCmds { /// Should the file be made accessible to all. (This is irreversible) #[clap(long, name = "make_public", default_value = "false", short = 'p')] make_public: bool, - /// The retry_count for retrying failed chunks - /// during payment and upload processing. - #[clap(long, default_value_t = MAX_UPLOAD_RETRIES, short = 'r')] - max_retries: usize, + /// Set the strategy to use on chunk upload failure. Does not modify the spend failure retry attempts yet. + /// + /// Choose a retry strategy based on effort level, from 'quick' (least effort), through 'balanced', + /// to 'persistent' (most effort). + #[clap(long, default_value_t = RetryStrategy::Balanced, short = 'r', help = "Sets the retry strategy on upload failure. Options: 'quick' for minimal effort, 'balanced' for moderate effort, or 'persistent' for maximum effort.")] + retry_strategy: RetryStrategy, }, Download { /// The hex address of a folder. @@ -54,10 +55,16 @@ pub enum FoldersCmds { folder_addr: String, /// The name to apply to the downloaded folder. #[clap(name = "target folder name")] - folder_name: String, + folder_name: OsString, /// The batch_size for parallel downloading #[clap(long, default_value_t = BATCH_SIZE , short='b')] batch_size: usize, + /// Set the strategy to use on downloads failure. + /// + /// Choose a retry strategy based on effort level, from 'quick' (least effort), through 'balanced', + /// to 'persistent' (most effort). + #[clap(long, default_value_t = RetryStrategy::Quick, short = 'r', help = "Sets the retry strategy on download failure. Options: 'quick' for minimal effort, 'balanced' for moderate effort, or 'persistent' for maximum effort.")] + retry_strategy: RetryStrategy, }, } @@ -71,8 +78,8 @@ pub(crate) async fn folders_cmds( FoldersCmds::Upload { path, batch_size, - max_retries, make_public, + retry_strategy, } => { upload_files( path.clone(), @@ -81,7 +88,7 @@ pub(crate) async fn folders_cmds( root_dir.to_path_buf(), verify_store, batch_size, - max_retries, + retry_strategy, ) .await?; @@ -92,12 +99,12 @@ pub(crate) async fn folders_cmds( // add chunked files to the corresponding Folders for chunked_file in chunk_manager.iter_chunked_files() { - if let (Some(file_name), Some(parent)) = ( - chunked_file.file_name.to_str(), - chunked_file.file_path.parent(), - ) { + if let Some(parent) = chunked_file.file_path.parent() { if let Some(folder) = folders.get_mut(parent) { - folder.add_file(file_name.to_string(), chunked_file.head_chunk_address)?; + folder.add_file( + chunked_file.file_name.clone(), + chunked_file.head_chunk_address, + )?; } } } @@ -111,7 +118,7 @@ pub(crate) async fn folders_cmds( pay_and_upload_folders(folders, verify_store, client, root_dir).await?; println!( - "\nFolder hierarchy from {path:?} uploaded susccessfully at {}", + "\nFolder hierarchy from {path:?} uploaded successfully at {}", root_dir_address.to_hex() ); } @@ -119,12 +126,13 @@ pub(crate) async fn folders_cmds( folder_addr, folder_name, batch_size, + retry_strategy, } => { let address = RegisterAddress::from_hex(&folder_addr).expect("Failed to parse Folder address"); let download_dir = dirs_next::download_dir().unwrap_or(root_dir.to_path_buf()); - let download_folder_path = download_dir.join(folder_name); + let download_folder_path = download_dir.join(folder_name.clone()); println!( "Downloading onto {download_folder_path:?} from {} with batch-size {batch_size}", address.to_hex() @@ -136,12 +144,12 @@ pub(crate) async fn folders_cmds( let mut files_to_download = vec![]; let mut folders_to_download = - vec![("".to_string(), address, download_folder_path.clone())]; + vec![(folder_name, address, download_folder_path.clone())]; while let Some((name, folder_addr, target_path)) = folders_to_download.pop() { if !name.is_empty() { println!( - "Downloading Folder '{name}' from {}", + "Downloading Folder {name:?} from {}", hex::encode(folder_addr.xorname()) ); } @@ -173,10 +181,11 @@ pub(crate) async fn folders_cmds( download_file( files_api.clone(), *addr.xorname(), - (file_name.into(), local_data_map), + (file_name, local_data_map), &path, false, batch_size, + retry_strategy, ) .await; } @@ -197,18 +206,14 @@ fn build_folders_hierarchy( .filter_entry(|e| e.file_type().is_dir()) .flatten() .filter_map(|entry| { - entry - .path() - .parent() - .zip(entry.file_name().to_str()) - .map(|(parent, name)| { - ( - entry.path().to_path_buf(), - entry.depth(), - parent.to_owned(), - name.to_owned(), - ) - }) + entry.path().parent().map(|parent| { + ( + entry.path().to_path_buf(), + entry.depth(), + parent.to_owned(), + entry.file_name().to_owned(), + ) + }) }) { let curr_folder_addr = *folders @@ -290,8 +295,8 @@ async fn download_folder( client: &Client, target_path: &Path, folder_addr: RegisterAddress, - files_to_download: &mut Vec<(String, ChunkAddress, PathBuf)>, - folders_to_download: &mut Vec<(String, RegisterAddress, PathBuf)>, + files_to_download: &mut Vec<(OsString, ChunkAddress, PathBuf)>, + folders_to_download: &mut Vec<(OsString, RegisterAddress, PathBuf)>, ) -> Result<()> { create_dir_all(target_path)?; let folders_api = FoldersApi::retrieve(client.clone(), root_dir, folder_addr).await?; diff --git a/sn_client/src/folders.rs b/sn_client/src/folders.rs index 10669946b1..a392a619b9 100644 --- a/sn_client/src/folders.rs +++ b/sn_client/src/folders.rs @@ -19,6 +19,7 @@ use libp2p::PeerId; use serde::{Deserialize, Serialize}; use std::{ collections::BTreeSet, + ffi::OsString, path::{Path, PathBuf}, }; @@ -68,19 +69,13 @@ impl FoldersApi { } /// Add provided file as entry of this Folder (locally). - pub fn add_file(&mut self, name: String, address: ChunkAddress) -> Result<()> { - let entry = (name, FolderEntry::File(address)); - self.register - .write_atop(&rmp_serde::to_vec(&entry)?, &BTreeSet::default())?; - Ok(()) + pub fn add_file(&mut self, name: OsString, address: ChunkAddress) -> Result<()> { + self.add_entry(name, FolderEntry::File(address)) } /// Add subfolder as entry of this Folder (locally). - pub fn add_folder(&mut self, name: String, address: RegisterAddress) -> Result<()> { - let entry = (name, FolderEntry::Folder(address)); - self.register - .write_atop(&rmp_serde::to_vec(&entry)?, &BTreeSet::default())?; - Ok(()) + pub fn add_folder(&mut self, name: OsString, address: RegisterAddress) -> Result<()> { + self.add_entry(name, FolderEntry::Folder(address)) } /// Sync local Folder with the network. @@ -112,11 +107,24 @@ impl FoldersApi { } /// Returns the list of entries of this Folder - pub fn entries(&self) -> Result> { + pub fn entries(&self) -> Result> { let mut entries = vec![]; for (_, entry) in self.register.read() { - entries.push(rmp_serde::from_slice(&entry)?) + let (name, folder_entry): (String, FolderEntry) = rmp_serde::from_slice(&entry)?; + entries.push((name.into(), folder_entry)); } Ok(entries) } + + // Private helpers + + // Add the given entry to the underlying Register + fn add_entry(&mut self, name: OsString, entry: FolderEntry) -> Result<()> { + // TODO: conversion to String will be removed when metadata is moved out of the Register + let name = name.into_string().unwrap_or("unknown".to_string()); + let entry = (name, entry); + self.register + .write_atop(&rmp_serde::to_vec(&entry)?, &BTreeSet::default())?; + Ok(()) + } }