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

blockstore: give a direct access to the index for read operations #387

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

MichaelMure
Copy link
Contributor

This can be useful for advanced read-only operation, like indexing.

Copy link
Member

@willscott willscott left a comment

Choose a reason for hiding this comment

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

This seems fine to expose to me - @rvagg - do you have any major concerns?

@@ -178,6 +179,12 @@ func (b *ReadWrite) initWithRoots(v2 bool, roots []cid.Cid) error {
return carv1.WriteHeader(&carv1.CarHeader{Roots: roots, Version: 1}, b.dataWriter)
}

// Index gives direct access to the index.
Copy link
Member

Choose a reason for hiding this comment

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

do we need this on ReadWrite - that's where it's dangerous.

The worry with ReadWrite access is you add another record, and then when you go to finalize the blockstore the extra record creeps in.

on ReadOnly, the index you access isn't going to get written back out into the underlying car file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do need that on ReadWrite, but I can add a wrapper to enforce the read-only access.

Copy link
Member

Choose a reason for hiding this comment

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

do you need it before you finalize?
could we have finalizeReadOnly give you a ReadOnly variant that you can then get the index from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only need it after finalize, but I sort of see why some might.

@rvagg
Copy link
Member

rvagg commented Mar 13, 2023

I'm fine with this. Tests please, on both readonly and readwrite to prove that it's doing what you think it is.

readwrite gives you InsertionIndex which is quite a different beasty than the typical Index. A wrapper to protect write operations might be a good idea, although I tend to lean on the caveat emptor approach—if you figure out how to hack it to do your crazy thing then that's your problem; we already have plenty of warnings and safety procedures in place wrt "untrusted" indexes from third parties and this library isn't going to stop bad indexes getting published.

@MichaelMure
Copy link
Contributor Author

I added some tests.

@rvagg
Copy link
Member

rvagg commented Mar 20, 2023

needs a rebase @MichaelMure, I'll cut a semver-minor after this gets merged

@MichaelMure
Copy link
Contributor Author

@rvagg it's rebased now.

@rvagg rvagg merged commit 2f59674 into ipld:master Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

3 participants