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

Named pins & pins stored in datastore #4757

Closed
wants to merge 1 commit into from

Conversation

Voker57
Copy link
Contributor

@Voker57 Voker57 commented Mar 2, 2018

Problems of current pin implementation

  • Pins are stored in elaborate way in blockstore, enabling whole world to sniff your pins and introducing great deal of complexity to code like internal pins and global pin locking
  • Pins are unnamed, so you can't structure/review them easily

This PR:

  • Stores all pins in datastore as /local/pins/[pin type]/[pin name] entries, allowing fast & global lock -free pin operations
  • Adds prefixed searching to pin ls
  • Adds default prefixes to some pinning operations (like pin add)

Example:

% ipfs add <<< "Hello"
added QmY9cxiHqTFoWamkQVkpmmqzBrY3hCBEL2XNu3NtX74Fuu QmY9cxiHqTFoWamkQVkpmmqzBrY3hCBEL2XNu3NtX74Fuu                                                                
% ipfs add --pin=false <<< "Hello, don't pin me" 
added QmS6yrQ9ZNYkasM28o6E95p6j3Fs1UQm7MnevgfmeMWo3S QmS6yrQ9ZNYkasM28o6E95p6j3Fs1UQm7MnevgfmeMWo3S                                                                
% ipfs pin add testpin QmS6yrQ9ZNYkasM28o6E95p6j3Fs1UQm7MnevgfmeMWo3S
pinned QmS6yrQ9ZNYkasM28o6E95p6j3Fs1UQm7MnevgfmeMWo3S recursively
% ./ipfs pin ls --type=recursive                                       
QmY9cxiHqTFoWamkQVkpmmqzBrY3hCBEL2XNu3NtX74Fuu added/QmY9cxiHqTFoWamkQVkpmmqzBrY3hCBEL2XNu3NtX74Fuu recursive
QmS6yrQ9ZNYkasM28o6E95p6j3Fs1UQm7MnevgfmeMWo3S testpin recursive
% ./ipfs pin ls -r added --type=recursive 
QmY9cxiHqTFoWamkQVkpmmqzBrY3hCBEL2XNu3NtX74Fuu added/QmY9cxiHqTFoWamkQVkpmmqzBrY3hCBEL2XNu3NtX74Fuu recursive

TODO:

  • tests
  • adapt rest of commands & coreapi
  • code cleanup
  • check for locking required
  • migration

Includes #3600, probably addresses #4717, solves #4586

@whyrusleeping
Copy link
Member

This is the sort of change that needs serious discussion in an issue before it gets implemented. Yes, we want to move away from the current pinning system, but we need to come to a consensus on how to do that before doing it.

Could you open a new issue to discuss how to go about doing this? We've already brainstormed several different ideas in different issues across the repo, but we should have a discussion all in one place to really work this out.

In any case, this sort of change will need a repo migration written for it, and thorough testing.

@Kubuxu
Copy link
Member

Kubuxu commented Mar 4, 2018

It also needs coordination with JS team. I would say it is great example of a change that should go through RFC process (see ipfs-inactive/RFC#1 and https://github.com/ipfs/RFC/. It will be slower but slower is better in this case.

Nevertheless thank you very much @Voker57 for writing this code. If we decide to go with RFC for it, it is great to have working example to evaluate complexities, performance and so on.

@Kubuxu Kubuxu added status/WIP This a Work In Progress need/community-input Needs input from the wider community and removed status/WIP This a Work In Progress labels Mar 9, 2018
@daviddias
Copy link
Member

daviddias commented Mar 15, 2018

Is the datastore kv really our best option to store a very long list of hashes? Using IPLD might have a better payoff in the long term. For example, we would have more flexibility in the data structure/index scheme we use in the future & we will be able to use the same diffing and syncing tools we have to write for all the other IPLD graphs.

Super in favor of @Kubuxu proposal to do this as an RFC ❤️

@Kubuxu
Copy link
Member

Kubuxu commented Mar 15, 2018

Let's move the discussion to: #4763

@Voker57
Copy link
Contributor Author

Voker57 commented Sep 9, 2018

I'm stumped on how to make the migration work, help would be appreciated.

Otherwise, I consider this PR finished.

@Voker57 Voker57 changed the title [WIP] Named pins & pins stored in datastore Named pins & pins stored in datastore Sep 9, 2018
@Voker57 Voker57 force-pushed the feat/named-pins branch 2 times, most recently from c349ded to db8de85 Compare October 10, 2018 10:28
@Voker57 Voker57 force-pushed the feat/named-pins branch 2 times, most recently from f31d621 to 44d4e6e Compare January 10, 2019 10:15
@Voker57 Voker57 force-pushed the feat/named-pins branch 2 times, most recently from fcf261a to 8366884 Compare June 6, 2019 21:24
Voker57 pushed a commit to Voker57/interface-go-ipfs-core that referenced this pull request Jun 6, 2019
@Voker57 Voker57 force-pushed the feat/named-pins branch 2 times, most recently from 68c114f to 2aef549 Compare June 6, 2019 21:30
@bqv
Copy link

bqv commented Aug 15, 2020

I know this is 2 years old, but is it still viable and on track for merging?

Voker57 added a commit to Voker57/go-ipfs-pinner that referenced this pull request Sep 19, 2020
Voker57 added a commit to Voker57/go-ipfs-pinner that referenced this pull request Sep 19, 2020
Voker57 added a commit to Voker57/interface-go-ipfs-core that referenced this pull request Sep 19, 2020
* Change pin structure to a tree stored in KV store
* Adjust pin-related commands accordingly
* Make pinning routines use meaningful default prefixes

License: MIT
Signed-off-by: Iaroslav Gridin <voker57@gmail.com>
@Voker57
Copy link
Contributor Author

Voker57 commented Sep 19, 2020

didn't care to fix some tests, but patch is rebased and seems to work

@aschmahmann
Copy link
Contributor

aschmahmann commented Sep 21, 2020

@bqv @Voker57 as some of the other high priority work in go-ipfs has been completed moving pins into the datastore (instead of in a DAG in the blockstore) is back on the radar (although quarterly priorities aren't finalized yet so no guarantees 😄).

I haven't had a chance to look through this particular PR yet, although I suspect we may still need some discussion on whether the "index" should be the pin "name", multihash, CID, or something else. For what it's worth js-ipfs went with using the multihash as the index (see the PR linked above). We also definitely need to implement the migration.

When a new issue is opened, or a design review call is scheduled to work on some of the details here I'll post back.

@akavel
Copy link

akavel commented Nov 12, 2020

It looks like js-ipfs did this change (ipfs/js-ipfs#2771) and it brought them a huge performance improvement in pinning speed, making js-ipfs literally order(s) of magnitude faster than go-ipfs at pinning. Could that performance improvement be co-opted by go-ipfs (e.g. by merging this PR), and IIUC hopefully thus improve situation on/resolve #5221?

@S3bb1
Copy link

S3bb1 commented Nov 12, 2020

It looks like js-ipfs did this change (ipfs/js-ipfs#2771) and it brought them a huge performance improvement in pinning speed, making js-ipfs literally order(s) of magnitude faster than go-ipfs at pinning. Could that performance improvement be co-opted by go-ipfs (e.g. by merging this PR), and IIUC hopefully thus improve situation on/resolve #5221?

maybe this PR also adresses this?
#7750

@akavel
Copy link

akavel commented Nov 12, 2020

@S3bb1 Hm, the #7750's actual implementation seems to include a commit from @aschmahmann, and be developed in a branch of go-ipfs-pinner repo, so my guess is it might be something at least partially done by the team, which makes me cautiously optimistic it may actually land in some finite timeframe 🤞😌

@aschmahmann
Copy link
Contributor

Yep @akavel the plan is to land putting pins in the datastore quite soon (v0.8.0) along with the remote pinning api (which supports named pins), and then we can come back to adding a new pin cli+http api to handle 1) named pins 2) multiple pins for the same CID (e.g. I can pin "Othello" and "Best Play" to the same CID, then if I remove or change the "Best Play" pin I don't unpin "Othello" by accident).

@Stebalien
Copy link
Member

This has landed.

@Stebalien Stebalien closed this May 10, 2021
@aschmahmann
Copy link
Contributor

This has landed.

Clarifying that "this" means the v0.8.0 things regarding the datastore. A local pinning CLI with named pins is not done yet.

@Voker57
Copy link
Contributor Author

Voker57 commented May 13, 2021

This has landed.

Could you link to what exactly has landed?

@Stebalien
Copy link
Member

Stebalien commented May 13, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/community-input Needs input from the wider community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants