Repository navigation
[1/n] [installinator] add PeerAddress wrapper - #8035
Conversation
Created using spr 1.3.6-beta.1
|
|
||
| #[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Ord, Eq)] | ||
| #[cfg_attr(test, derive(test_strategy::Arbitrary))] | ||
| pub(crate) struct PeerAddress { |
There was a problem hiding this comment.
I don't necessarily object to this, but I'm not seeing the benefit. I skimmed the later PRs and didn't see any additions to PeerAddress but certainly could've missed something. Do we build on this later in a way that a raw socket addr doesn't work? (If the only methods are "create with a SocketAddr" and "get the SocketAddr", why not keep using SocketAddr directly?)
There was a problem hiding this comment.
While developing the series I felt like having the newtype wrapper helped me reason about it better. But I used to have more functionality hanging off of it than I do right now so 🤷🏽♀️
(In particular I briefly looked at making it v6-only, but realized that in testing workflows we use v4 sometimes)
There was a problem hiding this comment.
Gotcha. Personally I'd vote to keep using SocketAddr, but if that's a pain or you think we might add more functionality in the future, I'm on board.
There was a problem hiding this comment.
Ah actually I remember now -- wanted to add a FromIterator<PeerAddress> for PeerAddresses impl in #8040 or a followup to it. Felt like having the newtype means you wouldn't just be able to collect any iterator of socket addresses.
There's been a long-standing issue with the installinator where reports to wicketd get delayed quite substantially. That happens because we use one task to send reports out to every peer, blocking until all of those peers come back. I'd assumed in the past that if installinator reached out to an unreachable peer, it would receive a TCP connection refused message, but that isn't the case -- instead, it times out. This causes reports to only be sent out roughly every 15 seconds which isn't ideal. To fix this issue, spin up separate report tasks for each peer. Introduce separate tasks for: * discovery (make this a persistent task that publishes updates to a watch channel) * peer reconciliation (new peers have new report tasks spun up, while removed peers' report tasks are cancelled) * reporting (each peer now has its own report loop) Also simulate some kinds of network flakiness in our property-based tests. I did a mupdate on dublin and saw that installinator reports started coming through every 2 seconds rather than every 15 or so, as expected. Depends on: - #8035 - #8036 - #8037 - #8038 - #8039 - #8040
2 participants
Add a newtype wrapper to ensure that we aren't handling bare
SocketAddrinstances.