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

update to context datastores #275

Merged
merged 1 commit into from
Nov 29, 2021
Merged

update to context datastores #275

merged 1 commit into from
Nov 29, 2021

Conversation

whyrusleeping
Copy link
Member

No description provided.

@ipfs ipfs deleted a comment from welcome bot Nov 19, 2021
@rvagg
Copy link
Member

rvagg commented Nov 19, 2021

Check is failing on go mod tidy, test is failing on a flaky being discussed @ #273 which should be unrelated to this change.

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.

I pushed a fixup for the mod tidy; just need to remember to squash that in when merging

go.sum Outdated
Comment on lines 327 to 328
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=
Copy link

@aschmahmann aschmahmann Nov 19, 2021

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

Copy link

@aschmahmann aschmahmann Nov 19, 2021

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Collaborator

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:

  1. context/tracing in the blockstore/datastore pipeline kubo#6803 -- adding contexts
  2. 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.

Copy link
Collaborator

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?)

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

@aschmahmann aschmahmann force-pushed the deps/update-ctx-ds branch 2 times, most recently from f03a1e3 to 99752d2 Compare November 19, 2021 19:40
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

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.

Copy link
Collaborator

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 ?

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome thanks for clarification.

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

@hannahhoward hannahhoward merged commit d3b589b into main Nov 29, 2021
rvagg pushed a commit that referenced this pull request Nov 30, 2021
@aschmahmann aschmahmann mentioned this pull request Dec 1, 2021
80 tasks
@mvdan mvdan deleted the deps/update-ctx-ds branch December 15, 2021 14:17
marten-seemann pushed a commit that referenced this pull request Mar 2, 2023
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.

5 participants