Please note that we also have the notion of a "best effort" pin used to pin anything off the files root. It works by keeping the 'GC' from removing anything under it but the GC won't fail if one of the children can not be found. I am thinking we should just make that pin type explicit.

@hsanjuan does that type of pin not meet your needs?

@whyrusleeping what do you think?

Also if we go trough we this, I want to avoid the special case when the recursion is 0 by defining a direct pin as a recursive pin with a depth of 0.

@hsanjuan does that type of pin not meet your needs?

I don't think it does, honestly I don't fully understand it. I need go-ipfs to NOT fetch all the subtree.

Also if we go trough we this, I want to avoid the special case when the recursion is 0 by defining a direct pin as a recursive pin with a depth of 0.

That's doable, happy to do it.

I don't think it does, honestly I don't fully understand it. I need go-ipfs to NOT fetch all the subtree.

Creating a best-effort pin won't fetch anything. It will simply prevent any subtrees already fetched from being garbage collected.

edit: (not a cancel comment button...)

@kevina I'm thinking that deleting Direct pins as a fully different set might have some performance impact (in order to keep current APIs), as direct pins will need to be extracted from the recursive set by filtering them out, thus listing direct pins may become a slower operation if the recursive set is very big (something that doesn't happen now). Is this something acceptable?

Creating a best-effort pin won't fetch anything. It will simply prevent any subtrees already fetched from being garbage collected.

I need to fetch partial subtrees too.

I need to fetch partial subtrees too.

I am thinking this should be implemented directly without complicating the pinier or the gc. You can then create a best-effort pin to keep them from getting GC.

However, I am not really against this idea if it will be useful in other contextes. @Stebalien @whyrusleeping what do you think.

@kevina I'm thinking that deleting Direct pins as a fully different set might have some performance impact (in order to keep current APIs), as direct pins will need to be extracted from the recursive set by filtering them out, thus listing direct pins may become a slower operation if the recursive set is very big (something that doesn't happen now). Is this something acceptable?

I would think so. But others may disagree.

@Kubuxu @whyrusleeping can I get some attention on this?

@Stebalien maybe you can help with this?

Let's go back to the issue and discuss ways to do this right. While this technically addresses the immediate need, it won't solve the long-term issue of needing more flexible ways to specify pins and it abuses an enum to store a number. Really, we need a more flexible pinset that allows for complex pin policies.

@Stebalien I am a bit dissapointed it took 1 month to get a full frontal rejection with little alternative proposal. I understand that the "right" way to do this:

• Is a larger change
• Touches more critical sections
• Is all unclear at this point
• Would throw away all the hacks in the current pinning system (mine, and previously existing) anyways

My request is clearly spec-ed and implementable within the current state of things. So instead of deferring to larger abstract change, I would like to get a list of concrete steps that we can take to move forward, possibly in parallel with a the long total-revamp-of-the-pin-system discussion.

For example, this change, provides you with a bunch of areas to discuss on already that would also clarify parts in the larger discussion:

• There are a bunch of functions depth-first-traversing the DAG. It does not seem crazy to update them with a depth limit as I did. The fact that there are 4 or 5 of these, means more lines of code. The bulk of the change is this. Are we against that? This can be PRed separately.
• There is the commands part, with flags defining an API and expected behaviour that we can agree on or not. What would be response format for depth-limited pins? Extra key in the response object? Or are we ok with type "recursive%d"?
• The package in thirdparty should be moved out
• There is a "enum abuse". Or extending the recursive types to account for arbitrary max-depths. This is a hack. What would be the alternative? It needs to keep the API compatible. I did not come up with a way of doing it in less amounts of code, or touching less critical sections, but yes it's a hack. We can add a new single MaxDepth pin type, but we'll have to make sure we carry not just the type but metadata associated to it (the actual depth) all around.
• Finally, the last part of the change affects how the new pins are stored in disk. Again, cannot think how to do it in less amounts of code or touching less critical paths. Since most --max-depth values will be 1 or 2, it made sense to do it like this.

Despite the critic, the change does not get much in the way of the current pinning system (it does not change the logic, the api, the storage format) and the standard pinning path.

There is another way of approaching this too. I only need max-depth 1 and 2 (or maybe even reducing to just 1). I can just support those values, introducing 2 specific pin types and potentially reducing the parts of the "hack" to support arbitrary pin depths.

Is there a way of doing this now that you'd consider workable?

I am a bit dissapointed it took 1 month to get a full frontal rejection with little alternative proposal.

It's a large patch, touches a bunch of critical code, hasn't been flagged as a priority, and the design wasn't discussed at all before implementing. I don't even look at patches like this until I have some time to actually think about them.

little alternative proposal

I don't know enough about the current pin system to give one off the top of my head.

Despite the critic, the change does not get much in the way of the current pinning system (it does not change the logic, the api, the storage format) and the standard pinning path.

This patch adds a cluster-specific, weird addition to pinning by hacking it into the the existing pin system with no discussion. Whatever feature we end up adding to support depth-limited pins, we'll have to maintain it.

The primary issue here is the lack of discussion and/or context. You should probably read:

• Store pins in MFS #4675
• Named pins & pins stored in datastore #4757
• Moving away from current pin system #4763

There are a bunch of functions depth-first-traversing the DAG. It does not seem crazy to update them with a depth limit as I did. The fact that there are 4 or 5 of these, means more lines of code. The bulk of the change is this. Are we against that? This can be PRed separately.

It should be possible to achieve the same thing by passing the depth to the visit function (can create a new EnumerateChildrenWithDepth). EnumerateChildrenWithDepth shouldn't have to care about the max depth (the visit function should just return false when it hits the max depth).

Whatever we do, those functions shouldn't talk about pinning or use types like RecPin. That'll just confuse readers.

There is the commands part, with flags defining an API and expected behaviour that we can agree on or not. What would be response format for depth-limited pins? Extra key in the response object? Or are we ok with type "recursive%d"?

Personally, I'd give them the type "partial" and then add a "max-depth" field, or something like that.

The package in thirdparty should be moved out.

Yes.

There is a "enum abuse". Or extending the recursive types to account for arbitrary max-depths. This is a hack. What would be the alternative? It needs to keep the API compatible. I did not come up with a way of doing it in less amounts of code, or touching less critical sections, but yes it's a hack. We can add a new single MaxDepth pin type, but we'll have to make sure we carry not just the type but metadata associated to it (the actual depth) all around.

We (and cluster) will need more complex pins anyways.

Finally, the last part of the change affects how the new pins are stored in disk. Again, cannot think how to do it in less amounts of code or touching less critical paths. Since most --max-depth values will be 1 or 2, it made sense to do it like this.

The values will usually be 1 or 2 for cluster.

@hsanjuan @Stebalien I attempted to start a discussion about alternative ways to solve this problem, but it seams I was ignored.

In particular it would likely be better to enhance our best-effort pins and then fetch the needed subtrees separately.

In particular it would likely be better to enhance our best-effort pins and then fetch the needed subtrees separately.

We just need to make sure that fills the need. If we do that, we won't end up removing any accidentally downloaded nodes.

Let's move the discussion back to the issue.

@hsanjuan let's do the depth limited recursion stuff in a separate PR, and discuss pinning improvements where @kevina and @Stebalien are mentioning.

Sorry for the wait, but let's sketch this out a bit more before moving forward.

@Stebalien

This patch adds a cluster-specific, weird addition to pinning by hacking it into the the existing pin system with no discussion. Whatever feature we end up adding to support depth-limited pins, we'll have to maintain it.

This is the discussion. This PR is here to kickstart a discussion: how to support this feature in the current pin system. It was told to me (offline) that I could attempt to do this in the current pin system until the whole MFS thing is figured out. Now you have a bunch of concrete stuff to criticize and propose improvements on, a list of all the pieces that are potentially touched and the minimal approach to it (hacky, yes, but minimal). Sorry but I thought actually understanding the current pin system and propose how to change it was a better approach than nicely asking to someone else to write my feature.

That said, I'll gladly maintain it too whatever the outcome is. Sorry I don't like the stress on the "we'll have to maintain it". It leaves me with very mixed feelings about what you're implying and makes me a little sad.

It should be possible to achieve the same thing by passing the depth to the visit function (can create a new EnumerateChildrenWithDepth). EnumerateChildrenWithDepth shouldn't have to care about the max depth (the visit function should just return false when it hits the max depth).

I think you cannot shortcut branches from the search by knowing only the current depth. You need to know how deep you explored the last time you visited the CID (I tried that way first). At least I could not figure out how to do it just with depth while at the same time not re-exploring branches.

Whatever we do, those functions shouldn't talk about pinning or use types like RecPin. That'll just confuse readers.

Happy to rename.

We (and cluster) will need more complex pins anyways.

The values will usually be 1 or 2 for cluster.

That's fair to say, but since cluster is requesting this feature and is very limited in scope compared to "complex pins", it should make sense that it works for well for the cluster usecase. It can always be improved in the future as more use cases and needs appear. Perhaps having added a single "only children/maxdetph=1" pin type would have been a better approach to this. (as a side not, it would be interesting to hear use-cases of people pinning large amounts of pins all with different depths limits, but I agree this should eventually work well).

@whyrusleeping

@hsanjuan let's do the depth limited recursion stuff in a separate PR

Sure, no problem.

This is the discussion.

This PR was couched as a finished product. No WIP, "proposal", open design questions, "should we do this", etc. and the remaining TODOs are "write more tests" and "implement refs -r --max-depth". That's why I reacted the way I did. A fair amount of your time was wasted writing this patch and some (although significantly less) of my time was wasted reviewing it.

That said, I'll gladly maintain it too whatever the outcome is. Sorry I don't like the stress on the "we'll have to maintain it". It leaves me with very mixed feelings about what you're implying and makes me a little sad.

By "we", I meant all of us. My point was that this is a group effort. And no, you won't (and shouldn't) handle every bug report possibly related to this PR.

It should be possible to achieve the same thing by passing the depth to the visit function (can create a new EnumerateChildrenWithDepth). EnumerateChildrenWithDepth shouldn't have to care about the max depth (the visit function should just return false when it hits the max depth).

I think you cannot shortcut branches from the search by knowing only the current depth. You need to know how deep you explored the last time you visited the CID (I tried that way first). At least I could not figure out how to do it just with depth while at the same time not re-exploring branches.

Correct. As this patch currently does, the visit function would have to remember how deep it was when it last explored a CID.

However, that means that you can:

1. Only track the current depth in EnumerateChildrenWithDepth function.
2. Decide whether or not to go deeper in the visit function (based on the current depth, the max depth (local to the visit function), and the lowest depth at which we've explored the current CID (visit function state).

@hsanjuan The way I see it, the main sticking point around 'maintaining' this is whether or not you're okay breaking it when we come up with something better.

Upgrading pinning to something better will require a migration anyways, so thats not too big of an issue, however, what we have to make sure of is: Are we confident the future solution will support exactly this behavior? One potential thing I see being problematic is if we switch to ipld selectors, and that makes more sense for cluster to use, then we might get stuck maintaining a feature we really don't need anymore because someone else might start using it.

Let's see how this PR looks without all the depth limited traversal logic, and revisit the design then.

Decide whether or not to go deeper in the visit function (based on the current depth, the max depth (local to the visit function), and the lowest depth at which we've explored the current CID (visit function state).

Yes, ok that works too and I see the advantage now. Thanks for taking the time to explain.

Are we confident the future solution will support exactly this behavior?

As long as there's an equivalent way of doing things, I'm happy with breaking changes (but this opens a bad, or weird precedent at least, even if it's good for me). Anyway, as you said, let's do the things we agree on first and revisit the discussion.

Is this going to be revisited in the future?

From what I understand the ability to pin subsections of graphs is blocking cluster shipping their feature of splitting an unreasonably large DAG across multiple IPFS nodes - this would very much help package manager maintainers increase availability of their registries if stored on IPFS.

@achingbrain hopefully as this is still blocking the sharding functionality in IPFS Cluster.

Is there any news/progress on this issue over the past few months? Whilst I don't was to be 'that guy' who asks others to implement features, ipfs-cluster is currently pretty useless for clusters over a few TB with large files inside due to the requirement to replicate entire files.

Whilst likely not possible for a while due to the need for me to familiarise myself with the technicalities of IPFS under the hood and the codebase, if there are no plans to revisit this in the near future, I will try to start learning about IPFS and thinking about some of the problems in previous comments, with a view to hopefully writing this feature in X months time. Though whilst considering that option, there seem to be a few mentions that the entire pin system is due for a refactor - is this happening anytime soon, as it makes thinking about an implementation for this feature harder if this is the case.