feat(transport): start working on transport metering & metrics #102
Conversation
msg-socket/src/req/mod.rs
Outdated
| pub(crate) transport: Arc<RwLock<Arc<S>>>, | ||
| } | ||
|
|
||
| // Manual clone implementation needed here because `S` is n`. |
| where | ||
| Io: AsyncRead + AsyncWrite + PeerAddress<A> + Unpin, | ||
| A: Address, | ||
| S: for<'a> TryFrom<&'a Io, Error: Debug>, |
There was a problem hiding this comment.
This took me a while ngl
thedevbirb
left a comment
thedevbirb
left a comment
There was a problem hiding this comment.
First pass, mostly readability / clarity!
| type ConnectionCtl<Io, S, A> = | ||
| ConnectionState<Framed<MeteredIo<Io, S, A>, reqrep::Codec>, ExponentialBackoff, A>; |
There was a problem hiding this comment.
Can you keep generics a little bit descriptive? Like Stats and Addr, otherwise looks a bit like the alphabet.
There was a problem hiding this comment.
I usually avoid doing that because then they become indistinguishable from concrete type implementations
There was a problem hiding this comment.
I see. Anyway, I think C# got it right since a lot of years with the T* naming convention. https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/identifier-names#type-parameter-naming-guidelines. I've also seen it a lot in TypeScript and I think it's great.
| /// The interval at which the stats should be refreshed. | ||
| refresh_interval: Duration, | ||
|
|
||
| _marker: PhantomData<A>, |
There was a problem hiding this comment.
The marker is not needed here, A is already captured by Io.
There was a problem hiding this comment.
Still needs to be actually referenced in one of the fields, I can't remove this.
msg-socket/src/req/mod.rs
Outdated
| pub(crate) struct SocketState<S> { | ||
| /// The socket stats. | ||
| pub(crate) stats: Arc<SocketStats<ReqStats>>, | ||
| pub(crate) transport: Arc<RwLock<Arc<S>>>, |
There was a problem hiding this comment.
I'm a bit doubtful of the Arc wrapping RwLock wrapping Arc... It feels a bit of an anti pattern
There was a problem hiding this comment.
Could you explain why it's necessary? Maybe with a short comment here even
Co-authored-by: Karrq <franci.dainese@gmail.com>
To Do
Stats for other transports will be added in follow-up