decouple the DAG traversal logic from the DAG reader #12

schomatis · 2018-08-27T12:11:20Z

unixfs: decouple the DAG traversal logic from the DAG reader

Decouple the DAG traversal logic from the `PBDagReader` encapsulating it in the
(new) `Walker` structure.

Collapse PB and Buffer DAG readers into one (`dagReader`) removing the
`bufdagreader.go` and `pbdagreader.go` files, moving all the code to
`dagreader.go`.

Remove `TestSeekAndReadLarge` and `TestReadAndCancel` which operated directly on
the `NodePromise` structure that is now abstracted away in `NavigableIPLDNode`,
in the `go-ipld-format` repo, where they should be recreated.

Imported from ipfs/kubo#5257.

Based on #58 (should be merged first).

Fixes #14.
Fixes ipfs/kubo#5255.
Closes ipfs/kubo#5192.

TODO:

Use the new go-ipld-format release with the Walker
Commit message
Evaluate improving the WriteTo method now in this PR, the previous (PBDagReader) implementation was more performant (this doesn't block the review of the rest of the PR).
Port TestSeekAndReadLarge and TestReadAndCancel to go-ipld-format

schomatis · 2018-11-09T17:30:27Z

With ipfs/go-ipld-format#39 merged this is now unblocked, I'll give it the finishing touches and get it ready for review next week.

schomatis · 2018-12-03T04:51:24Z

@Stebalien There are some particulars pending but we can start the review process of this PR now, feel free to delegate the review to whomever you see fit.

(edit: @warpfork might be the best choice here since he's already familiar with the Walker)

schomatis · 2018-12-11T00:37:22Z

/cc @warpfork

schomatis · 2018-12-17T01:23:52Z

@Stebalien This doesn't have a high priority but it's part of one of my Q4 OKRs, if @warpfork is busy at the moment is there anyone else who could help with the review? Basically what I need at this point is not an exhaustive review but just a check to see if this PR makes sense or not to help me plan how minor or major it will be the work needed to eventually make it land.

Stebalien · 2018-12-17T09:09:15Z

I'll bring it up at the meeting tomorrow. Could you put it in the notes: ipfs/team-mgmt#674 (comment)?

magik6k

Looks good, 1 question.

io/dagreader.go

schomatis · 2018-12-18T15:16:05Z

Thanks for taking a look @magik6k! ❤️

eingenito · 2019-01-14T19:47:41Z

@schomatis Is this awaiting further review?

schomatis · 2019-01-14T20:19:58Z

No, thanks for the ping, I'll act on this during this week (and then I'll probably need a final review).

Decouple the DAG traversal logic from the `PBDagReader` encapsulating it in the (new) `Walker` structure. Collapse PB and Buffer DAG readers into one (`dagReader`) removing the `bufdagreader.go` and `pbdagreader.go` files, moving all the code to `dagreader.go`. Remove `TestSeekAndReadLarge` and `TestReadAndCancel` which operated directly on the `NodePromise` structure that is now abstracted away in `NavigableIPLDNode`, in the `go-ipld-format` repo, where they should be recreated. License: MIT Signed-off-by: Lucas Molas <schomatis@gmail.com>

This version writes one node at a time instead of first buffering the entire file contents in a single array (taking up too much memory) before actually writing it.

schomatis · 2019-01-18T20:18:20Z

@magik6k Can I bother you to check the last commit that completes this PR before I merge it please?

magik6k · 2019-01-21T12:12:41Z

io/dagreader.go

+
+		// Skip internal nodes, they shouldn't have any file data
+		// (see the `balanced` package for more details).
+		if len(node.Links()) > 0 {


We should probably check for data and return errors / warnings if preset.

magik6k · 2019-01-21T12:17:03Z

io/dagreader.go

+	})
+
+	if err == ipld.EndOfDag {
+		return n, io.EOF


Looking at impl of bytes.Reader.WriteTo, error should be nil here - https://golang.org/src/bytes/reader.go?s=3220:3278#L124

magik6k · 2019-01-21T12:20:21Z

io/dagreader.go

+		// searches in the `Walker` (see `Walker.Seek`).
+
+	case io.SeekEnd:
+		return dr.Seek(int64(dr.Size())-offset, io.SeekStart)


Suggested change

return dr.Seek(int64(dr.Size())-offset, io.SeekStart)

return dr.Seek(int64(dr.Size())+offset, io.SeekStart)

This is also a bug in current implementation - https://golang.org/pkg/io/#Seeker - https://golang.org/src/bytes/reader.go?s=2744:2806#L103 - ipfs/kubo#5934

Stebalien

It's always nice when the SLOC goes down but the number of lines in a package goes up.

(I'll defer to @magik6k for an actual review/signoff).

Stebalien · 2019-01-21T15:21:26Z

io/dagreader.go

+					return err
+				}
+
+				_, err = dr.currentNodeData.Seek(left, io.SeekStart)


We should probably sanity check to make sure this worked (i.e., we seeked the right amount).

schomatis · 2019-01-28T17:51:22Z

Closed in favor of #60.

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

schomatis self-assigned this Aug 27, 2018

This was referenced Sep 17, 2018

Add a DAG walker with support for IPLD Nodes ipfs/go-ipld-format#39

Merged

[community contribution] Files API milestone ipfs/kubo#5388

Open

[WIP] unixfs: decouple the DAG traversal logic from the DAG reader ipfs/kubo#5257

Closed

schomatis requested a review from Stebalien December 3, 2018 04:49

Stebalien requested a review from warpfork December 3, 2018 19:15

magik6k self-requested a review December 17, 2018 18:07

magik6k reviewed Dec 18, 2018

View reviewed changes

io/dagreader.go Show resolved Hide resolved

magik6k approved these changes Dec 19, 2018

View reviewed changes

ghost assigned Stebalien Jan 9, 2019

mod: TestDagSync, seek before reading

6acf2fb

This was referenced Jan 17, 2019

mod: TestDagSync, seek before reading #58

Merged

Import DAG reader tests involving NodePromises to actually test NavigableIPLDNode ipfs/go-ipld-format#50

Open

schomatis added 2 commits January 18, 2019 17:12

io: implement a performant WriteTo version in the DAG reader

67f1675

This version writes one node at a time instead of first buffering the entire file contents in a single array (taking up too much memory) before actually writing it.

schomatis changed the title ~~[WIP] decouple the DAG traversal logic from the DAG reader~~ decouple the DAG traversal logic from the DAG reader Jan 18, 2019

schomatis added status/in-progress In progress and removed status/in-progress In progress labels Jan 18, 2019

magik6k self-requested a review January 19, 2019 17:54

magik6k reviewed Jan 21, 2019

View reviewed changes

Stebalien reviewed Jan 21, 2019

View reviewed changes

magik6k self-assigned this Jan 25, 2019

magik6k mentioned this pull request Jan 28, 2019

decouple the DAG traversal logic from the DAG reader (local branch) #60

Merged

schomatis closed this Jan 28, 2019

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

schomatis deleted the feat/dag-reader/use-walker branch January 28, 2019 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

decouple the DAG traversal logic from the DAG reader #12

decouple the DAG traversal logic from the DAG reader #12

schomatis commented Aug 27, 2018 •

edited

Loading

schomatis commented Nov 9, 2018

schomatis commented Dec 3, 2018 •

edited

Loading

schomatis commented Dec 11, 2018

schomatis commented Dec 17, 2018

Stebalien commented Dec 17, 2018

magik6k left a comment

schomatis commented Dec 18, 2018

eingenito commented Jan 14, 2019

schomatis commented Jan 14, 2019

schomatis commented Jan 18, 2019

magik6k Jan 21, 2019

magik6k Jan 21, 2019

magik6k Jan 21, 2019

Stebalien left a comment

Stebalien Jan 21, 2019

schomatis commented Jan 28, 2019

	return dr.Seek(int64(dr.Size())-offset, io.SeekStart)
	return dr.Seek(int64(dr.Size())+offset, io.SeekStart)

decouple the DAG traversal logic from the DAG reader #12

decouple the DAG traversal logic from the DAG reader #12

Conversation

schomatis commented Aug 27, 2018 • edited Loading

schomatis commented Nov 9, 2018

schomatis commented Dec 3, 2018 • edited Loading

schomatis commented Dec 11, 2018

schomatis commented Dec 17, 2018

Stebalien commented Dec 17, 2018

magik6k left a comment

Choose a reason for hiding this comment

schomatis commented Dec 18, 2018

eingenito commented Jan 14, 2019

schomatis commented Jan 14, 2019

schomatis commented Jan 18, 2019

magik6k Jan 21, 2019

Choose a reason for hiding this comment

magik6k Jan 21, 2019

Choose a reason for hiding this comment

magik6k Jan 21, 2019

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Stebalien Jan 21, 2019

Choose a reason for hiding this comment

schomatis commented Jan 28, 2019

schomatis commented Aug 27, 2018 •

edited

Loading

schomatis commented Dec 3, 2018 •

edited

Loading