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

Simplify merkledag package interface to ease IPLD inclusion #2359

Merged
merged 12 commits into from
Mar 4, 2016

Conversation

mildred
Copy link
Contributor

@mildred mildred commented Feb 18, 2016

  • Remove usage of merkledag.Link.Node pointer outside of merkledag
  • Make Node.Unmarshal() private: it is not used elsewhere
  • Rename Decoded(= into DecodeProtobuf() to clearly give intent about the byte wire format

Obsoletes #1916 and #1902.

This prepares for inclusion of IPLD where the Node pointer might not be publicly accessible of even be there. We should probably move away from using the struct attributes and use functions to access them instead. This will make it easier to plug in IPLD in place of merkledag.

@mildred
Copy link
Contributor Author

mildred commented Feb 18, 2016

  • AppVeyor fails with install deps: failed to fetch dependencies
  • Travis also fails with dependency error, except on Windows where I don't understand what is the error

Is there a way to manually trigger the build so I could test what does wrong incrementally?

@chriscool
Copy link
Contributor

Travis also fails with dependency error, except on Windows where I don't understand what is the error

You mean on MacOS as Travis runs on MacOS and Linux.
Anyway all the errors on Travis and Appveyor seem to happen when fetching dependencies.

@whyrusleeping
Copy link
Member

the appveyor one is now better, its moved on to failing on sharness tests. The travis one is weird... its hanging while contacting the gateways for some reason.

That aside, I think now is a good opportunity to, if not remove, at least unexport the Node field, i'm in favor of removing it if youre feeling adventurous.

if nlink.Node == nil {
// fetch object for link and assign to nd
ctx, cancel := context.WithTimeout(ctx, time.Minute)
if nlink.GetCachedNode() == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesnt seem right now... We should probably just drop this timeout logic altogether

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried not to change the code logic at all, but you are right, this could be simplified.

@mildred
Copy link
Contributor Author

mildred commented Feb 24, 2016

Made the Node field private, and simplified path/resolver.go as you suggested.

@mildred
Copy link
Contributor Author

mildred commented Feb 28, 2016

@whyrusleeping I'm removing AddRecursive and RemoveRecursive so I can finally remove the Node field as you suggested. I still have a commit to push and this field is gone.

@mildred
Copy link
Contributor Author

mildred commented Mar 1, 2016

These past weeks, I have been thinking on how to include IPLD in IPFS, and my last idea is to transform the Get() methods on DAGService and NodeGetter to GetPB() methods. And add a Get() method that returns an IPLDNode instead of a protocol buffer Node. Then gradually phase out the GetPB() method in favor of the Get() method.

See mildred/go-ipfs@ipld...mildred:ipld-inclusion

This prepares for inclusion of IPLD where the Node pointer won't be there.

License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
This function work only with protocol buffer encoding. To make this clear,
rename the function.

License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
@mildred mildred force-pushed the ipld branch 3 times, most recently from 4a5d25e to 34f8327 Compare March 1, 2016 16:07
@mildred
Copy link
Contributor Author

mildred commented Mar 1, 2016

Tests are failing since commit "merkledag: Remove cached Node.node …".

@whyrusleeping
Copy link
Member

I don't know how onboard i am with the GetPB change right now, i think we should go ahead and get the earlier cleanup work merged first, and then discuss that change in another PR

@mildred
Copy link
Contributor Author

mildred commented Mar 2, 2016

@whyrusleeping makes sense. I'll leave this commit out for the moment (in my next branch update).

Also, I'd need to discuss how integration of IPLD could be done. Is there an issue for that ? Also, I'll need some review on ipld/go-ipld-deprecated#19

if err := i.node.DAG.AddRecursive(newnode); err != nil {
webError(w, "Could not add recursively new node", err, http.StatusInternalServerError)
if _, err := i.node.DAG.Add(newnode); err != nil {
webError(w, "Could not add recursively root node", err, http.StatusInternalServerError)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably remove the word recursively here

@whyrusleeping
Copy link
Member

one minor comment and then LGTM

License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
License: MIT
Signed-off-by: Mildred Ki'Lya <mildred-pub.git@mildred.fr>
@mildred
Copy link
Contributor Author

mildred commented Mar 4, 2016

Corrected.

@whyrusleeping
Copy link
Member

Thanks!

whyrusleeping added a commit that referenced this pull request Mar 4, 2016
Simplify merkledag package interface to ease IPLD inclusion
@whyrusleeping whyrusleeping merged commit 91c6f0f into ipfs:master Mar 4, 2016
hacdias pushed a commit to ipfs/boxo that referenced this pull request Jan 27, 2023
Simplify merkledag package interface to ease IPLD inclusion

This commit was moved from ipfs/kubo@91c6f0f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants