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

Implement a streaming read only IPLD native storage #349

Closed
wants to merge 1 commit into from

Conversation

hannahhoward
Copy link
Collaborator

Goals

Start to implement CARv2 IPLD Storage native interfaces

Implementation

So this is definitely just an experiment, but I think it revealed some tangled dependencies in go-car/v2 blockstore interfaces.

Essentially, all I wanted to do was implement a go-ipld-prime ReadableStorage interface as defined here but I also wanted support for native read streaming -- https://github.com/ipld/go-ipld-prime/blob/master/storage/api.go#L91 -- cause I think this is an interesting use case.

To implement the streaming GetStream I needed lower level access to the car components, but if I implemented in a seperate package it was tough to get all the functionality of the blockstore interfaces.

I think maybe there's common wrapper code to extract? I dunno I didn't want to do a bunch of refactors.

Ultimately, this was for a particular use case I wanted to try -- I'm not convinced it can be easily written outside the go-car/v2 package, but I'm not not convinced it shouldn't be done that way.

@hannahhoward hannahhoward requested a review from rvagg January 14, 2023 05:10
var fnErr error
var foundOffset int64
err = ros.ro.idx.GetAll(key, func(offset uint64) bool {
rdr := internalio.NewOffsetReadSeeker(ros.ro.backing, int64(offset))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the bit that prevents you from doing this elsewhere isn't it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.

in fairness, the same weakness applies to most of our existing storage system cause we've never designed them around this kind of access.

@willscott
Copy link
Member

this does seem pretty similar to readonly.go in the same package - I'm not sure I fully understand why the extra methods for streaming can't be added there?
The interface for ReadableStorage is pretty new and didn't exist at the time that this blockstore package was written, which is why it has the lower level interface of a direct blockstore - another option there is to use the storage/bsadapter shim provided in ipld-prime

@rvagg
Copy link
Member

rvagg commented Feb 8, 2023

done in v2.7.0 in the v2/storage package

@rvagg rvagg closed this Feb 8, 2023
@rvagg rvagg deleted the feat/ipld-read-only-storage branch February 8, 2023 02:40
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.

3 participants