-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
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. |
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. |
Is the Super in favor of @Kubuxu proposal to do this as an RFC ❤️ |
Let's move the discussion to: #4763 |
9536f12
to
ba746d1
Compare
942bd58
to
7ceb35f
Compare
7935595
to
9297a6c
Compare
959a49a
to
043df06
Compare
I'm stumped on how to make the migration work, help would be appreciated. Otherwise, I consider this PR finished. |
c349ded
to
db8de85
Compare
db8de85
to
2ea3600
Compare
f31d621
to
44d4e6e
Compare
fcf261a
to
8366884
Compare
68c114f
to
2aef549
Compare
I know this is 2 years old, but is it still viable and on track for merging? |
* 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 <[email protected]>
2aef549
to
48877ca
Compare
didn't care to fix some tests, but patch is rebased and seems to work |
@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. |
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? |
@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 🤞😌 |
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). |
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. |
Could you link to what exactly has landed? |
Sorry, "this" is ipfs/go-ipfs-pinner#4.
- Each pin is stored as a separate key in the datastore.
- Each pin can carry a name (optional).
- CIDs can be pinned multiple times with different names.
- Metadata can be extended at any time (IPLD/CBOR).
|
Problems of current pin implementation
This PR:
Example:
TODO:
Includes #3600, probably addresses #4717, solves #4586