Adds basic support for pin and unpin operations #117

saresend · 2020-03-23T18:38:34Z

What

Should address #11, by adding some basic pin / unpin operations to the dag

Todo

Add support for querying whether a block is pinned
Add Tests once design is finished

aphelionz · 2020-03-23T19:17:18Z

This is really exciting, thank you @saresend. If this works for @koivunej then it'd be great to see tests, and then if you want to go for the gold, add the http endpoints! 🎉

dvc94ch · 2020-03-23T21:46:47Z

I guess remove should check if the block is pinned.

…check when upinning

saresend · 2020-03-24T16:01:54Z

Alright, a couple of changes:

First, pinning is defined by the existence of a pair in the Pin hashmap, rather than a boolean flag of 1 or 0. This way we don't have two False states, in the form of Null or Zero, and instead consolidate all into the null case.
It should now also error when trying to unpin an unpinned block, rather than just silently performing a no-op.

aphelionz · 2020-03-24T21:10:36Z

@saresend Here's the overall spec, including what the conformance tests expect for the /pin APIs. I hope this is helpful. Let me know if you have any questions

rs-ipfs/ipfs-rust-conformance#2

Edit: /pin APIs

dvc94ch

I assume you're done with this? Could you please rebase your branch?

saresend · 2020-03-25T19:46:57Z

I was going to add some tests at the very least. I might leave the actual http endpoints to a separate PR however. If you have some code points for how you write tests for this project, I'd be happy to add them.

aphelionz · 2020-03-25T19:49:33Z

@saresend @dvc94ch Tests and doc would be important. You can add simple module tests at the end of the respective files, e.g. dag.rs and repo/mod.rs. It would also be huge if you could do the endpoints. There's more examples of testing in that crate as well.

Thank you so much again for this.

saresend · 2020-03-25T20:01:57Z

No problem, I'll open up a new PR for the http endpoints, just to keep concerns separate!

dvc94ch · 2020-03-25T20:02:56Z

It's also not clear why you're exposing it on the dag, why unpin should error if a block already is unpinned, as stuff may share blocks or how this works as you can still remove a block even if it's pinned.

saresend · 2020-03-25T20:11:48Z

Totally fair, I'm still ramping up on IPFS and don't have great context on how you envision this being architected or how concerns are being separated. If you have suggestions on better ways to expose the API I'm all ears. My understanding of pinning is that it protects against garbage collection, and I wasn't able to find whether garbage collection is already being done in rust-ipfs. My thought was that garbage collection could leverage the pinning query logic when removing nodes whenever it does get built

aphelionz · 2020-03-25T20:14:12Z

As far as the error goes, it should simply follow what js-ipfs or go-ipfs does. So if they error when trying to unpin and already unpinned block, then we should too. Otherwise not.

dvc94ch · 2020-03-25T20:18:49Z

I don't think the dag should expose pin/unpin, and if it did then the semantics would probably not be to pin the root of a path as a path can point to other blocks, so you'd want to pin the last block it refers to.

Yes pin is for garbage collection, which is not currently implemented. What currently is implemented is manually removing blocks, which shouldn't happen if it's pinned.

Since blocks can be shared, pinning should probably increment a counter and unpin should decrement it, instead of having a boolean.

Garbage collection can be implemented by iterating through all blocks, checking if the pin count is zero and if it is remove the block.

saresend · 2020-03-25T20:28:18Z

Where would you recommend pinning be implemented?

dvc94ch · 2020-03-25T20:32:40Z

src/lib.rs

@@ -312,6 +312,21 @@ impl<Types: IpfsTypes> Ipfs<Types> {
        Ok(self.repo.remove_block(cid).await?)
    }

+    /// Pins a given Cid
+    pub async fn pin_block(&self, path: IpfsPath) -> Result<(), Error> {


Suggested change

pub async fn pin_block(&self, path: IpfsPath) -> Result<(), Error> {

pub async fn pin_block(&self, cid: &Cid) -> Result<(), Error> {

self.repo.pin_block(cid).await

}

aphelionz · 2020-03-25T22:16:37Z

I might suggest we focus on tests and the HTTP endpoints here, so that at least there's a foundation for architecture changes + refactors. Always room for improvement, especially if we keep getting awesome community contributions.

dvc94ch · 2020-03-25T22:21:56Z

Do you have anything to add in regards to my comment? Did I not explain in which ways the implementation suggested here is flawed clearly enough? Conformance tests are not a way to avoid using your brain, as tests are just as buggy as code. Besides tests do not ensure correctness, they sometimes show the presence of bugs.

dvc94ch · 2020-03-25T22:40:46Z

Let me try again, I'll use letters to denote blocks in the following diagram:

a -> b <- c

and these two concurrent processes running in parallel

process 1:
    put(b)
    pin(b)
    put(a)
    pin(a)
    ...
    unpin(a)
    unpin(b)
    gc()

process 2:
    put(b)
    pin(b)
    put(c)
    pin(c)

    get(b)???

dvc94ch · 2020-03-25T22:50:20Z

Also put should probably pin automatically to prevent a race, but that's something to be discussed, if someone can supply a usecase where you don't want to put_and_pin

dvc94ch · 2020-03-25T22:56:31Z

I approved the PR and would have merged it if it didn't have conflicts with master. But I agree with @aphelionz, this features flaws should be extensively documented and tested.

saresend · 2020-03-26T00:27:58Z

Before the next couple changes, I just want to clarify what you'd want for this PR. So far, the changes requested are:

Move to use &Cid, rather than ipfsPath
Automatically Pin on Put actions
Prevent removal based on whether a block is pinned
Add tests and documentation

Is there anything I'm missing here?

dvc94ch · 2020-03-26T00:34:35Z

If we didn't have the conformance tests I'd suggest removing remove altogether. Also don't use a bool as shown in the example above it can lead to issues. But mainly I'm a bit annoyed, which has nothing to do with your pr. Thanks for contributing...

saresend · 2020-03-27T01:35:10Z

@dvc94ch @aphelionz, alright this should include a more RC like logic for pinning, removes it from the Dag API, and finally includes a test to verify behavior. It should also now be rebased so if it all checks out feel free to merge.

dvc94ch · 2020-03-27T12:58:17Z

Looks good! So I think this is much better, thanks. Can you rebase again, looks like it's not mergeable.

saresend · 2020-03-27T15:21:42Z

Alright that should be merged!

koivunej · 2020-03-27T16:50:01Z

Interesting the same test is flaky on windows ... Again. Need to investigate why.

koivunej

Looks good!

koivunej · 2020-03-27T17:12:25Z

The test failure will be fixed with #124 but I think this is now good to merge?

aphelionz · 2020-04-01T16:40:11Z

@saresend Please add yourself to rs-ipfs/welcome#1 and also let us know if you plan on doing the endpoints as well

saresend · 2020-04-01T18:28:41Z

Yep! I plan to have a PR up in the next week or so, classes have started back up again for me so it may take me a bit longer unfortunately

aphelionz · 2020-04-01T18:40:33Z

That's ok, no rush.

saresend added 6 commits March 23, 2020 11:10

Adding partial stups to support pin/unpin

3ad1494

fixing edits

3c11bcb

reformatting repo/mod.rs

dee4e1c

punching datastore functions through to API

d6e159c

exposing properly on API side

fe2a51b

augments datastore to include pinnable data

fe6ab04

saresend mentioned this pull request Mar 23, 2020

Pinning support #11

Closed

fixing clippy lints

c9dec62

pushed through a query function and unpinning to top level, and adds …

94a1c94

…check when upinning

dvc94ch approved these changes Mar 25, 2020

View reviewed changes

dvc94ch reviewed Mar 25, 2020

View reviewed changes

saresend added 6 commits March 26, 2020 17:49

Merge branch 'master' of https://github.com/ipfs-rust/rust-ipfs

5e2a7d5

removes api from dag, operates directly on the repo

8f1349d

moving to use RC-like logic

e95bd89

removing extraneous logic

9a7215f

adds test for pin and unpin

42c9d7a

formatting

344a3aa

merging saresend/rust-ipfs to master ipfs-rust/rust-ipfs

5ac059f

koivunej approved these changes Mar 27, 2020

View reviewed changes

koivunej merged commit 0d214dc into rs-ipfs:master Mar 27, 2020

koivunej mentioned this pull request Mar 30, 2020

Support for /block endpoints #90

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds basic support for pin and unpin operations #117

Adds basic support for pin and unpin operations #117

saresend commented Mar 23, 2020 •

edited

Loading

aphelionz commented Mar 23, 2020

dvc94ch commented Mar 23, 2020

saresend commented Mar 24, 2020

aphelionz commented Mar 24, 2020 •

edited

Loading

dvc94ch left a comment •

edited

Loading

saresend commented Mar 25, 2020

aphelionz commented Mar 25, 2020

saresend commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

saresend commented Mar 25, 2020

aphelionz commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

saresend commented Mar 25, 2020

dvc94ch Mar 25, 2020

aphelionz commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

saresend commented Mar 26, 2020

dvc94ch commented Mar 26, 2020 •

edited

Loading

saresend commented Mar 27, 2020

dvc94ch commented Mar 27, 2020

saresend commented Mar 27, 2020

koivunej commented Mar 27, 2020

koivunej left a comment

koivunej commented Mar 27, 2020

aphelionz commented Apr 1, 2020

saresend commented Apr 1, 2020

aphelionz commented Apr 1, 2020

-    pub async fn pin_block(&self, path: IpfsPath) -> Result<(), Error> {
+    pub async fn pin_block(&self, cid: &Cid) -> Result<(), Error> {
+        self.repo.pin_block(cid).await
+    }

Adds basic support for pin and unpin operations #117

Adds basic support for pin and unpin operations #117

Conversation

saresend commented Mar 23, 2020 • edited Loading

What

Todo

aphelionz commented Mar 23, 2020

dvc94ch commented Mar 23, 2020

saresend commented Mar 24, 2020

aphelionz commented Mar 24, 2020 • edited Loading

dvc94ch left a comment • edited Loading

Choose a reason for hiding this comment

saresend commented Mar 25, 2020

aphelionz commented Mar 25, 2020

saresend commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

saresend commented Mar 25, 2020

aphelionz commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

saresend commented Mar 25, 2020

dvc94ch Mar 25, 2020

Choose a reason for hiding this comment

aphelionz commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

dvc94ch commented Mar 25, 2020

saresend commented Mar 26, 2020

dvc94ch commented Mar 26, 2020 • edited Loading

saresend commented Mar 27, 2020

dvc94ch commented Mar 27, 2020

saresend commented Mar 27, 2020

koivunej commented Mar 27, 2020

koivunej left a comment

Choose a reason for hiding this comment

koivunej commented Mar 27, 2020

aphelionz commented Apr 1, 2020

saresend commented Apr 1, 2020

aphelionz commented Apr 1, 2020

saresend commented Mar 23, 2020 •

edited

Loading

aphelionz commented Mar 24, 2020 •

edited

Loading

dvc94ch left a comment •

edited

Loading

dvc94ch commented Mar 26, 2020 •

edited

Loading