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

Implement 'Raw Node' node type #3307

Merged
merged 5 commits into from
Oct 24, 2016
Merged

Implement 'Raw Node' node type #3307

merged 5 commits into from
Oct 24, 2016

Conversation

whyrusleeping
Copy link
Member

This PR implements a 'Raw Node' dag node that is essentially just a block. Among other things, this allows us to make unixfs not have leaf nodes with extra framing and fit nicely inside multiples of 1024 and 4096.

A new flag --raw-leaves has been added to ipfs add that enables this functionality.

Do note that if you create objects with this flag, you will not be able to transfer them to other peers who are running any version before this (meaning those objects will not be able to be processed by ipfs 0.4.3 nodes).

This depends on the previous PR that turns the merkledag.Node into an interface.

@kevina
Copy link
Contributor

kevina commented Oct 16, 2016

@whyrusleeping, what is the ETA for this. (Ideally I would like my filestore code (#2634) to go in before this.)

Other than the usual conflicts my main concern is that this is implemented in a way the filestore can use it. In order for that to happen the filestore needs to know what format the block is in. If it is the old format it needs to decode it, if it is raw it needs to do nothing.

@whyrusleeping
Copy link
Member Author

@kevina you will be able to check the cid, or do a type switch on the object. It should actually make things easier for you

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
@whyrusleeping
Copy link
Member Author

@Kubuxu @lgierth could you two review this for me?

@kevina
Copy link
Contributor

kevina commented Oct 18, 2016

@whyrusleeping if possible please try not to rebase this, I merged this into kevina/filestore.

@kevina
Copy link
Contributor

kevina commented Oct 18, 2016

@whyrusleeping I notice that neither "ipfs cat" or "ipfs get" work with raw nodes. Can I take it that those parts are just not implemented yet?

Also (although it does not need to go into this pull request) I would suggest that the DagService GetLinks() method has a special case for Raw Nodes to avoid having to unnecessary retrieve a Raw Node when it is not required.

@whyrusleeping
Copy link
Member Author

Ohhhh... yeah. I should make sure cat and get work with raw nodes. Thats an issue.

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
@kevina
Copy link
Contributor

kevina commented Oct 18, 2016

@whyrusleeping I can cat large files, but not small ones, for example:

kevina@intel:/aux/home/gocode/src/github.com/ipfs/go-ipfs$ ipfs add --raw-leaves Makefile
added zdvgq8kK8fHCAsT89ehxvm78PJqJj6FjHkPEMZTkARQnSQRyy Makefile
kevina@intel:/aux/home/gocode/src/github.com/ipfs/go-ipfs$ ipfs cat zdvgq8kK8fHCAsT89ehxvm78PJqJj6FjHkPEMZTkARQnSQRyy
Error: expected protobuf dag node

Note: I don't mind if you rebase 3796e70 (but not ded60a7 as that was was merged in, while 3796e70 was cherry picked).

@whyrusleeping
Copy link
Member Author

@kevina fixed the add/cat of small files

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
test_expect_success "ipfs add --only-hash succeeds" '
echo "unknown content for only-hash" | ipfs add --only-hash -q > oh_hash
'

#TODO: this doesn't work when online hence separated out from test_add_cat_file
#TODO: this doesnt work when online hence separated out from test_add_cat_file
Copy link
Contributor

@kevina kevina Oct 18, 2016

Choose a reason for hiding this comment

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

Typo? "doesnt"

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it because it messed up my syntax highlighting.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay.

@kevina
Copy link
Contributor

kevina commented Oct 18, 2016

@whyrusleeping have you tried this when HashOnRead() is true? It looks like the verification code in blockstore/blockstore.go needs updating.

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
@whyrusleeping
Copy link
Member Author

@kevina good catch, fixed! Anything else you can see?

@whyrusleeping whyrusleeping assigned ghost and Kubuxu Oct 19, 2016
@kevina
Copy link
Contributor

kevina commented Oct 19, 2016

good catch, fixed! Anything else you can see?

I am working on adding raw node support to the filestore, i let you know if I see anything else missing :)

@Kubuxu
Copy link
Member

Kubuxu commented Oct 19, 2016

I would feel much better about it if we run full tests with it enabled by default.
I will do it when I get back home.

@@ -124,9 +137,20 @@ func (n *UnixfsNode) SetData(data []byte) {
n.ufmt.Data = data
}

func (n *UnixfsNode) DataSize() uint64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you called this DataSize() and not FileSize()? FileSize() means the size of the file if this node was the root. In #3314 I added a similar helper method but I called it FileSize() and not DataSize().

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, FileSize probably makes more sense, i'll change it.

@whyrusleeping
Copy link
Member Author

@Kubuxu running the tests with it enabled by default will be hard. All the hashes will be different.

License: MIT
Signed-off-by: Jeromy <why@ipfs.io>
@whyrusleeping whyrusleeping added this to the ipld integration milestone Oct 19, 2016
Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

Left one comment.

I generally feel not great about the quasi state when some features will work some won't in case of raw nodes.

About tests: we should change our tests to keep the all added blocks/files in one place so later the hashes can be remapped easily.

@@ -417,8 +418,14 @@ func (i *gatewayHandler) putHandler(w http.ResponseWriter, r *http.Request) {
return
}

pbnewnode, ok := newnode.(*dag.ProtoNode)
if !ok {
webError(w, "Cannot read non protobuf nodes through gateway", dag.ErrNotProtobuf, http.StatusBadRequest)
Copy link
Member

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 see it is put handler?
Does reading rawnodes through gateway work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, reading raw nodes through the gateway uses the dagreader, which works fine. This nasty hacky area of code is the writeable gateway, which we don't really use...

Copy link
Member

Choose a reason for hiding this comment

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

Can you change "Cannot read" to "Cannot write".

@kevina
Copy link
Contributor

kevina commented Oct 19, 2016

I have added this to the filestore and things seam to work. I would say to go ahead and let this is even if there are still places that might brake with the raw nodes.

@whyrusleeping whyrusleeping merged commit 6f3ae5d into master Oct 24, 2016
@whyrusleeping whyrusleeping deleted the feat/raw-nodes branch October 24, 2016 18:59
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Oct 24, 2016
@RichardLitt
Copy link
Member

Hey @christynelson467 - thanks for the message. Please try and keep comments relevant to the conversation at hand. Open a new issue if you have any questions about go-ipfs. Thanks.

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.

4 participants