Skip to content
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 support for converting badger2 datastore #26

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gammazero
Copy link
Contributor

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

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
Comment on lines 56 to 57

func TestBasicConvertBadger2(t *testing.T) {
Copy link
Contributor

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:

  1. The CLI works (cli_test.go)
  2. The strategies work (strategy_test.go)
  3. 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.

Copy link
Contributor Author

@gammazero gammazero Oct 1, 2020

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

Copy link
Contributor

@aschmahmann aschmahmann left a 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.

@coveralls
Copy link

coveralls commented Oct 1, 2020

Coverage Status

Coverage increased (+0.1%) to 77.834% when pulling e388605 on feat/badger2 into 44820b9 on master.

@gammazero
Copy link
Contributor Author

... 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 TestCoverAll in convert/convert_test.go does this for adding conversion tests: add a spec file in testfiles/ and add a test case to TestCoverAll. That second step could be eliminated by iterating spec files in a directory and running a conversion test for each spec.

@lidel
Copy link
Member

lidel commented Feb 7, 2021

@aschmahmann @gammazero I tried to convert default flatfs datastore from 0.8.0-rc2 to badger with ipfs-ds-convert and got:

$ GO111MODULE=on go get github.com/ipfs/ipfs-ds-convert@latest
go: github.com/ipfs/ipfs-ds-convert latest => v0.5.0

$  ipfs-ds-convert convert
convert 2021/02/07 16:11:01 unsupported fsrepo version: 11

Does it mean we need to land this PR before we ship 0.8.0? (ipfs/kubo#7707)

@aschmahmann
Copy link
Contributor

aschmahmann commented Feb 7, 2021

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

SupportedRepoVersion = 10
to 11. Try a local build and give it a 👍 if that works for you,

@lidel
Copy link
Member

lidel commented Feb 18, 2021

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants