Make networking handshake timeout configurable #1796
Conversation
| /// | ||
| /// Time a node waits for handshake message to be received before | ||
| /// giving up | ||
| pub handshake_timeout: Duration, |
There was a problem hiding this comment.
Can we use humantime::Duration instead and setup schemar's correctly for it? You can find an example in config/http.rs
| } | ||
|
|
||
| impl ConnectionManager { | ||
| /// Creates the connection manager. | ||
| pub(super) fn new(metadata: Metadata) -> Self { | ||
| pub(super) fn new(metadata: Metadata, handshake_timeout: Duration) -> Self { |
There was a problem hiding this comment.
It might be better to future-proof this as pass in NetworkingOptions since this is a long-living object anyway.
There was a problem hiding this comment.
Yeah, i was thinking of that i just didn't want to make clones of it everywhere. And was honestly considering using an Arc for it (maybe with a type alias to reduce clutter).
On the other hand, the value cloned is not big and hence this can be an unnecessary optimisation
what do you think ?
There was a problem hiding this comment.
ConnectionManager is a long living value that lives as long as the node, cloning is absolutely fine here.
Take the entire NetworkingOptions for future-proofing
| let retry_policy = self.options.connect_retry_policy.clone(); | ||
| let mut retry_policy = retry_policy.iter(); |
There was a problem hiding this comment.
Why do you need to clone here? I think if you capture max_attempts() here you wouldn't have a problem with the projection reference below.
There was a problem hiding this comment.
Ah definitely, my fault. for some reason in my head iter() consumed self but it actually doesn't.
drop the close of the rety policy
There was a problem hiding this comment.
Thanks for making the handshake timeout configurable @muhamadazmy. The changes look good to me.
Fixes #1765