feat(swarm): improve PeerAddresses configurability
#5574
Conversation
stormshield-frb
force-pushed
the
feat/identify-make-peer-cache-configurable
branch
3 times, most recently
from
8fbd19b
to
be31d42
Compare
stormshield-frb
force-pushed
the
feat/identify-make-peer-cache-configurable
branch
from
be31d42
to
e5435fc
Compare
protocols/identify/src/behaviour.rs
Outdated
| /// Configures the size of the LRU cache, caching addresses of discovered peers. | ||
| pub fn with_cache_size(mut self, cache_size: usize) -> Self { | ||
| self.cache_size = cache_size; | ||
| /// Configuration for the LRU cache responsible for caching addresses of discovered peers. | ||
| /// | ||
| /// If set to [`None`], caching is disabled. | ||
| pub fn with_cache_config(mut self, cache_config: Option<PeerAddressesConfig>) -> Self { | ||
| self.cache_config = cache_config; |
There was a problem hiding this comment.
We should probably keep with_cache_size, deprecating it for with_cache_config and just have use that internally so we could probably introduce this as a patch update. Thoughts?
There was a problem hiding this comment.
If this is possible, I'm definitely doing that. I did not think of that thanks.
There was a problem hiding this comment.
It's done. I'll let you resolve this if you're ok with it. 😉
There was a problem hiding this comment.
Ok. It was slightly more complicated than expected because since every attributes of identify::Config are public, I cannot remove cache_size if we want to stay in 0.45.1 and not upgrade to 0.46.0.
Just a thought, but I checked the other "Config" object across the project and some do expose the attributes but most of them don't. Maybe we should homogenize that and make every attributes private to not have this kind of problem later on ? It would be a pain to make many new major updates but it would once and for all. If you think there is something here, I will happily open an issue to discuss this mater more.
There was a problem hiding this comment.
@dariusc93 I'm closing this conversation. Feel free to re-open it if you have other suggestions about it.
There was a problem hiding this comment.
I agree with François on this one, can we just introduce a breaking change, and take the minor release cycle to also do #5660?
|
This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏 |
stormshield-frb
force-pushed
the
feat/identify-make-peer-cache-configurable
branch
from
14c1974
to
4d17c21
Compare
stormshield-frb
force-pushed
the
feat/identify-make-peer-cache-configurable
branch
from
1b14122
to
457c063
Compare
|
This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏 |
| } | ||
|
|
||
| /// Configure the maximum number of cached addresses per peer. | ||
| pub fn with_number_of_addresses_by_peer( |
There was a problem hiding this comment.
| pub fn with_number_of_addresses_by_peer( | |
| pub fn with_number_of_addresses_per_peer( |
|
This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏 |
Description
Our average peer has over 10 advertised addresses. Doing so,
identifywas continuously (every 1 minute) emittingToSwarm::NewExternalAddrOfPeerwhich was overflowing our logs and was making thePeerCachekind of useless.For this reason, we made some changes in order to be able to configure the number of addresses that should be cached for a peer. We are upstreaming this changes.
Notes & open questions
N/A
Change checklist