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

feat(mdns): change mdns::Event to hold Vec #3621

Merged
merged 21 commits into from
May 2, 2023
Merged

Conversation

drHuangMHT
Copy link
Contributor

@drHuangMHT drHuangMHT commented Mar 16, 2023

Description

In previous PR #3606 we've made mdns::Event Clone, but cloning single-use iterators doesn't sound right. Also you have to create an iterator from the actual data returned before putting it into events. So in this PR the iterators are replaced by Vec, as it's the type the data originally come from.

Related #3612.

Notes & open questions

breaking change
Should we remove the use of SmallVec altogether? When should we roll out this breaking change?

Should we remove smallvec entirely from the library? The performance impact is not noticeable before a large P2P network is formed.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@thomaseizinger
Copy link
Contributor

I think I'd rather not have smallvec in our public API.

If we decide to do this, we can ship it with the next set of breaking changes.

Needs a changelog entry.

@thomaseizinger thomaseizinger added this to the v0.52.0 milestone Mar 16, 2023
@drHuangMHT
Copy link
Contributor Author

I think I'd rather not have smallvec in our public API.

Glad to see that coming. Let's do it at once.

protocols/mdns/CHANGELOG.md Outdated Show resolved Hide resolved
protocols/mdns/src/behaviour.rs Show resolved Hide resolved
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

I am in favor of the removal of the Iterator, I am in favor of the removal of SmalVec. Thanks @drHuangMHT!

@drHuangMHT drHuangMHT changed the title feat(mdns): change mdns::Event to hold SmallVec feat(mdns): change mdns::Event to hold Vec Mar 17, 2023
@mergify
Copy link
Contributor

mergify bot commented Mar 24, 2023

This pull request has merge conflicts. Could you please resolve them @drHuangMHT? 🙏

@drHuangMHT
Copy link
Contributor Author

drHuangMHT commented Mar 24, 2023

Are we releasing 0.52 right away? Ping @thomaseizinger

@thomaseizinger
Copy link
Contributor

Are we releasing 0.52 right away? Ping @thomaseizinger

Not right away but there are some breaking changes on the horizon, most notable #2680 which will enable kademlia client mode. See #3651 if you want to track progress. Once we decide to merge that, this PR can also go in. I want to do a round of patch releases before that though.

Happy to resolve the merge conflicts for you if you don't want to bother :)

@drHuangMHT
Copy link
Contributor Author

Happy to resolve the merge conflicts for you if you don't want to bother :)

Thanks. But I'll resume my activity. My server refused to boot because of a faulty off-brand USB hub. I'll try to resolve the conflict myself because it's a chance for me to learn more about git CLI. It's a necessity, I can't get away with that.

@drHuangMHT
Copy link
Contributor Author

Oh no I did not actually resolve the conflict but left the marker here. I thought it was an overlay......

@mergify

This comment was marked as resolved.

@mergify
Copy link
Contributor

mergify bot commented May 2, 2023

This pull request has merge conflicts. Could you please resolve them @drHuangMHT? 🙏

@thomaseizinger
Copy link
Contributor

We've now opened the merge window for breaking changes so this is good to go in once the merge conflicts are resolved.

@drHuangMHT drHuangMHT marked this pull request as ready for review May 2, 2023 10:39
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks!

@mergify mergify bot merged commit 02d8715 into libp2p:master May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants