-
Notifications
You must be signed in to change notification settings - Fork 38
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
update to context datastores #275
Conversation
Check is failing on go mod tidy, test is failing on a flaky being discussed @ #273 which should be unrelated to this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a fixup for the mod tidy; just need to remember to squash that in when merging
go.sum
Outdated
github.com/ipfs/go-ipfs-ds-help v1.0.0 h1:bEQ8hMGs80h0sR8O4tfDgV6B01aaF9qeTrujrTLYV3g= | ||
github.com/ipfs/go-ipfs-ds-help v1.0.0/go.mod h1:ujAbkeIgkKAWtxxNkoZHWLCyk5JpPoKnGyCcsoF6ueE= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not merge this PR, or any related to contexts in the datastore, until there are no references to go-ipfs-ds-help >v1 in the go.sum file.
The same is true for merging any PRs in a dependency of go-ipfs. No v1 go-ipfs-ds-help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some context (pun intended 😄) here. What's going on is that we're in the middle of adding contexts to go-datastore (in go-datastore v0.5.0) and bubbling up through the various dependencies ipfs/kubo#6803.
There's an ongoing technical debt/stress in the ecosystem (largely go-ipfs vs newer things) in that there are two versions, v0 and v1, of repos like go-ipfs-ds-help, go-ipfs-blockstore, and go-filestore where v0 stores blocks keyed on CIDs and v1 stores them keyed on multihashes.
This means that anything that uses a blockstore and is used by go-ipfs should be dependent on the v0 ones of the above and not v1, as v1 will break go-ipfs, but v0 will silently upgrade to v1 and lotus, etc. will be fine.
go-ipfs is planning to finally get this migration done in v0.12.0 as the only change in that release ipfs/kubo#6815, at which point we shouldn't have to deal with this anymore.
Ideally these updates across repos should be as atomic as possible, but with so many repos and dependencies it takes time. Apologies for any delays or problems it causes if you need to push a change into a repo that's already been released with the context changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @whyrusleeping @aschmahmann ,
Graphsync seems caught in the middle here. I'm wondering: if go-ipfs is updating in the next major release, and the high high odds are that you're not going to update go-graphsync in go-ipfs until then, what is the problem with going to the v1's now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also: we can always cut point releases for go-ipfs if that's super neccesary. But as go-ipfs is hardly a frequent consumer of go-graphsync -- it's still experimental as far as I know -- do we need to hold this up on go-ipfs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aschmahmann Ok, upon further review, I think I understand. There are two endeavors:
- context/tracing in the blockstore/datastore pipeline kubo#6803 -- adding contexts
- Updating IPFS datastores to use multihashes instead of CIDs (v0 -> v1 versions of everything)
So I guess then we need to find the v0 versions of these repos that support the new interfaces. It's not entirely clear from ipfs/kubo#6803 what those are, especially since there's a comment from just two days ago saying there's a need to back out cause the v0 go-ipfs-blockstore wasn't done correctly.
If you can give us the exact v0 versions we should use, I can update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not though, and this is urgent for Estuary, I am inclined to prioritize the needs of heavy users of go-graphsync over go-ipfs, which can continue to just use old versions until it's ready to update. (again, graphsync is to my knowledge experimental in go-ipfs, so I can't imagine keeping it super up to date is very important -- maybe I'm missing something though?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hannahhoward the complexity here isn't really about the v0/v1 blockstore, but more about the go-datastore v0.5.0 breaking changes.
This PR is now up to speed, however we're left with #275 (comment) and can continue in that thread
f03a1e3
to
99752d2
Compare
go.mod
Outdated
github.com/ipfs/go-unixfs v0.2.4 | ||
github.com/ipld/go-codec-dagpb v1.3.0 | ||
github.com/ipld/go-ipld-prime v0.12.3 | ||
github.com/jbenet/go-random v0.0.0-20190219211222-123a90aedc0c | ||
github.com/libp2p/go-buffer-pool v0.0.2 | ||
github.com/libp2p/go-libp2p v0.13.0 | ||
github.com/libp2p/go-libp2p-core v0.8.5 | ||
github.com/libp2p/go-libp2p v0.16.0-dev.0.20211116110622-8df360043e98 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is waiting on a go-libp2p release.
Many of these modules have a dependency on go-datastore. While it might be possible to leave go-libp2p a bit lower here due to the tests not exercising the datastore via libp2p it could cause upstream problems, so realistically this is a minimum dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double checking -- should we wait for the release to merge @aschmahmann ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I wouldn't want people relying on the cutting edge go-graphsync to be on an unreleased go-libp2p, that seems like a recipe for false reports.
Hopefully the go-libp2p release can get out in the next couple of days and we can merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome thanks for clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-libp2p v0.16.0 has been released
99752d2
to
e838b67
Compare
e838b67
to
e1c0329
Compare
…ng new initiator) (#275)
No description provided.