-
Notifications
You must be signed in to change notification settings - Fork 930
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
feat(kad)!: add ModeChanged
event
#4341
Conversation
Signed-off-by: Dave Huseby <[email protected]>
Maybe the correct behavior is to change the Handler so that after it has made changes in response to the KademliaHandlerIn::ReconfigureMode event, then it confirms that change by telling the behavior to emit the event. Thoughts? |
0d7b317
to
f81207f
Compare
Signed-off-by: Dave Huseby <[email protected]>
Pull-Request: libp2p#4337.
This pull request has merge conflicts. Could you please resolve them @dhuseby? 🙏 |
ModeChanged
event
Please no force-pushing :) |
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.
Thanks!
The test now fails which is good. We should update the test to also assert the new event being emitted.
I've queued this for the |
The handler doesn't know about the "automatic" reconfiguration based on the external address. In other words, it also reconfigures itself if the user sets the mode manually. I think we should not emit an event in that case because the user already knows that they just changed the mode. We should probably also mention that in the docs of the new event. |
Pull-Request: libp2p#4344.
Pull-Request: libp2p#4340.
Following libp2p#4289 (comment), hereby is the PR to also improve the `create_socket` using [`for_addr`](https://docs.rs/socket2/latest/socket2/struct.Domain.html#method.for_address). We also add a test for listening on IPv4 and IPv6 separately. Pull-Request: libp2p#4328.
Request to add [TAPLE](https://www.taple.es) Project to rust-libp2p notable users. TAPLE is a permissioned DLT solution for traceability of assets and processes. We use libp2p for the network layer. Pull-Request: libp2p#4309.
Pull-Request: libp2p#4296.
I can kill this PR, create a new clean branch on my end and resubmit with your suggested changes. |
Co-authored-by: Thomas Eizinger <[email protected]>
Co-authored-by: Thomas Eizinger <[email protected]>
Co-authored-by: Thomas Eizinger <[email protected]>
Signed-off-by: Dave Huseby <[email protected]>
Signed-off-by: Dave Huseby <[email protected]>
Co-authored-by: Thomas Eizinger <[email protected]>
Co-authored-by: Thomas Eizinger <[email protected]>
Co-authored-by: Thomas Eizinger <[email protected]>
Signed-off-by: Dave Huseby <[email protected]>
Signed-off-by: Dave Huseby <[email protected]>
Signed-off-by: Dave Huseby <[email protected]>
Signed-off-by: Dave Huseby <[email protected]>
No need for the extra effort! Continuing here is fine :) |
- [Ursa](https://github.com/fleek-network/ursa) - Decentralized Content Delivery & Edge Compute. | ||
- [Taple](https://github.com/opencanarias/taple-core) - Sustainable DLT for asset and process traceability by [OpenCanarias](https://www.opencanarias.com/en/). |
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.
Something seems to have gone wrong during the rebase / merge because this diff is showing up here as if it were in this PR?
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.
yeah...I think I pulled the upstream master into my branch at some point. let me see if I can clean this up.
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 tried to do an interactive rebase and drop the unwanted commits and they don't show up in the list of available commits to drop. So instead I created a new branch and used cherry-pick to recreate this PR's branch with just my commits and your review commits. The new branch is here: https://github.com/dhuseby/rust-libp2p/tree/add-kad-modechanged-event
I was think the best course of action at this point is to close this PR and file a new PR with the new clean branch. What do you think?
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 was think the best course of action at this point is to close this PR and file a new PR with the new clean branch. What do you think?
Yes, sounds good!
This branch is messed up. Replaced by PR #4499 |
Description
Previously, the kademlia protocol would only log changes to the internal mode. With this patch, we now also emit an event which allows users to code against the internal state of the kademlia protocol.
Fixes #4310.
Notes & open questions
I added the code to emit the event in the reconfigure_mode function on the assumption that all mode changes result in that function being called.
Change checklist