Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified BlockStore trait #257

Merged
merged 8 commits into from
May 22, 2023
Merged

Conversation

organizedgrime
Copy link
Contributor

@organizedgrime organizedgrime commented May 5, 2023

Here, many changes have been made, but all within the confines of BlockStores and how they operate.

BlockStores are no longer passed around mutably. Instead, an individual implementation of a given BlockStore will use RefCells, Locks, Cows, etc. to achieve interior mutability in cases where it is required. This changes the signature and lifetimes for many functions, including the put_block and get_block functions required by the trait itself.

@organizedgrime organizedgrime requested a review from a team as a code owner May 5, 2023 18:35
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #257 (6988855) into main (020aac2) will decrease coverage by 2.81%.
The diff coverage is 21.19%.

❗ Current head 6988855 differs from pull request most recent head 2bdb463. Consider uploading reports for the commit 2bdb463 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #257      +/-   ##
==========================================
- Coverage   60.06%   57.26%   -2.81%     
==========================================
  Files          38       40       +2     
  Lines        2697     2885     +188     
  Branches      666      712      +46     
==========================================
+ Hits         1620     1652      +32     
- Misses        624      769     +145     
- Partials      453      464      +11     
Impacted Files Coverage Δ
wnfs-common/src/blockstore/carblockstore.rs 0.00% <0.00%> (ø)
wnfs-common/src/blockstore/diskblockstore.rs 0.00% <0.00%> (ø)
...ommon/src/blockstore/threadsafememoryblockstore.rs 0.00% <0.00%> (ø)
wnfs-hamt/src/hamt.rs 20.58% <0.00%> (ø)
wnfs-hamt/src/node.rs 55.03% <0.00%> (ø)
wnfs-hamt/src/pointer.rs 30.00% <0.00%> (ø)
wnfs/examples/private.rs 0.00% <0.00%> (ø)
wnfs/examples/tiered_blockstores.rs 0.00% <0.00%> (ø)
wnfs/src/private/link.rs 47.29% <ø> (-1.36%) ⬇️
wnfs/src/private/node/header.rs 86.66% <0.00%> (+1.18%) ⬆️
... and 17 more

... and 8 files with indirect coverage changes

Copy link
Member

@appcypher appcypher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another great PR. Thank you for creating this.
We've wanted change the ref mut blockstore params to ref so this partly fixes #251.
I have some comments on the PR. They are mostly nits.

wnfs-common/src/blockstore/mod.rs Outdated Show resolved Hide resolved
wnfs-common/src/blockstore/mod.rs Outdated Show resolved Hide resolved
wnfs-common/src/blockstore/mod.rs Outdated Show resolved Hide resolved
wnfs/Cargo.toml Outdated Show resolved Hide resolved
wnfs-common/Cargo.toml Outdated Show resolved Hide resolved
wnfs-common/src/blockstore/threadsafememoryblockstore.rs Outdated Show resolved Hide resolved
wnfs-common/src/blockstore/diskblockstore.rs Outdated Show resolved Hide resolved
wnfs-common/src/blockstore/threadsafememoryblockstore.rs Outdated Show resolved Hide resolved
wnfs-common/src/blockstore/diskblockstore.rs Outdated Show resolved Hide resolved
wnfs-common/src/blockstore/carblockstore.rs Outdated Show resolved Hide resolved
Copy link
Member

@appcypher appcypher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another great PR. Thank you for creating this.
We've wanted change the ref mut blockstore params to ref so this partly fixes #251.
I have some comments on the PR. They are mostly nits.

Signed-off-by: Vera Gonzalez <[email protected]>
@matheus23
Copy link
Member

Thanks for this Vera!

The BlockStore trait adjustment seems good :)
create_cid looks fine to me for now. But for reference and because it's interesting, other blockstore trait implementations go even further: They split out a function put_keyed which takes the "pre-baked" Cid as a parameter.


About where the blockstore implementations belong: IMO it makes most sense to put them closer to your data prep tool, outside of rs-wnfs.
We want to keep rs-wnfs' dependencies pretty slim, so we can keep compiling it to wasm (where we'd need different block store implementations). That's also the reason we've kept the blockstore implementations we are using outside of rs-wnfs (except for the one used for tests).
Blockstores are actually a more abstract concept that doesn't have to be used to store wnfs data. The only reason we came up with our own trait is that there wasn't an async variant of a blockstore trait out there yet. The plan is to eventually move the trait outside of rs-wnfs into a common lib that we & e.g. noosphere are using.

Signed-off-by: Vera Gonzalez <[email protected]>
Co-Authored-By: Stephen Akinyemi <[email protected]>
@organizedgrime organizedgrime changed the title Modified BlockStore trait; implemented new BlockStores Modified BlockStore trait May 18, 2023
Copy link
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small nits remaining, but otherwise GTG!


Hm. This PR has merge conflicts now that #265 landed. I know it can be annoying (to have to repeatedly re-rebase a refactoring PR while smaller PRs are coming in). If you want to, we can take over rebasing @organizedgrime, just let us know (and enable the "allow edits from maintainers" thingie on this PR unless you already have).

wnfs-common/src/blockstore.rs Outdated Show resolved Hide resolved
wnfs/Cargo.toml Outdated Show resolved Hide resolved
wnfs-common/src/blockstore.rs Outdated Show resolved Hide resolved
wnfs-common/Cargo.toml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants