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
dariusc93
left a comment
dariusc93
left a comment
There was a problem hiding this comment.
Thanks for the PR. Left a couple of comments
|
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? 🙏 |
jxs
left a comment
jxs
left a comment
There was a problem hiding this comment.
Hi François, thanks for this! And sorry for the delay, left some comments
|
This pull request has merge conflicts. Could you please resolve them @stormshield-frb? 🙏 |
| /// For each peer, we will at most store `config.number_of_addresses_per_peer` addresses. | ||
| pub fn new(config: PeerAddressesConfig) -> Self { | ||
| let inner = LruCache::new(config.number_of_peers.get()); | ||
| Self { config, inner } |
There was a problem hiding this comment.
Ah sorry, I just noticed now that this is a breaking change.
Breaking changes in the swarm are quite painful, because we need to do a minor bump and release for every single behavior protocol that depends on the swarm.
We could avoid the breaking change by not adding the PeerAddressesConfig here and instead just add with_number_of_addresses_per_peer directly for PeerAddresses.
The PeerAddressesConfig could still live as CacheConfig in identity.
Wdyt? I understand that by now you might not be working on this anymore, so I can also push this change if folks agree with it. cc @jxs
There was a problem hiding this comment.
Hi Elena, yeah agree, let's have this as a separate method.
Btw François has left Stormshield and so this account is probably not working, nonetheless I think this is a useful feature to have, I can finish this PR if you are busy
| /// For each peer, we will at most store `config.number_of_addresses_per_peer` addresses. | ||
| pub fn new(config: PeerAddressesConfig) -> Self { | ||
| let inner = LruCache::new(config.number_of_peers.get()); | ||
| Self { config, inner } |
There was a problem hiding this comment.
Hi Elena, yeah agree, let's have this as a separate method.
Btw François has left Stormshield and so this account is probably not working, nonetheless I think this is a useful feature to have, I can finish this PR if you are busy
| /// Defaults to 100 | ||
| cache_size: usize, | ||
| /// Configuration for the LRU cache of discovered peers. | ||
| cache_config: Option<PeerAddressesConfig>, |
There was a problem hiding this comment.
If it's None the cache is disabled. Before, cache size 0 was used for that.
4 participants
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