Add Files API root as best-effort pin #2872

kevina · 2016-06-18T18:03:17Z

This fixes #2697 and #2698.

whyrusleeping · 2016-06-19T05:53:36Z

merkledag/merkledag.go

+		if !set.Has(k) {
+			set.Add(k)
+			child, err := ds.Get(ctx, k)
+			if err == ErrNotFound {


lets do something like

switch err { case ErrNotFound: continue default: return err case nil: break }

kevina · 2016-06-20T07:50:23Z

@whyrusleeping see my line note on your second comment. There are two versions for you to chose from.

Please do not merge this as is. I will squash this all down to one commit once we decide what works best.

whyrusleeping · 2016-06-20T20:20:28Z

@kevina i think the latter option of passing the array of best effort root keys is the better of the two for now. My other thought was to enumerate the best effort set before calling gc, and pass that into the gc call as a 'extra keys that arent garbage' parameter. But i think that might be a little more difficult and uglier overall

Closes ipfs#2697. Closes ipfs#2698. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

kevina · 2016-06-20T21:12:17Z

@whyrusleeping, okay, ready to merge once you are happy with it.

whyrusleeping · 2016-06-20T22:44:23Z

test/sharness/t0252-files-gc.sh

+test_expect_success "object not removed after gc" '
+  echo "hello world"  | ipfs files write --create /hello.txt &&
+  ipfs repo gc &&
+  ipfs cat QmVib14uvPnCP73XaCDpwugRuwfTsVbGyWbatHAmLSdZUS


we should also add another test_expect_success to make sure that /hello.txt is still accessible through the files api here

@whyrusleeping Added.

So... if this is "best effort", under what conditions should gc remove these, and can we make sure to test that here?

All "best effort" means is not to abort if a child is unreadable. For the GC to remove the object will need to be detached somehow from the MFS Root.

@kevina want to take this one on and add a few more tests?

Test that the read test below outputs the right text

Exercise a situation that uses the best effort pin:

Add a directory containing a file with --pin=false

pin the hash directly

run a gc so that file is no longer accessible

ipfs files cp <dirhash> /somewhere (this should work just fine)

ipfs repo gc should succeed

re-add the file that was originally in that directory with --pin=false

run ipfs repo gc again, that file shouldnt be removed.

OK Will do.

@kevina want to take this one on and add a few more tests?

@whyrusleeping See #3151

…pin. License: MIT Signed-off-by: <[email protected]>

whyrusleeping · 2016-06-21T16:46:08Z

This LGTM. @Kubuxu wanna take a look over too?

Kubuxu · 2016-06-21T19:09:04Z

SGTM

jbenet · 2016-08-27T09:56:18Z

pin/gc/gc.go

@@ -69,7 +70,7 @@ func GC(ctx context.Context, bs bstore.GCBlockstore, pn pin.Pinner) (<-chan key.
 	return output, nil
 }

-func Descendants(ctx context.Context, ds dag.DAGService, set key.KeySet, roots []key.Key) error {
+func Descendants(ctx context.Context, ds dag.DAGService, set key.KeySet, roots []key.Key, bestEffort bool) error {


i strongly dislike how this is implemented. Adding bestEffort here to this function ruins the whole abstraction. You can implement this "best effort" notion outside of this function, just in the GC function.

@jbenet in this function (Descendants) and EnumerateChildren all that bestEffort means is to not abort if a child is unreadable. In order to collect the set of keys to keep on a "best effort" bases I needed a way to to collected all the children of the initial best effort set that are local on the node.

I am not entirely clear what abstraction I am ruining so I am not 100% sure how to fix this. I could create a new function to find the Descendants of a keySet that doesn't abort on an error, but to me it seamed better to extend the functionally of the existing function.

@whyrusleeping I could appreciate any insights you can offer.

jbenet · 2016-08-27T09:59:54Z

The idea of "best effort" pins is very good and we already want to incorporate it into the pinning model.
The idea of making the files api "best effort" makes sense. it solves the problems we had with making it proper full pins.
That said, the way this PR is implemented is not great. It muddles the abstractions, threading in a "best effort" flag into things, instead of just treating that set of roots as just another set to mark and consider.
It's also not clear when GC will actually remove these "best effort" pins.
This is not good quality, and I'm sad this merged as is.
To be clear, the idea is very good, and we SHOULD have such an abstraction. It should be implemented better though.

kevina · 2016-08-28T01:44:25Z

So to be clear all "best effort" means (as agreed to by @whyrusleeping and @Kubuxu) is to simply keep all children of the best effort pin that are reachable from the local node. (In other words if a child in unavailable simply skip it, rather than aborting or attempting to download it). Nothing More. The GC will never remove "best effort" pins.

jbenet · 2016-08-29T04:34:48Z

(Thinking out loud)

Oh wow, that means something totally different, then. "best effort" in computer science tends to mean "up to best abilities, but not unreliable". Specifically, "best effort" in networks. Applied to pins, i took it to mean "pin a subgraph while you can, but GC if you need to", sort of like a "gc these last" type of thing.

What you describe ("pin the graph objects currently present in the node")

It's a bit concerning, too because it has unpredictable guarantees and consequences, in a very different way than "unreliable (best effort)"-- because how much is pinned forever depends entirely on order of events, which is very hard to reason about.

What happens if more objects in that graph are downloaded, as a result of other operations? are they suddenly pinned too? forever? and why forever?
It seems to me this is trying to approximate some use case that is better served by other mechanisms. For example, i might "pin local objects" that are part of a VM i want to boot, leaving the rest of the objects to be fetched later and gc-ed differently (i.e. i want to pin only the critical parts). Or similarly, I might pin the "index sub graph" and first few objects in a video, so i can always start playing it quickly and seek quickly, but let the retrieval of the big data blocks to when they're needed.
Though in these use cases, i dont actually want the mechanism you describe, i want a different pin which can be configured to pin a chosen subgraph (i.e. what i really want is a pin with an associated IPLD selector).

As it is now, I think your "objects that are currently local" pins can be turned into a other tools, and use regular pin sets:

first, create a tool that list all local refs for a graph. eg. ipfs refs local [-r] <root> or ipfs refs [-r] --local <root> which would list out all refs from <root> available locally. Can pin those directly.
Potentially create a recursive pin with a depth parameter (again, covered by IPLD selectors later), so all you need to do is create one object that points to all the local refs you listed, and pin that with the appropriate depth.

Thinking about it another way, let's rename "best effort" into "incremental". Because that's what it really is, right? it's an incremental pin that over time will pin more and more of a subgraph, as it's accessed and retrieved. This has different use cases than i mentioned above (eg "incrementally pin" all my local maps' area in a google maps like thing, or "incrementally pin" a dataset i'm exploring, or a dungeon in a dungeon crawler). Is this more what you mean?

Would be very useful to me to hear what use cases you have in mind for this.

kevina · 2016-08-29T06:03:38Z

@jbenet, please see #2698 for context. @whyrusleeping actually came up with the name "best effort" and I just used it.

whyrusleeping · 2016-08-30T15:05:36Z

@jbenet this isnt a separate pin type. All this is is a way to make the files api space not break when you run a GC, but also not require that everything in the files API be local.

whyrusleeping reviewed Jun 19, 2016
View reviewed changes

kevina force-pushed the best-effort-pins branch from 848b950 to ed601f6 Compare June 20, 2016 06:15

kevina changed the title ~~Add Files API root as best-effort pin~~ [WIP] Add Files API root as best-effort pin Jun 20, 2016

kevina force-pushed the best-effort-pins branch from 174d2ce to dce70d1 Compare June 20, 2016 21:08

Add Files API root as best-effort pin.

714f2de

Closes ipfs#2697. Closes ipfs#2698. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>

kevina force-pushed the best-effort-pins branch from dce70d1 to 714f2de Compare June 20, 2016 21:10

kevina changed the title ~~[WIP] Add Files API root as best-effort pin~~ Add Files API root as best-effort pin Jun 20, 2016

whyrusleeping reviewed Jun 20, 2016
View reviewed changes

Enhance tests and fix tests copyright for files API root best-effort …

466d709

…pin. License: MIT Signed-off-by: <[email protected]>

kevina force-pushed the best-effort-pins branch from 4e80d91 to 466d709 Compare June 21, 2016 01:20

whyrusleeping added this to the Ipfs 0.4.3 milestone Jun 21, 2016

whyrusleeping merged commit 472deb0 into ipfs:master Jun 22, 2016

kevina deleted the best-effort-pins branch June 22, 2016 05:38

kevina mentioned this pull request Aug 20, 2016

Handling "broken" pins. #3069

Closed

jbenet reviewed Aug 27, 2016
View reviewed changes

jbenet mentioned this pull request Aug 28, 2016

Code Review 0.4.2..0.4.3-rc3 #3138

Closed

58 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Files API root as best-effort pin #2872

Add Files API root as best-effort pin #2872

kevina commented Jun 18, 2016 •

edited

Loading

whyrusleeping Jun 19, 2016 •

edited

Loading

kevina commented Jun 20, 2016

whyrusleeping commented Jun 20, 2016

kevina commented Jun 20, 2016

whyrusleeping Jun 20, 2016

kevina Jun 21, 2016

jbenet Aug 27, 2016

kevina Aug 28, 2016

whyrusleeping Aug 29, 2016

kevina Aug 29, 2016

kevina Aug 30, 2016 •

edited

Loading

whyrusleeping commented Jun 21, 2016

Kubuxu commented Jun 21, 2016

jbenet Aug 27, 2016

kevina Aug 28, 2016

jbenet commented Aug 27, 2016 •

edited

Loading

kevina commented Aug 28, 2016

jbenet commented Aug 29, 2016 •

edited

Loading

kevina commented Aug 29, 2016

whyrusleeping commented Aug 30, 2016

Add Files API root as best-effort pin #2872

Add Files API root as best-effort pin #2872

Conversation

kevina commented Jun 18, 2016 • edited Loading

whyrusleeping Jun 19, 2016 • edited Loading

Choose a reason for hiding this comment

kevina commented Jun 20, 2016

whyrusleeping commented Jun 20, 2016

kevina commented Jun 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevina Aug 30, 2016 • edited Loading

Choose a reason for hiding this comment

whyrusleeping commented Jun 21, 2016

Kubuxu commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented Aug 27, 2016 • edited Loading

kevina commented Aug 28, 2016

jbenet commented Aug 29, 2016 • edited Loading

kevina commented Aug 29, 2016

whyrusleeping commented Aug 30, 2016

kevina commented Jun 18, 2016 •

edited

Loading

whyrusleeping Jun 19, 2016 •

edited

Loading

kevina Aug 30, 2016 •

edited

Loading

jbenet commented Aug 27, 2016 •

edited

Loading

jbenet commented Aug 29, 2016 •

edited

Loading