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.

Sign up for GitHub

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

Jump to bottom

Implement initial read-only blockstore #88

Merged

masih merged 2 commits into ipld:wip/v2 from masih:carv2-ro-blockstore

Jun 18, 2021

Member

masih commented Jun 18, 2021

Refactor packages into internal as needed, and implement a ReadOnlyBlockstore as provided by carbs.
Improve write efficiency for CAR v2 primitives.

Note, future PRs will adjust the read-only blockstore to accept CAR v2 format and work transparently.


          Implement read-only random access blockstore

8cbd42f

Refactor the carbon and carbs packages into a read-only blockstore.

Move the packages to `internal` as needed and remove duplicate reader
implementations.

Improve serialization efficiency for car v2 primitives.

Note the readonly blockstore will be altered in the coming PRs to
understand car v2 format and work transparently. For now we want to
push the refactoring and changes to unblock other parallel workstreams.

masih added the kind/enhancement label

masih self-assigned this

masih marked this pull request as ready for review

June 18, 2021 13:13

masih requested a review from mvdan

June 18, 2021 13:13

willscott reviewed

View reviewed changes

v2/blockstore/ro_blockstore.go Outdated

+              	// ReadOnlyBlockStore provides a read-only Car Block Store.
+              	ReadOnlyBlockStore struct {
+              		backing io.ReaderAt
+              		idx     index.Index

Member

willscott Jun 18, 2021

we probably want a non-internal Index type - it sounds like the sharded blockstore would like to be able to detach indexes to store them separate from cars, and also be able to recombine a carv1 + index into a carv2

Member Author

masih Jun 18, 2021

v2/blockstore/ro_blockstore.go Outdated

+              }
+              // AllKeysChan returns the list of keys in the store
+              func (b *ReadOnlyBlockStore) AllKeysChan(ctx context.Context) (<-chan cid.Cid, error) {

Member

willscott Jun 18, 2021

we may use this walk for populating the index, and we need to be able to iterate keys in this way somewhere for index generation. In general though, when it's asked for all keys from a blockstore with an index, we should iterate through the index when possible rather than linear reads through the full car.

Member Author

masih Jun 18, 2021

Added TODO thank you for pointing this out

mvdan approved these changes

View reviewed changes

v2/blockstore/ro_blockstore.go Outdated

		@@ -0,0 +1,190 @@
		package blockstore

Contributor

mvdan Jun 18, 2021

this package is called blockstore, so I think blockstore/readonly.go is slightly better than blockstore/ro_blockstore.go

v2/blockstore/ro_blockstore.go Outdated

+              	"encoding/binary"
+              	"errors"
+              	"fmt"
+              	"golang.org/x/exp/mmap"

Contributor

mvdan Jun 18, 2021

nit: not a std import, shouldn't be in this group

v2/blockstore/ro_blockstore.go Outdated

+              // errUnsupported is returned for unsupported operations
+              var errUnsupported = errors.New("unsupported operation")
+              type (

Contributor

mvdan Jun 18, 2021

any particular reason you use () groups for single types? consistency? it does add two lines for very little benefit, so I'd probably avoid it.

Member Author

masih Jun 18, 2021

face of habit I usually do this to group type declarations

v2/internal/carbon/carbon_fds.go Outdated

@@ @@ -14,8 +15,8 @@ import ( @@
               type carbonFD struct {
               	path        string
               	writeHandle *poswriter
-              	carbs.BlockStore
-              	idx *insertionIndex
+              	blockstore.ReadOnlyBlockStore

Contributor

mvdan Jun 18, 2021

like the filename, I wonder if this type stutters a bit. blockstore.ReadOnly is pretty good, or if you still prefer to clarify what it is, maybe blockstore.ReadOnlyStore?

also, nit: I'm pretty sure the go-ipfs-blockstore package uses the name Blockstore, not BlockStore (note the lack of capitalization of Store), so I think go-car should be consistent too. Maybe its name is a historical typo/mistake, but best to not confuse things further with multiple spellings.

v2/writer.go Outdated

@@ @@ -3,12 +3,12 @@ package car @@
               import (
               	"bytes"
               	"context"
+              	"github.com/ipld/go-car/v2/internal/index"

Contributor

mvdan Jun 18, 2021

hmmm :) maybe run goimports or gofumpt on all the files to make sure you're not mixing things into the std group?

masih mentioned this pull request

Provide public API for CAR v2 index #89

Closed


          Address PR reviews

- Sort imports
- Rename types for clarity
- Add TODO for future PRs

masih merged commit a25f5b1 into ipld:wip/v2

masih deleted the carv2-ro-blockstore branch

June 18, 2021 15:01

masih added this to the CAR v2 milestone

Member

rvagg commented Jun 21, 2021

The inclusion of ReadCid here is unfortunate, it really should be something go-cid can do. We added a similar one in JS recently, decodeFirst() and having the "find a CID in these bytes, starting from here" functionality has generic utility.
Not suggesting this is critical to address now, but is there a good reason anyone can see why this shouldn't be on the future-refactor list? (Perhaps something I might get to on a day I need a distraction).

Member

rvagg commented Jun 21, 2021

noted here ipfs/go-cid#126

Member Author

masih commented Jun 21, 2021

That makes sense thanks for pointing this out @rvagg 🍻 I have captured this on go-car repo so we don't forget to do it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment