Conversation
License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
**DO NOT MERGE!** Refactor IPFS to use: ipfs/go-ipld-format#8 Context: IPLD isn't currently very usable from libp2p because interfaces like the DAGService are bundled into go-ipfs. This PR extracts these interfaces and some of the related helper functions. License: MIT Signed-off-by: Steven Allen <steven@stebalien.com>
There was a problem hiding this comment.
So, there are a few of things I'm not happy about.
EnumerateChildren takes a GetLinks function. Given that we've removed the GetLinks function from DAGService (made it a helper function that takes a NodeGetter), I'd like to just make .EnumerateChildren take a NodeGetter
edit: I've changed my mind now that I've read the relevant commit. However, I'm still confused about case 1 below.
There are two cases where this gets tricky:
We use a(answered)GetLinksDirectfunction here (and a few other places). Why do we do this? Was there any reason to bypass theLinkServiceor are we doing this because we didn't have aLinkService? @kevina?We define a customGetLinksfunction here that logs errors to a channel instead of returning them. I'd like to reimplement this as a customNodeGetter.
Implementing OfflineNodeGetter can be a bit annoying.
GetMany should probably be moved to NodeGetter (from DAGService). This would allow more services to take NodeGetters instead of full DAGServices. Unfortunately, this would be yet another method that needs to be implemented on NodeGetters (I really wish golang had a concept of default implementations).
| ses exchange.Fetcher | ||
| } | ||
|
|
||
| func (s *Session) Blockstore() blockstore.Blockstore { |
There was a problem hiding this comment.
I need this to implement OfflineNodeGetter() on sesGetter.
| prefix: dbp.Prefix, | ||
| maxlinks: dbp.Maxlinks, | ||
| batch: dbp.Dagserv.Batch(), | ||
| batch: node.Batching(dbp.Dagserv), |
There was a problem hiding this comment.
Is this name clear? Maybe BufferedDAGAdder (yuck!)?
| t.size += len(nd.RawData()) | ||
| if t.size > t.MaxSize || len(t.blocks) > t.MaxBlocks { | ||
| return nd.Cid(), t.Commit() | ||
| func GetLinksWithDAG(ng node.NodeGetter) GetLinks { |
There was a problem hiding this comment.
| set := cid.NewSet() | ||
| for _, k := range n.Pinning.RecursiveKeys() { | ||
| err := dag.EnumerateChildren(n.Context(), n.DAG.GetLinks, k, set.Visit) | ||
| err := dag.EnumerateChildren(n.Context(), dag.GetLinksWithDAG(n.DAG), k, set.Visit) |
There was a problem hiding this comment.
I'm not sure EnumerateChildren has the right interface. IMO, it should be taking a NodeGetter.
There was a problem hiding this comment.
@Stebalien I take it we cleared this up and you are okay with the current interface?
There was a problem hiding this comment.
Yes, and thanks for answering my questions.
Yes, because in some cases we need to make sure the nodes are available locally, using LinkService defeats this purpose. For example FetchGraph uses it when calling EnumerateChildrenAsync. If the link service was used then FetchGraph may not fetch some of the children (i.e. leaves). |
|
And I should add that other times we don't care if the Nodes exist locally we just want to visit (i.e. call the visit function) for each Cid in the Dag. This is why Now that I think about it a bit, In any case I am not sure how the presence of the |
|
Oh, okay. Well I guess that is unavoidable as I hope we have established that |
|
@Stebalien This looks good to me, i'm fine to get this moving forward |
|
(to explicitly keep this in the comment thread instead of quietly tracking my questions in my first comment) @kevina got it, all questions answered. I'm moving forward with my original go-ipld-format PR as those changes appear to work. |
|
@Stebalien I have not gone over this is with a fine-tooth comb, but I am also okay with moving forward on this. |
|
This has served its purpose. I'll revive when I make progress on the linked PR. |
DO NOT MERGE!
Refactor IPFS to use: ipfs/go-ipld-format#8
Context: IPLD isn't currently very usable from libp2p because interfaces like the DAGService are bundled into go-ipfs. This PR extracts these interfaces and some of the related helper functions.