[WIP] Extract DAGService and friends #4063

Stebalien · 2017-07-12T18:48:02Z

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 <[email protected]>

**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 <[email protected]>

Stebalien

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 GetLinksDirect function here (and a few other places). Why do we do this? Was there any reason to bypass the LinkService or are we doing this because we didn't have a LinkService? @kevina? (answered)
~~We define a custom GetLinks function here that logs errors to a channel instead of returning them. I'd like to reimplement this as a custom NodeGetter.~~

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).

Stebalien · 2017-07-12T18:50:17Z

blockservice/blockservice.go

@@ -258,6 +258,10 @@ type Session struct {
 	ses exchange.Fetcher
 }

+func (s *Session) Blockstore() blockstore.Blockstore {


I need this to implement OfflineNodeGetter() on sesGetter.

Stebalien · 2017-07-12T18:51:25Z

importer/helpers/dagbuilder.go

@@ -56,7 +56,7 @@ func (dbp *DagBuilderParams) New(spl chunk.Splitter) *DagBuilderHelper {
 		rawLeaves: dbp.RawLeaves,
 		prefix:    dbp.Prefix,
 		maxlinks:  dbp.Maxlinks,
-		batch:     dbp.Dagserv.Batch(),
+		batch:     node.Batching(dbp.Dagserv),


Is this name clear? Maybe BufferedDAGAdder (yuck!)?

Stebalien · 2017-07-12T18:55:18Z

merkledag/merkledag.go

-	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 {


See https://github.com/ipfs/go-ipfs/pull/4063/files#diff-e003ddd8fa21505a93b34cbd7a569d30R552

Stebalien · 2017-07-12T18:58:11Z

core/commands/pin.go

@@ -548,7 +549,7 @@ func pinLsAll(typeStr string, ctx context.Context, n *core.IpfsNode) (map[string
 	if typeStr == "indirect" || typeStr == "all" {
 		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)


I'm not sure EnumerateChildren has the right interface. IMO, it should be taking a NodeGetter.

@Stebalien I take it we cleared this up and you are okay with the current interface?

Yes, and thanks for answering my questions.

kevina · 2017-08-09T17:56:16Z

@Stebalien

We use a GetLinksDirect function here (and a few other places). Why do we do this? Was there any reason to bypass the LinkService or are we doing this because we didn't have a LinkService? @kevina?

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).

kevina · 2017-08-09T18:24:52Z

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 EnumerateChildren/EnumerateChildrenAsync takes in a function that gets the links for a node, not the node itself.

Now that I think about it a bit, GetLinksDirect may not be strictly necessary as the visit function could retrieve the node itself (to make sure its available), but that just adds extra work.

In any case I am not sure how the presence of the GetLinksDirect effects your refactoring as it is a helper function not directly connected to the DAGService.

Stebalien · 2017-08-11T20:44:31Z

In any case I am not sure how the presence of the GetLinksDirect effects your refactoring as it is a helper function not directly connected to the DAGService.

DAGService no longer has a GetLinks function (it's a helper function that takes a dag and a cid) so one has to construct a curried GetLinks.

kevina · 2017-08-11T20:58:50Z

DAGService no longer has a GetLinks function (it's a helper function that takes a dag and a cid) so one has to construct a curried GetLinks.

Oh, okay. Well I guess that is unavoidable as I hope we have established that EumerateChildren/EnumerateChildrenAsync should take a function that gets the links for a node, not the node itself.

whyrusleeping · 2017-08-17T02:45:20Z

@Stebalien This looks good to me, i'm fine to get this moving forward

Stebalien · 2017-08-18T00:26:00Z

(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.

kevina · 2017-08-18T01:01:12Z

@Stebalien I have not gone over this is with a fine-tooth comb, but I am also okay with moving forward on this.

Stebalien · 2017-09-17T03:49:32Z

This has served its purpose. I'll revive when I make progress on the linked PR.

Stebalien added 2 commits July 11, 2017 23:15

gx: update go-ipld-format

4316c97

License: MIT Signed-off-by: Steven Allen <[email protected]>

daviddias added the status/in-progress In progress label Jul 12, 2017

Stebalien requested review from kevina and whyrusleeping July 12, 2017 18:48

Stebalien commented Jul 12, 2017

View reviewed changes

Stebalien mentioned this pull request Jul 21, 2017

Extract dagservice and friends from go-ipfs ipfs/go-ipld-format#8

Merged

Stebalien closed this Sep 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Extract DAGService and friends #4063

[WIP] Extract DAGService and friends #4063

Stebalien commented Jul 12, 2017 •

edited

Loading

Stebalien left a comment •

edited

Loading

Stebalien Jul 12, 2017

Stebalien Jul 12, 2017

Stebalien Jul 12, 2017

Stebalien Jul 12, 2017

kevina Aug 18, 2017

Stebalien Aug 18, 2017

kevina commented Aug 9, 2017

kevina commented Aug 9, 2017

Stebalien commented Aug 11, 2017

kevina commented Aug 11, 2017

whyrusleeping commented Aug 17, 2017

Stebalien commented Aug 18, 2017

kevina commented Aug 18, 2017

Stebalien commented Sep 17, 2017

[WIP] Extract DAGService and friends #4063

[WIP] Extract DAGService and friends #4063

Conversation

Stebalien commented Jul 12, 2017 • edited Loading

Stebalien left a comment • edited Loading

Choose a reason for hiding this comment

Stebalien Jul 12, 2017

Choose a reason for hiding this comment

Stebalien Jul 12, 2017

Choose a reason for hiding this comment

Stebalien Jul 12, 2017

Choose a reason for hiding this comment

Stebalien Jul 12, 2017

Choose a reason for hiding this comment

kevina Aug 18, 2017

Choose a reason for hiding this comment

Stebalien Aug 18, 2017

Choose a reason for hiding this comment

kevina commented Aug 9, 2017

kevina commented Aug 9, 2017

Stebalien commented Aug 11, 2017

kevina commented Aug 11, 2017

whyrusleeping commented Aug 17, 2017

Stebalien commented Aug 18, 2017

kevina commented Aug 18, 2017

Stebalien commented Sep 17, 2017

Stebalien commented Jul 12, 2017 •

edited

Loading

Stebalien left a comment •

edited

Loading