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

[WIP] Add Filestore support for the trickle DAG builder #4734

Closed
wants to merge 2 commits into from
Closed

[WIP] Add Filestore support for the trickle DAG builder #4734

wants to merge 2 commits into from

Conversation

schomatis
Copy link
Contributor

@schomatis schomatis commented Feb 23, 2018

This needs to be updated after #5135 is done. Any help from the community is welcome.


This is a work in progress, not ready to be merged.

As commented in #4052, the support for Filestore will be added only to the Layout function for now, not to Append. This means that ipfs add --trickle will support --nocopy (Filestore), but that added node when copied to MFS won't support ipfs files write (which uses Append).

It builds on top of the #4730 that should be merged first.

Closes #4052.

@schomatis
Copy link
Contributor Author

schomatis commented Feb 23, 2018

Due to this partial Filestore support (for Layout but not for Append) I'm not sure I can just add the SetPosInfo functionality (and add the offset parameter) to the support functions fillTrickleRec and FillNodeLayer as it was done for the balanced builder because they are also used in Append (and related), see this commit of this PR as an example (which doesn't build because the calls from Append weren't adapted). I'm thinking of a few possibilities:

  1. Just add SetPosInfo for Append as it won't make a difference because it won't be called for Filestore nodes. This makes it more homogeneous (Layout and Append can use the same support functions) but it will be unnecessarily modifying Append (as it's not defined how, and if, Append will support Filestore).

  2. Overload fillTrickleRec (and similarly FillNodeLayer) to call them from Append with "unset" parameters (e.g., offset: -1) that will cause it not to use SetPosInfo in that case. It won't be very nice, it will add many if clauses.

  3. Split the functionality of fillTrickleRec in two separate functions, one with Filestore support (with calls to SetPosInfo) and one without (do the same for FillNodeLayer). This will likely cause some code duplication.

@kevina WDYT?

@kevina
Copy link
Contributor

kevina commented Feb 23, 2018

@schomatis I don't know enough about the trickle DAG builder to make to know for sure. I would add some test to t0270-filestore.sh that use the add --chunker=rabin --nocopy ....

@schomatis
Copy link
Contributor Author

Another (4th) possibility is to refactor the Filestore functionality inside DagBuilderHelper, taking this responsibility away from the functions that rely upon the helper.

The function SetPosInfo is already contained by it, and (following the balanced DAG builder logic) SetPosInfo is always called after DagBuilderHelper.NewUnixfsNode() (see 1, 2), so the offset could be a variable inside the DagBuilderHelper structure and it's value would be updated after each call to DagBuilderHelper.GetNextDataNode (instead of getting it indirectly from the node's FileSize).

This option seems more organic to me because it's the helper the centralized point where data nodes are obtained from, and regardless of how they are organized (balanced, trickle, etc.) their offset will be the same (as long as the nodes are processed in a FIFO order). I would still need some pointers on how to organize this refactoring though.

@schomatis
Copy link
Contributor Author

@whyrusleeping WDYT? Could you give me some recommendations here please?

@whyrusleeping
Copy link
Member

Hey @schomatis, I'm a bit busy working on the 0.4.14 release. If I don't review this in the next couple days please do ping me again.

Also maybe @Kubuxu can help

@Kubuxu Kubuxu self-requested a review March 5, 2018 23:42
@Kubuxu
Copy link
Member

Kubuxu commented Mar 10, 2018

I will try to take a look at this after the weekend.

@schomatis schomatis self-assigned this Aug 20, 2018
@schomatis schomatis added topic/files Topic files help wanted Seeking public contribution on this issue topic/UnixFS Topic UnixFS labels Aug 20, 2018
@schomatis
Copy link
Contributor Author

This code has been moved to go-unixfs and with the trickle importer now abandoning the UnixfsNode structure this PR adds little value to justify an extraction there.

@schomatis schomatis closed this Dec 13, 2018
@schomatis schomatis deleted the feat/dag/trickle-filestore branch December 13, 2018 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Seeking public contribution on this issue topic/files Topic files topic/UnixFS Topic UnixFS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants