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

support --nocopy when adding URLs #735

Merged
merged 1 commit into from Mar 27, 2019
Merged

support --nocopy when adding URLs #735

merged 1 commit into from Mar 27, 2019

Conversation

jmank88
Copy link
Contributor

@jmank88 jmank88 commented Mar 26, 2019

ipfs/kubo#6065 added URL support to the add command, along with --nocopy as well, replacing the need for the experimental urlstore. This PR proposes supporting ipfs-cluster-ctl add --nocopy <url>.

@@ -349,6 +349,10 @@ cluster "pin add".
Value: defaultAddParams.ReplicationFactorMax,
Usage: "Sets the maximum replication factor for pinning this file",
},
cli.BoolFlag{
Name: "nocopy",
Usage: "Add the URL using filestore. Implies raw-leaves. (experimental)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't it add to the urlstore? I've had some problems figuring out how this is now working in ipfs (I haven' t been able to find the place where the URL is taken from the multipart request and used for the urlstore - I suspect it's somewhere in unixfs), but if urlstore is not enabled, adding urls like this on ipfs fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WebFile now implements FileInfo, and returns the URL via FileInfo.AbsPath(), which is why the upgrade to go-ipfs-files v0.0.2 is required. This is passed along via the abspath header, which ends up in the ReaderFile. It is then extracted and set as DagBuilderParams.fullpath https://github.com/ipfs/go-unixfs/blob/master/importer/helpers/dagbuilder.go#L85-L88, which in combination with the nocopy flag, triggers the same underlying mechanism as urlstore, but urlstore is not directly used. Only filestore is required https://github.com/ipfs/go-ipfs/blob/master/core/coreapi/unixfs.go#L52-L54. Here is the read logic: https://github.com/ipfs/go-ipfs/blob/master/filestore/fsrefstore.go#L138

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, enabling urlstore makes the command available, but that command just leverages filestore internals to actually do the work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, that's very useful. I have opened ipfs/kubo#6139

Copy link
Collaborator

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

I wonder, when adding a file like this, is the DAG builder outputting a single special unixfs block (symlink)? This is my intuition but I haven't verified it.

I don't see issues with this PR (thanks for submitting). Having a test would be great (adding a web file from a url with the api/rest/client and then checking that only a single symlink block has been generated and put on ipfs. But it's probably more complicated than the PR itself and I'm happing merging now if we're sure this works.

@jmank88
Copy link
Contributor Author

jmank88 commented Mar 27, 2019

I wonder, when adding a file like this, is the DAG builder outputting a single special unixfs block (symlink)? This is my intuition but I haven't verified it.

Not sure I fully grok your terminology, but it creates a FilestoreNode whose PosInfo.FullPath contains the URL, then checks for URLs on reads.

@hsanjuan
Copy link
Collaborator

I wonder, when adding a file like this, is the DAG builder outputting a single special unixfs block (symlink)? This is my intuition but I haven't verified it.

Not sure I fully grok your terminology, but it creates a FilestoreNode whose PosInfo.FullPath contains the URL, then checks for URLs on reads.

Ah right, I understand now. Adding will chunk and build the resulting DAG blocks in the same way, but those blocks are filestore nodes, and instead of having the data payload they have a link to source+offset. Thanks!

@hsanjuan hsanjuan merged commit 94a781e into ipfs-cluster:master Mar 27, 2019
@hsanjuan hsanjuan mentioned this pull request Mar 27, 2019
@jmank88 jmank88 deleted the url-nocopy branch March 27, 2019 14:37
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.

None yet

2 participants