-
Notifications
You must be signed in to change notification settings - Fork 129
ipld/unixfs/io: MaxLinks and MaxHAMTFanout for directories #897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
(This is a supporting PR to being able to set dag-width in Kubo). |
2a0a3f5
to
fb96ebb
Compare
288fddd
to
596680d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic all looks good, tests too. Could only find some trivial code nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slight concern that WithMaxLinks
alone does not allow users to hardcode current implicit defaults in this and other implementations, which is 174 or 1024 for Basic and 256 for HAMT fanout – comment inline.
WithMaxWidth() -------------- The new option sets effectively a maximum width to the directories. Currently, on a dynamic directory, the switching to-from Basic to HAMT is controlled by HAMTShardingSize, and the width of the HAMT shards by DefaultShardWidth. When WithMaxLinks() is specified, the switching is additionally controlled by the number of links exceeding the limit. In that case, MaxLinks is used as ShardWidth. The directory can only be converted back to BasicDirectory when the total number of links is below the limit. Backwards compatibility is kept and tests have been added. Note that when setting MaxLinks to a high number, it is possible that we still suffer automatic conversion to HAMT before hitting MaxLinks, if the estimated directory size is above 256KiB (as before). WithStat() ---------- Allows to set Stat on a new directory. WithCidBuilder() --------------- Allows to set the CidBuilder on a new directory.
596680d
to
562f0e5
Compare
New review requested. Changes addressed.
Let's continue on #906, as it has additionally touched the unixfs part a bit. |
This PR paves the way to support controlling and limiting the max number of links a directory can have by supporting option-based limitations on the unixfs directory implementation.
The result is that we can set a maximum number of links in Dynamic folders that will trigger conversion to HAMT, which has in turn it's own Fanout option.
This PR has a follow up at #906 .