[WIP] remove `UnixfsNode` from trickledag(part2) #54

bjzhang · 2018-12-25T09:04:52Z

This pull request try to fix ipfs/kubo#5135 and it is based on #10.

Currently, go test ./... successful. The test log:

$ go test ./...
ok      github.com/ipfs/go-unixfs       (cached)
?       github.com/ipfs/go-unixfs/archive       [no test files]
?       github.com/ipfs/go-unixfs/archive/tar   [no test files]
ok      github.com/ipfs/go-unixfs/hamt  (cached)
ok      github.com/ipfs/go-unixfs/importer      (cached)
ok      github.com/ipfs/go-unixfs/importer/balanced     (cached)
?       github.com/ipfs/go-unixfs/importer/helpers      [no test files]
ok      github.com/ipfs/go-unixfs/importer/trickle      (cached)
ok      github.com/ipfs/go-unixfs/io    (cached)
ok      github.com/ipfs/go-unixfs/mod   (cached)
?       github.com/ipfs/go-unixfs/pb    [no test files]
?       github.com/ipfs/go-unixfs/test  [no test files]

schomatis

First, this is great @bjzhang! Thanks for putting in all this work, the ./t0250-files-api.sh test is passing and starting now from a working PR everything is made much easier.

I left a couple of minor comments for now, give me some time to review this thoroughly, especially the modifications in FSNodeOverDag which I understand come from the Append logic in the trickle builder that wasn't present in the balanced one, I want to be sure we need those new methods or if there's something we can refactor on the trickle logic to avoid adding more complexity.

schomatis · 2018-12-25T15:48:19Z

importer/helpers/dagbuilder.go

+
+	// while we have room AND we're not done
+	for node.NumChildren() < db.maxlinks && !db.Done() {
+		//TODO size


Can this TODO be removed now?

schomatis · 2018-12-25T15:51:47Z

importer/helpers/dagbuilder.go

@@ -251,6 +246,26 @@ func (db *DagBuilderHelper) FillNodeLayer(node *UnixfsNode) error {
 	return nil
 }

+// FillFSNodeLayer do the same thing as FillNodeLayer.


Can we move the comments from the original UnixfsNode functions like FillNodeLayer here? Also, we can now just remove everything related to UnixfsNode right?

ok. Test passed after remove everything of UnixfsNode.

schomatis · 2018-12-25T15:55:36Z

importer/helpers/dagbuilder.go

@@ -388,6 +403,24 @@ func (db *DagBuilderHelper) NewFSNodeOverDag(fsNodeType pb.Data_DataType) *FSNod
 	return node
 }

+// NewFSNFromDag reconstructs a FSNodeOverDag node from a given dag node
+func (db *DagBuilderHelper) NewFSNFromDag(nd *dag.ProtoNode) (*FSNodeOverDag, error) {


nit: can we expand all the newFSN to newFSNode names to be consistent with the current terminology?

NewFSNFromDag should be NewFSNodeOverDagFromDag. It is too long for me, and I simply truncate it at that time. Is there some better name in your mind?

Agreed, drop the OverDag, I was thinking of expanding just the FSN to FSNode part: NewFSNodeFromDag.

schomatis · 2018-12-25T16:41:09Z

Note to self: the obscure portion in the trickle logic is the recursive loop appendFillLastChild -> appendRec -> appendFillLastChild.

bjzhang · 2018-12-26T09:17:48Z

Hi @schomatis I just push latest version of my patches which address above comments, here is not some notes when I remove everything relative to the UnixfsNode

NewLeafNode will return the FSNodeOverDag with the given fsNodeType. While the old NewLeaf: if data is nil the type field will be TRaw (for backwards compatibility), if data is defined (but possibly empty) the type field will be TRaw.. Not sure if I should follow this. And because of this, I keep the NewLeafNode and NewLeafDataNode, not rename them to NewLeaf and GetNextDataNode.
There is no functions in importer/helpers/helpers.go. I am thinking if I should move the FSNodeOverDag part of importer/helpers/dagbuilder.go into importer/helpers/helpers.go.
GetDagNode return FilestoreNode for RawNode. But I do not understand how it is used in the DagBuilderHelper.AddChild. And FileSize do not calculate the size of RawNode because there is no flag of Raw in FSNodeOverDag.

schomatis · 2018-12-26T21:01:31Z

here is not some notes when I remove everything relative to the UnixfsNode

Great, I'll take a closer look and get back to you.

NewLeafNode and NewLeafDataNode is introduced in commit 474b77a2bdb1c ("importer: remove `UnixfsNode` from the balanced builder"). It is intended to return ipfs.Node instead of UnixfsNode. But it only support creating the TFile leaf node for merkledag. This commit add fsNodeType to above two functions and update the code in dagbuild.go. Further patches of trickledag will make use of them and pass TRaw to create leaf node. License: MIT Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

After fsNodeType in NewLeafNode is supported by commit 85897b3 ("dag: add fsNodeType in NewLeafNode and NewLeafDataNode"). Move comments in NewLeafNode to importer/balanced/builder.go to clarify why TFile is used by balanced builder as leaves. License: MIT Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

This patch is the part of trickledag work which is similar to the merkledag work in commit 474b77a2bdb1c ("importer: remove `UnixfsNode` from the balanced builder"). Two helper functions(fillTrickleRecFSNode and FillFSNodeLayer) is introduced temporarily for modifing the Layout functions. These two funtions will be removed when all the code of UnixfsNode is removed in trickledag.go. Test ipfs add and get commands to check whether get the same hash of file after the code changes. License: MIT Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

License: MIT Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

Notes of removing UnixfsNode: - `NewLeafNode` will return the `FSNodeOverDag` with the given `fsNodeType`. While the old `NewLeaf`: `if data is nil the type field will be TRaw (for backwards compatibility), if data is defined (but possibly empty) the type field will be TRaw.`. Not sure if I should follow this. And because of this, I keep the `NewLeafNode` and `NewLeafDataNode`, not rename them to `NewLeaf` and `GetNextDataNode`. - There is no functions in importer/helpers/helpers.go. I am thinking if I should move the `FSNodeOverDag` part of importer/helpers/dagbuilder.go into importer/helpers/helpers.go. - `GetDagNode` return FilestoreNode for RawNode. But I do not understand how it is used in the `DagBuilderHelper.AddChild`. And `FileSize` do not calculate the size of RawNode because there is no flag of Raw in `FSNodeOverDag`. License: MIT Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

schomatis

Sorry for the delay in the review. Added some documentation and renamed a couple of variables in the append functions, there's definitely something strange in the relationship between appendFillLastChild and appendRec (without mentioning the fact that we are repeating the same for loop in all 3 append functions) but that's not something that needs to be fixed in this PR.

Great job @bjzhang!! I can't believe it but we have finally slain the UnixfsNode monster ⚔️ (it still shocks me to see the diff), when I started reading the UnixFS code last year this structure was really intimidating, but now new users won't have to go through the same as we did thanks to you. I recognize that delving into this code hasn't been easy (that's why it was abandoned for such a long time) so I really value your dedication here, I expected nothing less from a kernel hacker ❤️

bjzhang · 2019-01-19T10:34:16Z

Hi, @schomatis . Thanks for your guiding, suggestion and help. I hope I could do more for go-ipfs. Is there something else I could do?

schomatis · 2019-01-19T16:06:20Z

Glad you asked :)

ipfs/go-ipld-format#50

bjzhang requested a review from schomatis as a code owner December 25, 2018 09:04

bjzhang mentioned this pull request Dec 25, 2018

[WIP] remove UnixfsNode from trickledag #10

Closed

bjzhang force-pushed the fix/dag/WIP-remove-unixfs-node-from-trickledag branch from af4360a to 959f06a Compare December 25, 2018 09:20

schomatis reviewed Dec 25, 2018

View reviewed changes

bjzhang force-pushed the fix/dag/WIP-remove-unixfs-node-from-trickledag branch from 959f06a to 9402e69 Compare December 26, 2018 09:16

bjzhang force-pushed the fix/dag/WIP-remove-unixfs-node-from-trickledag branch from 9402e69 to 7c129ff Compare December 26, 2018 09:27

ghost assigned schomatis Jan 15, 2019

ghost added the status/in-progress In progress label Jan 15, 2019

schomatis force-pushed the fix/dag/WIP-remove-unixfs-node-from-trickledag branch 2 times, most recently from 0c485c2 to 29ce207 Compare January 15, 2019 20:31

Bamvor Zhang and others added 12 commits January 16, 2019 12:23

Trickle: add new functions for FSNodeOverDag

844fab0

Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

dag: remove UnixfsNode in Append of trickledag

921b53f

License: MIT Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

dag: remove fillTrickleRec

f956bdd

License: MIT Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

dag: remove trickleDepthInfo

5737e29

License: MIT Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

dag: remove the old FillNodeLayer

9216804

License: MIT Signed-off-by: Bamvor Zhang <jian.zhang@ipfsbit.com>

trickle: document fillTrickleRec and trickleDepthInfo

bc4239c

trickle: rename variables according to new trickleDepthInfo names

13730b7

helpers: doc and TODOs

002f54f

schomatis force-pushed the fix/dag/WIP-remove-unixfs-node-from-trickledag branch from 2bc1e15 to 002f54f Compare January 16, 2019 15:36

schomatis approved these changes Jan 16, 2019

View reviewed changes

schomatis merged commit 2ea1b47 into ipfs:master Jan 16, 2019

ghost removed the status/in-progress In progress label Jan 16, 2019

bjzhang deleted the fix/dag/WIP-remove-unixfs-node-from-trickledag branch January 19, 2019 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] remove `UnixfsNode` from trickledag(part2) #54

[WIP] remove `UnixfsNode` from trickledag(part2) #54

bjzhang commented Dec 25, 2018 •

edited

Loading

schomatis left a comment

schomatis Dec 25, 2018

bjzhang Dec 26, 2018

schomatis Dec 25, 2018

bjzhang Dec 26, 2018

schomatis Dec 25, 2018

bjzhang Dec 26, 2018

schomatis Dec 26, 2018

schomatis commented Dec 25, 2018

bjzhang commented Dec 26, 2018 •

edited

Loading

schomatis commented Dec 26, 2018

schomatis left a comment

bjzhang commented Jan 19, 2019

schomatis commented Jan 19, 2019

[WIP] remove UnixfsNode from trickledag(part2) #54

[WIP] remove UnixfsNode from trickledag(part2) #54

Conversation

bjzhang commented Dec 25, 2018 • edited Loading

schomatis left a comment

Choose a reason for hiding this comment

schomatis Dec 25, 2018

Choose a reason for hiding this comment

bjzhang Dec 26, 2018

Choose a reason for hiding this comment

schomatis Dec 25, 2018

Choose a reason for hiding this comment

bjzhang Dec 26, 2018

Choose a reason for hiding this comment

schomatis Dec 25, 2018

Choose a reason for hiding this comment

bjzhang Dec 26, 2018

Choose a reason for hiding this comment

schomatis Dec 26, 2018

Choose a reason for hiding this comment

schomatis commented Dec 25, 2018

bjzhang commented Dec 26, 2018 • edited Loading

schomatis commented Dec 26, 2018

schomatis left a comment

Choose a reason for hiding this comment

bjzhang commented Jan 19, 2019

schomatis commented Jan 19, 2019

[WIP] remove `UnixfsNode` from trickledag(part2) #54

[WIP] remove `UnixfsNode` from trickledag(part2) #54

bjzhang commented Dec 25, 2018 •

edited

Loading

bjzhang commented Dec 26, 2018 •

edited

Loading