From 74373aebcfdfcf5c03e4fb6510d8dd664a0b9b88 Mon Sep 17 00:00:00 2001 From: Trinity Pointard Date: Tue, 6 Apr 2021 05:25:28 +0200 Subject: [PATCH] make most requested changes --- src/api/error.rs | 4 ++-- src/api/lib.rs | 1 - src/garage/main.rs | 3 +-- src/model/block.rs | 6 +++--- src/model/block_ref_table.rs | 8 +++----- src/model/key_table.rs | 3 ++- src/model/lib.rs | 1 - src/model/object_table.rs | 14 +++++++------- src/model/version_table.rs | 6 +++--- src/rpc/lib.rs | 1 - src/rpc/membership.rs | 11 ++++++----- src/rpc/ring.rs | 4 +++- src/rpc/rpc_client.rs | 9 +++++---- src/table/lib.rs | 1 - src/table/replication/parameters.rs | 1 - src/table/schema.rs | 4 ++-- src/util/error.rs | 1 - src/util/lib.rs | 1 - src/web/error.rs | 2 +- src/web/lib.rs | 1 - 20 files changed, 38 insertions(+), 44 deletions(-) diff --git a/src/api/error.rs b/src/api/error.rs index acd8ebf7..ad0174ad 100644 --- a/src/api/error.rs +++ b/src/api/error.rs @@ -29,7 +29,7 @@ pub enum Error { NotFound, // Category: bad request - /// The request used an invalid path + /// The request contained an invalid UTF-8 sequence in its path or in other parameters #[error(display = "Invalid UTF-8: {}", _0)] InvalidUTF8Str(#[error(source)] std::str::Utf8Error), @@ -65,7 +65,7 @@ impl From for Error { } impl Error { - /// Convert an error into an Http status code + /// Get the HTTP status code that best represents the meaning of the error for the client pub fn http_status_code(&self) -> StatusCode { match self { Error::NotFound => StatusCode::NOT_FOUND, diff --git a/src/api/lib.rs b/src/api/lib.rs index d036cdd6..be7e37c8 100644 --- a/src/api/lib.rs +++ b/src/api/lib.rs @@ -1,4 +1,3 @@ -#![deny(missing_crate_level_docs, missing_docs)] //! Crate for serving a S3 compatible API #[macro_use] extern crate log; diff --git a/src/garage/main.rs b/src/garage/main.rs index 34bf5501..a78e0f03 100644 --- a/src/garage/main.rs +++ b/src/garage/main.rs @@ -1,6 +1,5 @@ -#![deny(missing_crate_level_docs, missing_docs)] #![recursion_limit = "1024"] -//! Garage CLI, used to interact with a running Garage instance, and to launch a Garage +//! Garage CLI, used to interact with a running Garage instance, and to launch a Garage instance #[macro_use] extern crate log; diff --git a/src/model/block.rs b/src/model/block.rs index 38b2325c..89685630 100644 --- a/src/model/block.rs +++ b/src/model/block.rs @@ -157,7 +157,7 @@ impl BlockManager { } /// Write a block to disk - pub async fn write_block(&self, hash: &Hash, data: &[u8]) -> Result { + async fn write_block(&self, hash: &Hash, data: &[u8]) -> Result { let _lock = self.data_dir_lock.lock().await; let mut path = self.block_dir(hash); @@ -176,7 +176,7 @@ impl BlockManager { } /// Read block from disk, verifying it's integrity - pub async fn read_block(&self, hash: &Hash) -> Result { + async fn read_block(&self, hash: &Hash) -> Result { let path = self.block_path(hash); let mut f = match fs::File::open(&path).await { @@ -208,7 +208,7 @@ impl BlockManager { } /// Check if this node should have a block, but don't actually have it - pub async fn need_block(&self, hash: &Hash) -> Result { + async fn need_block(&self, hash: &Hash) -> Result { let needed = self .rc .get(hash.as_ref())? diff --git a/src/model/block_ref_table.rs b/src/model/block_ref_table.rs index 2c5f9bf9..1f0c7bb0 100644 --- a/src/model/block_ref_table.rs +++ b/src/model/block_ref_table.rs @@ -10,16 +10,14 @@ use crate::block::*; #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)] pub struct BlockRef { - // Primary key - /// Hash of the block + /// Hash of the block, used as partition key pub block: Hash, - // Sort key - // why a version on a hashed (probably immutable) piece of data? + /// Id of the Version for the object containing this block, used as sorting key pub version: UUID, // Keep track of deleted status - /// Is that block deleted + /// Is the Version that contains this block deleted pub deleted: crdt::Bool, } diff --git a/src/model/key_table.rs b/src/model/key_table.rs index 444f3949..e1dcd7f4 100644 --- a/src/model/key_table.rs +++ b/src/model/key_table.rs @@ -6,7 +6,7 @@ use garage_table::*; /// An api key #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)] pub struct Key { - /// The id of the key (immutable) + /// The id of the key (immutable), used as partition key pub key_id: String, /// The secret_key associated @@ -19,6 +19,7 @@ pub struct Key { pub deleted: crdt::Bool, /// Buckets in which the key is authorized. Empty if `Key` is deleted + // CRDT interaction: deleted implies authorized_buckets is empty pub authorized_buckets: crdt::LWWMap, } diff --git a/src/model/lib.rs b/src/model/lib.rs index 70d2e2ce..b4a8ddb7 100644 --- a/src/model/lib.rs +++ b/src/model/lib.rs @@ -1,4 +1,3 @@ -#![warn(missing_docs)] #[macro_use] extern crate log; diff --git a/src/model/object_table.rs b/src/model/object_table.rs index 5b026ceb..ff42d065 100644 --- a/src/model/object_table.rs +++ b/src/model/object_table.rs @@ -14,13 +14,13 @@ use crate::version_table::*; /// An object #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)] pub struct Object { - /// The bucket in which the object is stored + /// The bucket in which the object is stored, used as partition key pub bucket: String, - /// The key at which the object is stored in its bucket + /// The key at which the object is stored in its bucket, used as sorting key pub key: String, - /// The list of known versions of the object + /// The list of currenty stored versions of the object versions: Vec, } @@ -53,7 +53,7 @@ impl Object { } } - /// Get a list of all versions known for `Object` + /// Get a list of currently stored versions of `Object` pub fn versions(&self) -> &[ObjectVersion] { &self.versions[..] } @@ -77,7 +77,7 @@ pub enum ObjectVersionState { Uploading(ObjectVersionHeaders), /// The version is fully received Complete(ObjectVersionData), - /// The version was never fully received + /// The version uploaded containded errors or the upload was explicitly aborted Aborted, } @@ -105,7 +105,7 @@ impl CRDT for ObjectVersionState { /// Data about an object version #[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Debug, Serialize, Deserialize)] pub enum ObjectVersionData { - /// The version is deleted + /// The object was deleted, this Version is a tombstone to mark it as such DeleteMarker, /// The object is short, it's stored inlined Inline(ObjectVersionMeta, #[serde(with = "serde_bytes")] Vec), @@ -159,7 +159,7 @@ impl ObjectVersion { } } - /// Is the object version available (received and not deleted) + /// Is the object version available (received and not a tombstone) pub fn is_data(&self) -> bool { match self.state { ObjectVersionState::Complete(ObjectVersionData::DeleteMarker) => false, diff --git a/src/model/version_table.rs b/src/model/version_table.rs index 428fac10..bb836868 100644 --- a/src/model/version_table.rs +++ b/src/model/version_table.rs @@ -13,7 +13,7 @@ use crate::block_ref_table::*; /// A version of an object #[derive(PartialEq, Clone, Debug, Serialize, Deserialize)] pub struct Version { - /// UUID of the version + /// UUID of the version, used as partition key pub uuid: UUID, // Actual data: the blocks for this version @@ -49,9 +49,9 @@ impl Version { #[derive(PartialEq, Eq, Clone, Copy, Debug, Serialize, Deserialize)] pub struct VersionBlockKey { - /// Number of the part, starting at 1 + /// Number of the part pub part_number: u64, - /// offset of the block in the file, starting at 0 + /// Offset of this sub-segment in its part pub offset: u64, } diff --git a/src/rpc/lib.rs b/src/rpc/lib.rs index 308baa83..96561d0e 100644 --- a/src/rpc/lib.rs +++ b/src/rpc/lib.rs @@ -1,4 +1,3 @@ -#![warn(missing_crate_level_docs, missing_docs)] //! Crate containing rpc related functions and types used in Garage #[macro_use] diff --git a/src/rpc/membership.rs b/src/rpc/membership.rs index c465ce68..4fce1a7b 100644 --- a/src/rpc/membership.rs +++ b/src/rpc/membership.rs @@ -1,4 +1,4 @@ -/// Module containing structs related to membership management +//! Module containing structs related to membership management use std::collections::HashMap; use std::fmt::Write as FmtWrite; use std::io::{Read, Write}; @@ -96,7 +96,7 @@ pub struct System { rpc_client: Arc>, pub(crate) status: watch::Receiver>, - /// The ring, viewed by this node + /// The ring pub ring: watch::Receiver>, update_lock: Mutex, @@ -114,9 +114,8 @@ struct Updaters { #[derive(Debug, Clone)] pub struct Status { /// Mapping of each node id to its known status - // considering its sorted regularly, maybe it should be a btreeset? pub nodes: HashMap>, - /// Hash of this entry + /// Hash of `nodes`, used to detect when nodes have different views of the cluster pub hash: Hash, } @@ -380,7 +379,7 @@ impl System { id_option, addr, sys.rpc_client - .get_addr() + .by_addr() .call(&addr, ping_msg_ref, PING_TIMEOUT) .await, ) @@ -418,6 +417,8 @@ impl System { } } else if let Some(id) = id_option { if let Some(st) = status.nodes.get_mut(id) { + // TODO this might double-increment the value as the counter is already + // incremented for any kind of failure in rpc_client st.num_failures.fetch_add(1, Ordering::SeqCst); if !st.is_up() { warn!("Node {:?} seems to be down.", id); diff --git a/src/rpc/ring.rs b/src/rpc/ring.rs index 6f341fa8..04f8b590 100644 --- a/src/rpc/ring.rs +++ b/src/rpc/ring.rs @@ -1,4 +1,5 @@ //! Module containing types related to computing nodes which should receive a copy of data blocks +//! and metadata use std::collections::{HashMap, HashSet}; use std::convert::TryInto; @@ -197,7 +198,7 @@ impl Ring { top >> (16 - PARTITION_BITS) } - /// Get the list of partitions and + /// Get the list of partitions and the first hash of a partition key that would fall in it pub fn partitions(&self) -> Vec<(Partition, Hash)> { let mut ret = vec![]; @@ -211,6 +212,7 @@ impl Ring { ret } + // TODO rename this function as it no longer walk the ring /// Walk the ring to find the n servers in which data should be replicated pub fn walk_ring(&self, from: &Hash, n: usize) -> Vec { if self.ring.len() != 1 << PARTITION_BITS { diff --git a/src/rpc/rpc_client.rs b/src/rpc/rpc_client.rs index 53051cee..8a6cc721 100644 --- a/src/rpc/rpc_client.rs +++ b/src/rpc/rpc_client.rs @@ -53,6 +53,7 @@ impl RequestStrategy { self } /// Set if requests can be dropped after quorum has been reached + /// In general true for read requests, and false for write pub fn interrupt_after_quorum(mut self, interrupt: bool) -> Self { self.rs_interrupt_after_quorum = interrupt; self @@ -103,8 +104,8 @@ impl RpcClient { self.local_handler.swap(Some(Arc::new((my_id, handler)))); } - /// Get the server address this client connect to - pub fn get_addr(&self) -> &RpcAddrClient { + /// Get a RPC client to make calls using node's SocketAddr instead of its ID + pub fn by_addr(&self) -> &RpcAddrClient { &self.rpc_addr_client } @@ -166,7 +167,7 @@ impl RpcClient { } /// Make a RPC call to multiple servers, returning either a Vec of responses, or an error if - /// strategy could not be respected due to to many errors + /// strategy could not be respected due to too many errors pub async fn try_call_many( self: &Arc, to: &[UUID], @@ -226,7 +227,7 @@ impl RpcClient { } } -/// Endpoint to which send RPC +/// Thin wrapper arround an `RpcHttpClient` specifying the path of the request pub struct RpcAddrClient { phantom: PhantomData, diff --git a/src/table/lib.rs b/src/table/lib.rs index 8dcb115d..c3e14ab8 100644 --- a/src/table/lib.rs +++ b/src/table/lib.rs @@ -1,4 +1,3 @@ -#![warn(missing_docs)] #![recursion_limit = "1024"] #[macro_use] diff --git a/src/table/replication/parameters.rs b/src/table/replication/parameters.rs index 0ab9ee5a..c2c78c8b 100644 --- a/src/table/replication/parameters.rs +++ b/src/table/replication/parameters.rs @@ -16,7 +16,6 @@ pub trait TableReplication: Send + Sync { fn write_nodes(&self, hash: &Hash) -> Vec; /// Responses needed to consider a write succesfull fn write_quorum(&self) -> usize; - // this feels like its write_nodes().len() - write_quorum() fn max_write_errors(&self) -> usize; // Accessing partitions, for Merkle tree & sync diff --git a/src/table/schema.rs b/src/table/schema.rs index f5fde95f..c17ccc15 100644 --- a/src/table/schema.rs +++ b/src/table/schema.rs @@ -4,7 +4,7 @@ use garage_util::data::*; use crate::crdt::CRDT; -/// Trait for partitionnable data +/// Trait for field used to partition data pub trait PartitionKey { /// Get the key used to partition fn hash(&self) -> Hash; @@ -22,7 +22,7 @@ impl PartitionKey for Hash { } } -/// Trait for sortable data +/// Trait for field used to sort data pub trait SortKey { /// Get the key used to sort fn sort_key(&self) -> &[u8]; diff --git a/src/util/error.rs b/src/util/error.rs index 13cbeba3..32dccbe6 100644 --- a/src/util/error.rs +++ b/src/util/error.rs @@ -1,5 +1,4 @@ //! Module containing error types used in Garage -#![allow(missing_docs)] use err_derive::Error; use hyper::StatusCode; use std::io; diff --git a/src/util/lib.rs b/src/util/lib.rs index faacd9c1..c080e3a3 100644 --- a/src/util/lib.rs +++ b/src/util/lib.rs @@ -1,4 +1,3 @@ -#![warn(missing_crate_level_docs, missing_docs)] //! Crate containing common functions and types used in Garage #[macro_use] diff --git a/src/web/error.rs b/src/web/error.rs index 2b8aeebe..f6afbb42 100644 --- a/src/web/error.rs +++ b/src/web/error.rs @@ -19,7 +19,7 @@ pub enum Error { #[error(display = "Not found")] NotFound, - /// The client requested a malformed path + /// The request contained an invalid UTF-8 sequence in its path or in other parameters #[error(display = "Invalid UTF-8: {}", _0)] InvalidUTF8(#[error(source)] std::str::Utf8Error), diff --git a/src/web/lib.rs b/src/web/lib.rs index 3e978af6..c06492a3 100644 --- a/src/web/lib.rs +++ b/src/web/lib.rs @@ -1,4 +1,3 @@ -#![deny(missing_crate_level_docs, missing_docs)] //! Crate for handling web serving of s3 bucket #[macro_use] extern crate log;