-
-
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
Add better support for Raw Nodes in MFS and elsewhere #3996
Conversation
b92e53b
to
30c7730
Compare
Towards #3989 |
Note this is a work in progress. In particular the Size() method in DagModifier needs to implemented to support raw nodes with MFS. I am trying to figure out if it is possible to get the equivalent of Filesize from the generic Node interface. |
30c7730
to
22e2f5a
Compare
Okay this is no longer a work in progress. Fully generalize this to allow any type of node to be stored in the MFS tree should probably me another P.R. |
Could you add test cases from this PR: https://github.com/ipfs/go-ipfs/pull/3901/files This makes sure that we test as much with RawNodes as we do test with normal ones. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, i would like to see another test in dagmodifier_test.go that tests dagmodifier.Size() on a raw node though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks good to me (except as noted). However, I'm not very familiar with MFS.
As an aside, as you appear to be reasonably familiar with this subsystem, please consider documenting functions as you modify them (e..g, Size
could mean "size of this node" but it looks like it means "file size" (number of user-specified bytes stored)).
unixfs/mod/dagmodifier.go
Outdated
case *mdag.ProtoNode: | ||
root = nd | ||
case *mdag.RawNode: | ||
// TODO: be able to append to rawnodes. Probably requires making this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, you're appending a raw node to dm.curNode
which may or may not be raw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair point. the comment and error are wrong then, maybe we can actually support this part fine now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, no; I'm totally wrong. All callers pass dm.currentNode
. Now, as for why this function even takes a node...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whyrusleeping I just removed your comment. If you wish to readd something please give me the correct text to use.
unixfs/mod/dagmodifier.go
Outdated
func (dm *DagModifier) appendData(node *mdag.ProtoNode, spl chunk.Splitter) (node.Node, error) { | ||
func (dm *DagModifier) appendData(nd node.Node, spl chunk.Splitter) (node.Node, error) { | ||
|
||
var root *mdag.ProtoNode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling this root
is misleading (it's the node to-be-appended).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not my change. @whyrusleeping care to comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, root is probably not the best name here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored that method to avoid the problem.
unixfs/mod/dagmodifier.go
Outdated
dbp := &help.DagBuilderParams{ | ||
Dagserv: dm.dagserv, | ||
Maxlinks: help.DefaultLinksPerBlock, | ||
} | ||
|
||
return trickle.TrickleAppend(dm.ctx, node, dbp.New(spl)) | ||
return trickle.TrickleAppend(dm.ctx, root, dbp.New(spl)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error in TrickleAppend should probably be changed to ErrNoRawYet
(or dm.curNode
should be checked here).
I thought the test I added where more comprehensive but I didn't see what you will trying to do. My added tests conflict with your but will make sure mine are just as comprehensive. |
test/sharness/t0250-files-api.sh
Outdated
@@ -15,6 +15,7 @@ test_expect_success "can create some files for testing" ' | |||
FILE1=$(echo foo | ipfs add -q) && | |||
FILE2=$(echo bar | ipfs add -q) && | |||
FILE3=$(echo baz | ipfs add -q) && | |||
FILE9=$(echo zip | ipfs add -q --raw-leaves) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is different here than in the rest of the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sorry about that. Tab vs space issue.
6586737
to
0ab50f8
Compare
@Kubuxu I redid the tests so they are closer to what you had Raw nodes are now working in the files API as far as I can tell. |
It was non-obvious how to do this so I instead tested that the size was reported correctly in a sharness test. |
0ab50f8
to
7eef45c
Compare
@kevina The current dagmodifier tests use: https://github.com/ipfs/go-ipfs/blob/master/unixfs/test/utils.go#L45 which generates a random file of the given size, using a standard importer. You could make a new method there that creates a random file using the raw leaves importer option instead, and run some of the same tests on that node type in dagmodifier_test.go |
@whyrusleeping most all of the tests in dagmodifier_test has to do with appending data, of which is not supported by raw leaves. It was non-obvious how to modify the test to support the creation of a node without appending. I can attempt (with some effort) to write a new set of tests for just raw leaves if you want. As an aside, I am not sure of the benefit of supporting the full range of operations on raw leaves. Appending to a unixfs dir node is useful to add files. How is appending to a raw node useful? Is there even an API for appending to a file (I don't see it). Also when appending, what should we do when it becomes too large. |
@kevina yes there is an API (using |
@Kubuxu, oh okay, append would be more complicated though and I think should be pushed to a separate p.r. With this one raw nodes are usable within the files api as long as you don't try to create one within the files API. |
@kevina thats fair enough. I think we can live with that restriction for now. |
@whyrusleeping are you okay with testing the size via the sharness tests only? The existing tests are designed for when the node is created within the API and I will have to write a new set of go tests to test that the raw node size is reported correctly, of which I am unsure how to do so I will have to spend some time figuring it out. |
@kevina yeah, for now i guess testing through the api is good enough. |
@whyrusleeping then I think this p.r. is ready |
@kevina mind rebasing on top of latest master? Should make the tests green again |
License: MIT Signed-off-by: Jeromy <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
7eef45c
to
33f699f
Compare
License: MIT Signed-off-by: Jeromy <[email protected]>
No description provided.