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

block put --pin #5969

Merged
merged 1 commit into from
Feb 6, 2019
Merged

block put --pin #5969

merged 1 commit into from
Feb 6, 2019

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Feb 4, 2019

License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k magik6k requested a review from Kubuxu as a code owner February 4, 2019 17:06
@ghost ghost assigned magik6k Feb 4, 2019
@ghost ghost added the status/in-progress In progress label Feb 4, 2019
@alanshaw
Copy link
Member

alanshaw commented Feb 4, 2019

@magik6k could I get some background info on why this change?

Is this default false for backwards compatibility? Should we consider adding a pin option and default it to true for all operations that add data to IPFS? Including for example dag.put?

@magik6k
Copy link
Member Author

magik6k commented Feb 4, 2019

go-ipfs-http-client implements DAGService using block calls (js-ipfs-http-client does the same thing), and also needs to implement a pinning node adder:
https://github.com/ipfs/go-ipfs/blob/803e9a7e6608468616e0379cafd94f0abe96da51/core/coreapi/interface/dag.go#L7-L13

Calls to this adder automatically pin related nodes, and because go-ipfs-http-client adds nodes through block calls, this is also where pinning needs to happen (because we need to hold a pinning lock)

Is this default false for backwards compatibility?

Defaulting to false is mostly for backwards compatibility (users may not be aware that adding objects this way pins them, and currently our pinning system isn't happy about many small pins)

Should we consider adding a pin option [...] for all operations that add data to IPFS?

That would be nice for consistency, but I feel like there should be a better way

default it to true for all operations

Current pinning system really struggles with many individual pins, I'd say not before we get a more scalable pinning system.

for example dag.put

In go-ipfs dag.put already has a pin option - https://github.com/ipfs/go-ipfs/blob/master/core/commands/dag/dag.go#L65, which defaults to false too.

@Stebalien
Copy link
Member

Waiting on a final signoff by @alanshaw.

@magik6k magik6k added RFM and removed need/review Needs a review labels Feb 6, 2019
@Stebalien Stebalien merged commit e11b93b into master Feb 6, 2019
@ghost ghost removed the status/in-progress In progress label Feb 6, 2019
@Stebalien Stebalien deleted the feat/block-pin branch February 6, 2019 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants