-
Notifications
You must be signed in to change notification settings - Fork 16
Add support for converting badger2 datastore #26
Conversation
This feature allows the ipfs-ds-convert utility to convert to and from a badger2 datastore. This is a provisional PR and should not be merged until the following dependencies are merged into their master branches: - github.com/ipfs/go-ds-badger2 - github.com/ipfs/go-ipfs - github.com/ipfs/go-ipfs-config
cli_test.go
Outdated
|
||
func TestBasicConvertBadger2(t *testing.T) { |
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 I mentioned in #23 (comment) there aren't really any natural places to add Badger2 tests in the repo that are just copy-paste's of the Badger1 tests (aside from the short Validator tests). This is mostly because this library is mostly datastore agnostic and so it shouldn't be relevant.
The existing tests are:
- The CLI works (cli_test.go)
- The strategies work (strategy_test.go)
- Datastore configs look right (validate_test.go)
I think the right way to make this work is to just add a basic test that converts from all primitive datastores (flatfs, leveldb, memory, badger, badger2) to in memory leveldb and then back again.
You should be able to use t.Run to create the various subtests and utilize the testutils (e.g. testutil.PrepareTest and FinishTest) to generate the random keys and check their validity.
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.
Done, with the exception of memory datastore. While it makes sense to have a memory datastore for IPFS testing, does it makes sense to support conversion to/from a memory-only datastore? It would mean IPFS would need to be running during the conversion, which does not seem safe.
See TestCoverAll
in convert/convert_test.go
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 mentioned in my comment https://github.com/ipfs/ipfs-ds-convert/pull/26/files#r497842591, I think we could rework the tests here to be more helpful and also make it easier to figure out where to plug in tests for new datastores in the future.
I think |
@aschmahmann @gammazero I tried to convert default flatfs datastore from 0.8.0-rc2 to badger with ipfs-ds-convert and got:
Does it mean we need to land this PR before we ship 0.8.0? (ipfs/kubo#7707) |
Nope, Badger2 support is unrelated and won't be coming in go-ipfs v0.8.0. @lidel I think for that issue we just need to make a new release that bumps this constant https://github.com/ipfs/ipfs-ds-convert/blob/44820b97e01c6d6410cf5a9557d4d5ed93ab8220/repo/const.go#L8 to 11. Try a local build and give it a 👍 if that works for you, |
@aschmahmann yes, that did the trick. Tested with flatfs→badger in 0.8.0-rc2 and confirmed conversion works as expected: $ ipfs config profile apply badgerds
[..]
$ ./ipfs-ds-convert convert
convert 2021/02/18 22:06:58 Checks OK
convert 2021/02/18 22:06:58 Copying keys, this can take a long time
copied 37 keys
convert 2021/02/18 22:06:58 All data copied, swapping repo
convert 2021/02/18 22:06:58 Verifying key integrity
convert 2021/02/18 22:06:58 37 keys OK
convert 2021/02/18 22:06:58 Saving new spec
convert 2021/02/18 22:06:58 All tasks finished |
Closing due to #50 |
This feature allows the ipfs-ds-convert utility to convert to and from a badger2 datastore.
This is a provisional PR and should not be merged until the following dependencies are merged into their master branches: