[WIP] remove `UnixfsNode` from trickledag #10

bjzhang · 2018-08-15T09:44:25Z

These are RFD patches which try to solve the ipfs/go-ipfs#5135 gradually. Current only Layout of trickledag is modified. I do the go test for go-unixfs package . And test ipfs add and get commands to check whether get the same hash of
file after the code changes.

I want to know if it is the right direction. Thanks.

schomatis · 2018-08-15T12:09:50Z

You most definitely are!!

One small nit: could we move this TODO about TFile being used in the balanced builder to the pacakge doc. Since now you (correctly) added the type as a parameter to the data node functions this is now a problem exclusively of the balanced builder, not the trickle one.

schomatis · 2018-08-15T12:14:13Z

(small edit: added our customary [WIP] prefix in the subject, which implies the RFD and also that this shouldn't be merged yet)

schomatis · 2018-08-16T13:22:45Z

@bjzhang Great, let me know if you need any help or want to discuss some strategy for the append functions.

schomatis · 2018-09-03T14:28:02Z

Hey @bjzhang, do you have any other questions to help you finish this PR? We could also evaluate wrapping up here (this is already a fantastic work) and leaving the rest of the UnixfsNode migration from the trickle builder for another issue.

bjzhang · 2018-09-04T01:19:03Z

@schomatis Sorry. I am back. It takes me some time for OOM in a 2G box和ipfs id show non-exist addresses with my colleague last week. And I am still working on the other functions of trickledag. I think I could post the patch in the next few days. Is it ok for you?

schomatis · 2018-09-04T01:35:32Z

Hey @bjzhang, no rush at all, take your time, I just wanted to check that you weren't blocked here.

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

bjzhang · 2018-11-12T11:50:13Z

Hi, @schomatis sorry for the very late update for my work. I update my patch for all the function in trickledag. Currently, only TestMultipleAppends fail. I have tried to understand why it fails but no progress in recent two weeks. Here is the log:

Running tool: /usr/local/opt/go/libexec/bin/go test -timeout 30s github.com/ipfs/go-unixfs/importer/trickle -coverprofile=/var/folders/7m/1j4wr9xn7zjg4r732r00ps2m0000gn/T/vscode-goEhQcYz/go-code-cover

should: 1000
loop: 0
loop: 1
loop: 2
loop: 3
loop: 4
should: 1000
loop: 0
loop: 1
loop: 2
loop: 3
loop: 4
--- FAIL: TestMultipleAppends (0.00s)
--- FAIL: TestMultipleAppends/leaves=ProtoBuf (0.00s)
/Users/bamvor/works/go/src/github.com/ipfs/go-unixfs/importer/trickle/trickle_test.go:583: expected file as branch node, got: Raw
--- FAIL: TestMultipleAppends/leaves=Raw (0.00s)
/Users/bamvor/works/go/src/github.com/ipfs/go-unixfs/importer/trickle/trickle_test.go:583: expected ProtoNode
[65 66] [65 66]
FAIL
coverage: 48.8% of statements
FAIL github.com/ipfs/go-unixfs/importer/trickle 0.779s
Error: Tests failed.

schomatis · 2018-11-12T15:22:10Z

Hey @bjzhang, thanks for following up on this PR, I'll take a look at it during this week.

schomatis · 2018-11-13T15:30:17Z

So, I have a few pointers for some parts of the patch but I'm not finding any noticeable bug here, we'll need to take a closer look at it, the problem is definitely in the Append family of functions which was modified in the last two commits.

If you do a global test, go test ./... -v in the go-unixfs package you'll notice other tests also failing which may help you diagnose this.

Also, bringing @overbool if he's interest in taking a look here: I'm not sure if you're familiar with the trickle importer, you can read the original issue (ipfs/kubo#5135) for more context and @bjzhang can fill you in on his implementation solution (and feel to ask me any follow-up questions).

bjzhang · 2018-11-21T07:20:52Z

So, I have a few pointers for some parts of the patch but I'm not finding any noticeable bug here, we'll need to take a closer look at it, the problem is definitely in the Append family of functions which was modified in the last two commits.

Yes, I think so, but I do not get the clue yet.

If you do a global test, go test ./... -v in the go-unixfs package you'll notice other tests also failing which may help you diagnose this.

Yes, I could see the failure in importer/trickle and mod.

overbool · 2018-11-30T07:57:06Z

unixfs.go

+
+	return n, nil
+}
+
 // NewFSNode creates a new FSNode structure with the given `dataType`.


@bjzhang Maybe we could wrap FSNodeFromBytes() to implement it. I think it's pretty much the same logic asFSNodeFromBytes().

Sure, it is a quick patch for fixing the type issue. I will update it in my next push. Thanks.

schomatis · 2018-12-13T22:33:09Z

Hey @bjzhang, sorry I haven't been able to dedicate more attention to this, I'm not sure I can allocate the time needed to figure out this bug in the near future so I think the next best thing is to merge the already great work done here without the last 2 commits that are failing, we can handle the Append functions later in a separate PR.

Could you move the last 2 commits to a separate PR (on top of this one) to allow me to do the final review here and merge this?

bjzhang · 2018-12-14T08:16:16Z

Hey @bjzhang, sorry I haven't been able to dedicate more attention to this, I'm not sure I can allocate the time needed to figure out this bug in the near future so I think the next best thing is to merge the already great work done here without the last 2 commits that are failing, we can handle the Append functions later in a separate PR.

Could you move the last 2 commits to a separate PR (on top of this one) to allow me to do the final review here and merge this?

Hi, @schomatis

For the Layout function, only 3 commits are needed. So, I force push these right now. It seems that I use the wrong size of node for Append. I will try to dig into it. And ask question later. Thanks.

bjzhang · 2018-12-24T07:21:48Z

hi @schomatis I fix the test failure of trickledag last weekend. But it still failed in dagmodifier, it seems that there is some misunderstanding of size of dag in my patch. I will push new pr base on this soon.

schomatis · 2018-12-24T12:37:42Z

Great!

What was the issue? Maybe it's related also to the size misunderstanding.

bjzhang · 2018-12-24T12:44:21Z

I miss the Commit of FSNodeOverDag which will set data to dag.

bjzhang · 2018-12-25T09:06:55Z

hi, @schomatis I push my current patch series in #54. ~~And describe the current status there~~. go test ./... pass in that pr. Thanks.

schomatis · 2018-12-25T15:22:38Z

Super, closing this PR then so we can focus on the other one.

schomatis added the status/in-progress In progress label Aug 15, 2018

schomatis changed the title ~~RFD: remove UnixfsNode from trickledag~~ [WIP] remove UnixfsNode from trickledag Aug 15, 2018

bjzhang force-pushed the remove-unixfs-node-from-trickledag-RFD branch from 770c69f to 5693eb7 Compare August 16, 2018 08:24

schomatis mentioned this pull request Sep 26, 2018

How are files formatted inside nodes? ipfs/kubo#5059

Closed

Bamvor Zhang added 3 commits November 12, 2018 19:37

bjzhang force-pushed the remove-unixfs-node-from-trickledag-RFD branch from 5693eb7 to 25d40bb Compare November 12, 2018 11:46

bjzhang requested a review from schomatis as a code owner November 12, 2018 11:46

overbool reviewed Nov 30, 2018

View reviewed changes

bjzhang force-pushed the remove-unixfs-node-from-trickledag-RFD branch from 25d40bb to ab44352 Compare December 14, 2018 08:12

bjzhang mentioned this pull request Dec 25, 2018

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

Merged

schomatis closed this Dec 25, 2018

ghost removed the status/in-progress In progress label Dec 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] remove `UnixfsNode` from trickledag #10

[WIP] remove `UnixfsNode` from trickledag #10

bjzhang commented Aug 15, 2018

schomatis commented Aug 15, 2018

schomatis commented Aug 15, 2018

schomatis commented Aug 16, 2018

schomatis commented Sep 3, 2018

bjzhang commented Sep 4, 2018

schomatis commented Sep 4, 2018

bjzhang commented Nov 12, 2018

schomatis commented Nov 12, 2018

schomatis commented Nov 13, 2018

bjzhang commented Nov 21, 2018

overbool Nov 30, 2018

bjzhang Dec 4, 2018

schomatis commented Dec 13, 2018

bjzhang commented Dec 14, 2018

bjzhang commented Dec 24, 2018

schomatis commented Dec 24, 2018

bjzhang commented Dec 24, 2018

bjzhang commented Dec 25, 2018 •

edited

Loading

schomatis commented Dec 25, 2018

[WIP] remove UnixfsNode from trickledag #10

[WIP] remove UnixfsNode from trickledag #10

Conversation

bjzhang commented Aug 15, 2018

schomatis commented Aug 15, 2018

schomatis commented Aug 15, 2018

schomatis commented Aug 16, 2018

schomatis commented Sep 3, 2018

bjzhang commented Sep 4, 2018

schomatis commented Sep 4, 2018

bjzhang commented Nov 12, 2018

schomatis commented Nov 12, 2018

schomatis commented Nov 13, 2018

bjzhang commented Nov 21, 2018

overbool Nov 30, 2018

Choose a reason for hiding this comment

bjzhang Dec 4, 2018

Choose a reason for hiding this comment

schomatis commented Dec 13, 2018

bjzhang commented Dec 14, 2018

bjzhang commented Dec 24, 2018

schomatis commented Dec 24, 2018

bjzhang commented Dec 24, 2018

bjzhang commented Dec 25, 2018 • edited Loading

schomatis commented Dec 25, 2018

[WIP] remove `UnixfsNode` from trickledag #10

[WIP] remove `UnixfsNode` from trickledag #10

bjzhang commented Dec 25, 2018 •

edited

Loading