Add DAGService.GetLinks() method and use it in the GC and elsewhere. by kevina · Pull Request #3255 · ipfs/kubo

kevina · 2016-09-24T17:52:36Z

This method will use the (also new) LinkService if it is available to
retrieving just the links for a MerkleDAG without necessary having to
retrieve the underlying block.

For now the main benefit is that the pinner will not break when a block
becomes invalid due to a change in the backing file. This is possible
because the metadata for a block (that includes the Links) is stored
separately and thus always available even if the backing file changes.

For context (and test cases) please see the original commits: 5a73226 8b396fd

kevina · 2016-09-24T17:53:20Z

Part of #2634.

whyrusleeping · 2016-09-26T02:39:45Z

It doesnt appear that the linkservice interface has anything that implements it, and doesnt appear to be used anywhere

kevina · 2016-09-26T03:07:11Z

Right. It doesn't have a use yet outside of the filestore. This commit has an implementation 5a73226.

kevina · 2016-09-28T06:16:08Z

@whyrusleeping, if #2634 is going to get merged within a reasonable amount of time, I am okay with this P.R. not getting merged at the same time as #2634. I mainly separated it out for easier reviewing.

whyrusleeping · 2016-09-30T00:27:38Z

@kevina I think that we should change the linkservice interface so that the existing dagservice implements it. That way any thing that just needs a link service can still have the dag service passed to it. And any place that needs both should just have the dagservice passed in. Then, for the filestore, we can make a small wrapper around the current dagservice code that overrides the GetLinks method to use the filestore backing instead.

That way, it makes it easy for us to do other things later, like plug in a graph database to help resolve links.

whyrusleeping · 2016-09-30T18:29:28Z

So what i'm getting at here, is that we want the linkservice and the dagservice to ideally be exposed via the same object. Where the linkservice interface is just a limited subset of the dagservices interface. With them separate, a few things start to feel weird. I would never want to call GetLinks on the dagservice if i have a linkservice would I? but now i have to check if i have a linkservice, then if not, call out to the dagservice.

So for the purpose of this PR, we should just be adding the linkservice interface with GetLinks, and adding that same method to the existing dagservice interface. That way we can pass the dagservice to any function that requires only a linkservice. Later, when we have different linkservice implementations, we can make a wrapper object for the dagservice like:

type FancyDagServiceWithLinkMagic struct {
    DAGService
    linkStuff *linkResolver
}

func (a *FancyDagServiceWithLinkMagic) GetLinks(....) (Link, error) {
    return a.linkStuff.GetLinks(...)
}

And upon node construction, use that object as the nodes DAG. That way everbody basically gets the extra fancy link resolver boost for free.

kevina · 2016-10-01T00:49:13Z

@whyrusleeping @Kubuxu I just pushed a commit that implemented what we seamed to agree on over IRC.

whyrusleeping

I'm still not sold on the GetOffline thing. I think that its probably okay to just expect the user to pass the right thing in. I get the appeal of having the type system check that for us, but i think it might be cluttering the interfaces a bit too much.

cc @Kubuxu @diasdavid @jbenet for their thoughts as well

whyrusleeping · 2016-10-03T22:22:21Z

merkledag/merkledag.go

+	return node.Links, nil
+}
+
+func (n *dagService) GetOfflineLinkService() LinkService {


I think this can be moved to the main DAGService interface. There are many situations where we would want to use this to get an offline dagservice.

Look at the return type.

Right, but the actual implementation is returning an offline DAGService, which is something that we would want to have available.

I'm thinking that when we need an offline linkservice, we could call dag.GetOffline() (which would return a DAGService) and then simply pass that into the function requiring the offline linkservice.

I did it this way to enforce type safety. Otherwise we could be passed a LinkService and get a DAGService. If you really don't like that I can change it.

Type safety is good, lets move forward with this

whyrusleeping · 2016-10-03T22:23:12Z

merkledag/merkledag.go

 // TODO: parallelize to avoid disk latency perf hits?
-func EnumerateChildren(ctx context.Context, ds DAGService, root *Node, visit func(*cid.Cid) bool, bestEffort bool) error {
-	for _, lnk := range root.Links {
+func EnumerateChildren(ctx context.Context, ds LinkService, links []*Link, visit func(*cid.Cid) bool, bestEffort bool) error {


I would prefer this just take a since *cid.Cid as its input instead of an array of links. Makes it easier to call in general.

Okay, I will look into it.

whyrusleeping · 2016-10-03T22:25:12Z

pin/gc/gc.go


-	bsrv := bserv.New(bs, offline.Exchange(bs))
-	ds := dag.NewDAGService(bsrv)
+	ls = ls.GetOfflineLinkService()


Ah, i see why this was on the LinkService and not the DAGService. Hrm... thats tricky.

whyrusleeping · 2016-10-03T22:26:45Z

pin/gc/gc.go


-		// EnumerateChildren recursively walks the dag and adds the keys to the given set
-		err = dag.EnumerateChildren(ctx, ds, nd, func(c *cid.Cid) bool {
+		// EnumerateChildren recursively walks the dag and adls the keys to the given set


Yes, will fix the comment.

whyrusleeping · 2016-10-03T22:28:52Z

pin/pin.go


-func hasChild(ds mdag.DAGService, root *mdag.Node, child key.Key) (bool, error) {
-	for _, lnk := range root.Links {
+func hasChild(ds mdag.LinkService, links []*mdag.Link, child key.Key) (bool, error) {


Should accept a *cid.Cid instead of []*mdag.Link

Okay, will look into it.

whyrusleeping · 2016-10-06T01:44:57Z

exchange/bitswap/bitswap.go

 	return out
 }
+
+func (bs *Bitswap) IsOnline() bool {


It doesnt look like we use this anywhere. Am i missing something or is this just dead code from a different approach?

It is used. I added a IsOnline() method to exchange to be able to tell if the exchange is online or not.

Ah, yeah. I missed it on my previous pass through

whyrusleeping · 2016-10-06T04:35:50Z

Alright, I think this looks good to me. @Kubuxu wanna give it a good look over?

whyrusleeping · 2016-10-06T04:37:47Z

core/core.go

-	Reporter   metrics.Reporter
-	Discovery  discovery.Service
-	FilesRoot  *mfs.Root
+	Peerstore   pstore.Peerstore     // storage for other Peer instances


Could you try an undo these changes? its just noise on the diff

Yes. I was just about to re-base to clean up this diff noise. 😄

This method will use the (also new) LinkService if it is available to retrieving just the links for a MerkleDAG without necessary having to retrieve the underlying block. For now the main benefit is that the pinner will not break when a block becomes invalid due to a change in the backing file. This is possible because the metadata for a block (that includes the Links) is stored separately and thus always available even if the backing file changes. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

Instead make LinkService a part of DAGService. The LinkService is now simply an interface that DAGService implements. Also provide a GetOfflineLinkService() method that the GC uses to get an offline instance. License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

…g.Link Author: Kevin Atkinson <k@kevina.org> Fix EnumerateChildren & hasChild to take a *cid.Cid instead of []*mdag.Link Author: Jeromy Johnson <why@ipfs.io> make FetchGraph use a cid pin: fix TestPinRecursiveFail License: MIT Signed-off-by: Jeromy <why@ipfs.io> License: MIT Signed-off-by: Kevin Atkinson <k@kevina.org>

kevina · 2016-10-06T15:57:43Z

Just rebased on master and assuming the tests pass this should be good to go. @Kubuxu what do you think?

whyrusleeping

LGTM

Kubuxu added the status/in-progress In progress label Sep 24, 2016

kevina mentioned this pull request Sep 24, 2016

[WIP] Filestore Implementation #2634

Closed

6 tasks

kevina force-pushed the kevina/getlinks branch from 439da7c to 0c0959a Compare September 24, 2016 18:06

whyrusleeping assigned whyrusleeping and Kubuxu Sep 25, 2016

whyrusleeping requested changes Oct 3, 2016

View reviewed changes

kevina force-pushed the kevina/getlinks branch from 723a743 to b315144 Compare October 4, 2016 03:22

whyrusleeping reviewed Oct 6, 2016

View reviewed changes

kevina force-pushed the kevina/getlinks branch 2 times, most recently from 2697850 to f0e5138 Compare October 6, 2016 05:04

kevina force-pushed the kevina/getlinks branch from e491d52 to 3899194 Compare October 6, 2016 15:43

kevina added 2 commits October 6, 2016 11:52

kevina force-pushed the kevina/getlinks branch from baad64b to 721df36 Compare October 6, 2016 15:56

kevina mentioned this pull request Oct 6, 2016

bitswap: redo: don't re-provide blocks we've provided very recently #3253

Closed

whyrusleeping approved these changes Oct 8, 2016

View reviewed changes

whyrusleeping merged commit 2fd045f into master Oct 8, 2016

whyrusleeping deleted the kevina/getlinks branch October 8, 2016 18:45

whyrusleeping removed the status/in-progress In progress label Oct 8, 2016

kevina added this to the Filestore implementation milestone Oct 19, 2016

ghost mentioned this pull request Dec 23, 2016

Code Review v0.4.4...v0.4.5 #3534

Closed

kevina mentioned this pull request Aug 19, 2017

[RFC] Add an offline hint to contexts #4009

Closed

Uh oh!

Conversation

kevina commented Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevina commented Sep 24, 2016

Uh oh!

whyrusleeping commented Sep 26, 2016

Uh oh!

kevina commented Sep 26, 2016

Uh oh!

kevina commented Sep 28, 2016

Uh oh!

whyrusleeping commented Sep 30, 2016

Uh oh!

whyrusleeping commented Sep 30, 2016

Uh oh!

kevina commented Oct 1, 2016

Uh oh!

whyrusleeping left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whyrusleeping commented Oct 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevina commented Oct 6, 2016

Uh oh!

whyrusleeping left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevina commented Sep 24, 2016 •

edited

Loading