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

consider reverting peerstore context changes #2327

Closed
Tracked by #2237
marten-seemann opened this issue Jun 3, 2023 · 2 comments · Fixed by #2328
Closed
Tracked by #2237

consider reverting peerstore context changes #2327

marten-seemann opened this issue Jun 3, 2023 · 2 comments · Fixed by #2328

Comments

@marten-seemann
Copy link
Contributor

I just merged #2312, and tried building Kubo. This is creating build errors all over the place (Kubo itself, Kademlia, Gossipsub). Upgrading libp2p will be extremely painful if we cut a release now.

I'm wondering if we really need this. Does respecting cancelations actually make sense if we don't get any result back from the peerstore that a call failed (e.g. an error on Put)? Now we can set a context with deadline, but we have no idea if the change was actually committed when that call returns.

What was the motivation behind #2231 anyway?

cc @chaitanyaprem @sukunrt @MarcoPolo

@chaitanyaprem
Copy link
Contributor

I did not expect it would break multiple things.
As you have mentioned above, context would only be required in case of database backed peer store. I was not aware that this is not used by many, but i do agree with your point as mentioned in #2329 .

Sorry for the trouble.

@marten-seemann
Copy link
Contributor Author

@chaitanyaprem No need to be sorry. Actually, this is our fault. We didn’t clearly think through the implications of this when we marked this issue as “help wanted”. I’m sorry.

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 a pull request may close this issue.

2 participants