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 a cidlink.Memory storage option #266

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Add a cidlink.Memory storage option #266

merged 2 commits into from
Oct 14, 2021

Conversation

willscott
Copy link
Member

the current storage.Memory does not provide cid semantics where data can be stored under e.g. cidV0 and retrieved as cidV1.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

sgtm
beInitialized() is a bit of a naming oddity though, and are you intending for Bag to be public? seems like something that should remain private and be mediated through accessors if you need to do something fancy with it.

@willscott
Copy link
Member Author

that part is directly copying https://github.com/ipld/go-ipld-prime/blob/master/storage/memory.go

@rvagg
Copy link
Member

rvagg commented Oct 14, 2021

ok then, @warpfork can consider that critique directed at him then, and given the consistency this is 👍

Co-authored-by: Rod Vagg <rod@vagg.org>
@warpfork
Copy link
Collaborator

yeah, the memory storage type that Will's riffing off of here was really only intended for testing and demo usage, so it's very yolo/we-are-all-consenting-adults-here/unsafe. I shan't defend it much :)

@warpfork
Copy link
Collaborator

I wonder if this would be easier to do more cleanly after the big storage API PR.

After that lands, I would propose we could solve this with a function like:

func ConfigureLinkSystemWithMultihashKeyedStorage(lsys *linking.LinkSystem, store storage.WritableStorage) {
	lsys.StorageWriteOpener = func(lctx linking.LinkContext) (io.Writer, linking.BlockWriteCommitter, error) {
		wr, wrcommit, err := storage.PutStream(lctx.Ctx, store)
		return wr, func(lnk datamodel.Link) error {
			return wrcommit(lnk.(cidlink.Link).Cid.Hash().HexString())
		}, err
	}
}

... the upside of which would be: we're not rewriting the storage implementation, we're just writing how we transform the link into a key before we hand it to the storage layer. This would seem cleaner and results in more code reuse.

@willscott
Copy link
Member Author

I needed this already and have been using it in e.g. https://github.com/ipfs/go-unixfsnode/pull/12/files#diff-daf113341551df24033a6ff3b492572582e857944008668d6647486c5ab415e2R33-R35

In general I'm not convinced about "there will be better paths later" as a reason not to merge things, since that's always true.

@willscott willscott merged commit 6a1bd1c into master Oct 14, 2021
@warpfork warpfork deleted the cidstorage branch October 22, 2021 08:28
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
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