-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add automatic sharding/unsharding tests #8547
Add automatic sharding/unsharding tests #8547
Conversation
9a0c4a5
to
fd55804
Compare
098b2bc
to
927fad8
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.
t0260 LGTM, left minor comments but nothing blocking.
test_add_large_dir "$UNSHARDED" | ||
|
||
test_expect_success "force sharding off" ' | ||
ipfs config --json Internal.UnixFSShardingSizeThreshold "\"1G\"" |
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.
The 0 value disables the options.
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.
ya that's fair although, I'm not sure how I feel about 0
here. I'm not even sure if when go-ipfs processes the config file it should accept 0 as a value. It restores behavior to the go-ipfs <v0.10.0 defaults, but I'm not sure it's obvious to reason about (i.e. the feature is turned off which means data is add as a basic directory and no automatic sharding or unsharding of existing MFS directories).
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.
no problem, just flagging that theoretically at least we expose a "disabled" option in UnixFS, but 1G is more than enough to consider it disabled for all intended purposes
test/sharness/t0260-sharding.sh
Outdated
@@ -137,4 +155,51 @@ test_list_incomplete_dir() { | |||
|
|||
test_list_incomplete_dir | |||
|
|||
# Test automatic sharding and unsharding |
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.
As discussed informally in chat (but can elaborate more here if needed) any kind of directory manipulation goes through the same "automatic" sharding mechanism, whether it is adding/removing one entry at a time from an MFS tree with ipfs files add/rm
, or consuming an entire directory at once with ipfs add -r <big-dir>
.
So above and below this line we are testing the same UnixFS code. This doesn't mean that the current division is superfluous as we're accessing that mechanism through different go-ipfs
paths but I'm mentioning this to (a) check if this was indeed the intention here and (b) think if we could re-label this boundary.
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.
True, but in the event the code paths were different we'd be testing different things. i.e. here we are testing that when we remove data from a sharded directory we get an unsharded one and when we add the data back we end up with a sharded one again.
Does that make sense?
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.
Yes, it does. I think my main issue here is with the "automatic" boundary, which might imply the previous commands were not, that they didn't use the new UnixFS mechanism and instead were forced or explicitly stated to shard (where in fact we calculated the exact entries number that would go over the threshold and trigger the mechanism). I think your previous explanation is more clear (but still we're in the nit realm so we can ignore this):
here we are testing that when we remove data from a sharded directory we get an unsharded one and when we add the data back we end up with [the same] sharded one again
* test: add tests for automatic sharding and unsharding * refactor: changed some naming in the sharding sharness tests to make more sense Co-authored-by: Adin Schmahmann <adin.schmahmann@gmail.com>
…xts (#8563) * plumb through go-datastore context changes * update go-libp2p to v0.16.0 * use LIBP2P_TCP_REUSEPORT instead of IPFS_REUSEPORT * use relay config * making deprecation notice match the go-ipfs-config key * docs(config): circuit relay v2 * docs(config): fix links and headers * feat(config): Internal.Libp2pForceReachability This switches to config that supports setting and reading Internal.Libp2pForceReachability OptionalString flag * use configuration option for static relays * chore: go-ipfs-config v0.18.0 https://github.com/ipfs/go-ipfs-config/releases/tag/v0.18.0 * feat: circuit v1 migration prompt when Swarm.EnableRelayHop is set (#8559) * exit when Swarm.EnableRelayHop is set * docs: Experimental.ShardingEnabled migration This ensures existing users of global sharding experiment get notified that the flag no longer works + that autosharding happens automatically. For people who NEED to keep the old behavior (eg. have no time to migrate today) there is a note about restoring it with `UnixFSShardingSizeThreshold`. * chore: add dag-jose code to the cid command output * add support for setting automatic unixfs sharding threshold from the config * test: have tests use low cutoff for sharding to mimic old behavior * test: change error message to match the current error * test: Add automatic sharding/unsharding tests (#8547) * test: refactored naming in the sharding sharness tests to make more sense * ci: set interop test executor to convenience image for Go1.16 + Node * ci: use interop master Co-authored-by: Marcin Rataj <lidel@lidel.org> Co-authored-by: Marten Seemann <martenseemann@gmail.com> Co-authored-by: Marcin Rataj <lidel@lidel.org> Co-authored-by: Gus Eggert <gus@gus.dev> Co-authored-by: Lucas Molas <schomatis@gmail.com>
Merge sharness tests from #8114 into #8527.
Closes #8114.