Deuxfleurs/garage
38
58
Fork 75
Code Issues 104 Pull requests 16 Releases 30 Wiki Activity

add doc comments #53

Merged
lx merged 10 commits from trinity-1686a/garage:doc-comments into main 2021-04-08 13:01:22 +00:00
Conversation 0 Commits 10 Files changed 38 +6 -15
6 changed files with 6 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 718ae00548 - Show all commits

1
src/model/block.rs
View file

@ -250,7 +250,6 @@ impl BlockManager {
}
/// Decrement the number of time a block is used
// when counter reach 0, it seems not put to resync which I assume put it to gc?
pub fn block_decref(&self, hash: &Hash) -> Result<(), Error> {
trinity-1686a marked this conversation as resolved Outdated
lx commented 2021-03-27 14:11:48 +00:00
Outdated
Review
Copy link

resync does both: if the rc is >0 and the block is not present, ask it to peers. on the contrary if rc=0 and block is present, delete it locally (after checking that no peer needs it)

resync does both: if the rc is >0 and the block is not present, ask it to peers. on the contrary if rc=0 and block is present, delete it locally (after checking that no peer needs it)
trinity-1686a commented 2021-04-06 01:17:24 +00:00
Outdated
Review
Copy link

I understand resync both get missing blocks and remove unnecessary blocks. My problem is that if I understand the code correctly, when the counter go down from 1 to 0, the value for new_rc is Some(0) (actually a Vec of 8 0u8), which is not None, so the block is not put to resync, and don't get deleted, unless some background thread regularly scan the table, which sound inneficient

I understand resync both get missing blocks and remove unnecessary blocks. My problem is that if I understand the code correctly, when the counter go down from 1 to 0, the value for new_rc is Some(0) (actually a Vec of 8 0u8), which is not None, so the block is not put to resync, and don't get deleted, unless some background thread regularly scan the table, which sound inneficient
lx commented 2021-04-06 14:03:10 +00:00
Outdated
Review
Copy link

block_decref actually deletes the value in the rc when it reaches 0, so it is indeed None and not Some(0), unless I'm mistaken? Maybe you're not looking at the last version of the code?

`block_decref` actually deletes the value in the rc when it reaches 0, so it is indeed None and not Some(0), unless I'm mistaken? Maybe you're not looking at the last version of the code?
trinity-1686a commented 2021-04-07 10:06:43 +00:00
Outdated
Review
Copy link

Nevermind, I somehow read old_v >= 1 instead of old_v > 1

Nevermind, I somehow read `old_v >= 1` instead of `old_v > 1`
let new_rc = self.rc.update_and_fetch(&hash, |old| {
let old_v = old.map(u64_from_be_bytes).unwrap_or(0);

8
src/model/bucket_table.rs
View file

@ -57,7 +57,7 @@ impl CRDT for BucketParams {
}
impl BucketParams {
/// Create a new default `BucketParams`
/// Initializes a new instance of the Bucket struct
pub fn new() -> Self {
BucketParams {
authorized_keys: crdt::LWWMap::new(),
@ -75,7 +75,7 @@ impl Bucket {
}
}
/// Query if bucket is deleted
/// Returns true if this represents a deleted bucket
lx commented 2021-03-27 14:17:12 +00:00
Outdated
Review
Copy link

This formulation is ambiguous/misleading, as is_deleted does not do a database query, it just checks if this struct that was returned earlier from the db has the deleted flag

This formulation is ambiguous/misleading, as `is_deleted` does not do a database query, it just checks if this struct that was returned earlier from the db has the deleted flag
lx commented 2021-04-06 14:35:47 +00:00
Outdated
Review
Copy link

Suggestion: returns true if this represents a deleted bucket

Suggestion: `returns true if this represents a deleted bucket`
pub fn is_deleted(&self) -> bool {
*self.state.get() == BucketState::Deleted
}
@ -113,10 +113,6 @@ impl TableSchema for BucketTable {
type E = Bucket;
type Filter = DeletedFilter;
fn updated(&self, _old: Option<Self::E>, _new: Option<Self::E>) {
// nothing to do when updated
}
fn matches_filter(entry: &Self::E, filter: &Self::Filter) -> bool {
filter.apply(entry.is_deleted())
}
lx marked this conversation as resolved Outdated
lx commented 2021-03-27 14:18:46 +00:00
Outdated
Review
Copy link

did you remove the default empty implementation in the trait? was it a bad practice to have it?

did you remove the default empty implementation in the trait? was it a bad practice to have it?
trinity-1686a commented 2021-04-06 01:34:32 +00:00
Outdated
Review
Copy link

I would not necessarily call it bad practice.
It conveyed to me that "doing nothing" is a good default behavior, and while doing nothing might be a valid behavior, I think wanting to do nothing should be rather explicit.

I would not necessarily call it bad practice. It conveyed to me that "doing nothing" is a good default behavior, and while doing nothing might be a valid behavior, I think wanting to do nothing should be rather explicit.
lx commented 2021-04-06 14:15:51 +00:00
Outdated
Review
Copy link

(I answererd below)

(I answererd below)

4
src/model/key_table.rs
View file

@ -125,10 +125,6 @@ impl TableSchema for KeyTable {
type E = Key;
type Filter = KeyFilter;
fn updated(&self, _old: Option<Self::E>, _new: Option<Self::E>) {
// nothing to do when updated
}
fn matches_filter(entry: &Self::E, filter: &Self::Filter) -> bool {
match filter {
KeyFilter::Deleted(df) => df.apply(entry.deleted.get()),

4
src/rpc/membership.rs
View file

@ -417,8 +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
// we need to increment failure counter as call was done using by_addr so the
// counter was not auto-incremented
lx commented 2021-04-06 14:31:52 +00:00
Outdated
Review
Copy link

Actually no because the increment in rpc_client.rs is done by the RpcClient and not the RpcAddrClient, but here we are using the RpcAddrClient. (we could put this in a comment to make it clearer)

Actually no because the increment in `rpc_client.rs` is done by the `RpcClient` and not the `RpcAddrClient`, but here we are using the `RpcAddrClient`. (we could put this in a comment to make it clearer)
st.num_failures.fetch_add(1, Ordering::SeqCst);
if !st.is_up() {
warn!("Node {:?} seems to be down.", id);

2
src/rpc/ring.rs
View file

@ -28,7 +28,7 @@ const PARTITION_MASK_U16: u16 = ((1 << PARTITION_BITS) - 1) << (16 - PARTITION_B
/// The maximum number of time an object might get replicated
pub const MAX_REPLICATION: usize = 3;
lx commented 2021-03-27 14:44:19 +00:00
Outdated
Review
Copy link

Not really, a node can be known in that it answers ping messages, and not be configured if it is idle or draining pending removal by admins.

Not really, a node can be known in that it answers ping messages, and not be configured if it is idle or draining pending removal by admins.
trinity-1686a commented 2021-04-06 02:33:25 +00:00
Outdated
Review
Copy link

would "available" be correct instead of known?

would "available" be correct instead of known?
lx commented 2021-04-06 14:11:47 +00:00
Outdated
Review
Copy link

Just put a TODO in the comment and I'll write it later

Just put a TODO in the comment and I'll write it later
lx commented 2021-04-06 14:37:22 +00:00
Outdated
Review
Copy link

Suggestion: The user-defined configuration of the cluster's nodes

Suggestion: `The user-defined configuration of the cluster's nodes`
/// The versionned configurations of all nodes known in the network
/// The user-defined configuration of the cluster's nodes
#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct NetworkConfig {
/// Map of each node's id to it's configuration

2
src/table/schema.rs
View file

@ -76,7 +76,7 @@ pub trait TableSchema: Send + Sync {
// as the update itself is an unchangeable fact that will never go back
// due to CRDT logic. Typically errors in propagation of info should be logged
// to stderr.
fn updated(&self, old: Option<Self::E>, new: Option<Self::E>);
fn updated(&self, _old: Option<Self::E>, _new: Option<Self::E>) {}
lx commented 2021-03-27 14:57:52 +00:00
Outdated
Review
Copy link

Why remove empty implementation?

Why remove empty implementation?
trinity-1686a commented 2021-04-06 03:22:23 +00:00
Outdated
Review
Copy link

As explained above, I feel like a default implementation should be what we want in most case. Of 5 implementations of this trait, only 2 actually use an empty updated, so being explicit about it seems to me like a better option.

As explained above, I feel like a default implementation should be what we want in most case. Of 5 implementations of this trait, only 2 actually use an empty `updated`, so being explicit about it seems to me like a better option.
lx commented 2021-04-06 14:10:53 +00:00
Outdated
Review
Copy link

Idk, to me the reasonning was that updated() is implemented when we need to add logic to do something when an entry changes, but if we don't need to add anything we shouldn't have to write an empty updated() handler. I don't see how having an empty default implementation can cause much confusion as it does literally nothing.

Idk, to me the reasonning was that `updated()` is implemented when we need to add logic to do something when an entry changes, but if we don't need to add anything we shouldn't have to write an empty `updated()` handler. I don't see how having an empty default implementation can cause much confusion as it does literally nothing.
fn matches_filter(entry: &Self::E, filter: &Self::Filter) -> bool;
}