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

[PoC] importer: UnixFS type Raw is never used #5120

Closed
wants to merge 1 commit into from
Closed

[PoC] importer: UnixFS type Raw is never used #5120

wants to merge 1 commit into from

Conversation

schomatis
Copy link
Contributor

Do not merge.

@schomatis schomatis added topic/files Topic files topic/technical debt Topic technical debt labels Jun 14, 2018
@schomatis schomatis added this to the Files API Documentation milestone Jun 14, 2018
@schomatis schomatis self-assigned this Jun 14, 2018
@schomatis schomatis requested a review from Kubuxu as a code owner June 14, 2018 23:58
@ghost ghost added the status/in-progress In progress label Jun 14, 2018
License: MIT
Signed-off-by: Lucas Molas <schomatis@gmail.com>
@schomatis
Copy link
Contributor Author

schomatis commented Jun 18, 2018

As explained in #5106 (comment), the balanced builder is not using the Raw type for the leaf nodes, only the trickle builder does (i.e., a minimal fraction of the code). This seems to be the case for a long time now, all of the hard-coded hashes in the tests are built with this bug. Right now I'm incorporating this in the PR #5118 which is refactoring the balanced builder to avoid upsetting Jenkins, but I'm not sure how to proceed in the long term. The worst part of this (IMO) is that we're saying something we're not doing (actually we're not saying much as there are no UnixFS specs, but the code clearly implies the use of the Raw type for leaves) and I imagine that would be frustrating for someone who would like to implement its own IPFS stack (or at least the Files API part of it).

@schomatis
Copy link
Contributor Author

/cc @whyrusleeping @Stebalien

@kevina
Copy link
Contributor

kevina commented Jun 18, 2018

@schomatis please see #4693 I think it is related, and had to add some special case code to avoid changing existing hashes of empty files. Any sort of refactoring will need careful testing to make sure we still do things in the same manor so that we don't change existing hashes.

@whyrusleeping
Copy link
Member

@schomatis Yeah, the lack of the usage of Raw here is erroneous here. Unfortunately we're committed to it unless we change hashes. With the --raw-leaves option we switch to using the raw node type which skips the protobuf overhead entirely, this is what we're hoping to switch to moving forward.

@schomatis
Copy link
Contributor Author

@whyrusleeping Could we change the trickle builder behavior (and hashes) to avoid using the Raw UnixFS type altogether then? (To avoid future confusions.)

@whyrusleeping
Copy link
Member

nope, can't change hashes.

@schomatis
Copy link
Contributor Author

@whyrusleeping Then we should write a clear documentation around the .proto UnixFS definition to clarify this. Is there anything else we could do to improve the current situation?

@whyrusleeping
Copy link
Member

Then we should write a clear documentation around the .proto UnixFS definition to clarify this

Yeah, more docs are good.

Is there anything else we could do to improve the current situation?

The current situation of not using Raw even though its there? Really just docs for now, and try to push work on unixfs v2 forward.

@schomatis
Copy link
Contributor Author

The current situation of not using Raw even though its there?

Just to clarify, I think it's a bit more complex than that because we are actually using it: the trickle DAGs use that type for its leaves (although yes, it's a tiny fraction of the total, not sure who uses trickle) and the balanced builder insinuates it in its code but doesn't (that should be partly addressed in #5118).

@Stebalien
Copy link
Member

As far as I know, MFS currently uses trickle. And yeah, this all around sucks.

@Stebalien
Copy link
Member

@schomatis status? Can we close this?

@Stebalien Stebalien added the need/author-input Needs input from the original author label Aug 29, 2018
@schomatis
Copy link
Contributor Author

Yes, we can't fix this and it has been documented in the code.

@schomatis schomatis closed this Aug 30, 2018
@schomatis schomatis removed the need/author-input Needs input from the original author label Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/files Topic files topic/technical debt Topic technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants