streamer: refactor config structs#9093
Conversation
alexpyattaev
force-pushed
the
streamer_configs
branch
from
83cf336 to
d8c8321
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #9093 +/- ##
=======================================
Coverage 82.6% 82.6%
=======================================
Files 889 889
Lines 320852 320857 +5
=======================================
+ Hits 265215 265241 +26
+ Misses 55637 55616 -21 🚀 New features to boost your workflow:
|
|
|
||
| pub(crate) fn max_concurrent_connections(&self) -> usize { | ||
| let conns = self.max_staked_connections + self.max_unstaked_connections; | ||
| conns + conns / 4 |
There was a problem hiding this comment.
We intentionally added this cushion of 1/4 in the max connections. Is this logic kept?
There was a problem hiding this comment.
Thank you for reminder, will bring it back.
alexpyattaev
force-pushed
the
streamer_configs
branch
from
d8c8321 to
cc5f599
Compare
alexpyattaev
changed the title
| max_connections_per_ipaddr_per_min: DEFAULT_MAX_CONNECTIONS_PER_IPADDR_PER_MINUTE, | ||
| wait_for_chunk_timeout: DEFAULT_WAIT_FOR_CHUNK_TIMEOUT, | ||
| num_threads: NonZeroUsize::new(num_cpus::get().min(1)).expect("1 is non-zero"), | ||
| max_concurrent_connections: (DEFAULT_MAX_STAKED_CONNECTIONS |
There was a problem hiding this comment.
maybe this logic preceding but requires a comment
There was a problem hiding this comment.
This did not exactly bring back the max_concurrent_connections() encapsulation -- as it is only handles when it is default, When it is configured by the user, it does not give the cushion.
There was a problem hiding this comment.
The cushion is needed so that allows the application layer to do more filtering based on that layer logic -- otherwise Quinn would just reject the connection.
There was a problem hiding this comment.
The rejection is all at application layer already. But you are right we need to compute it also in validator.rs in case someone increases e.g. max staked connections to 16k
There was a problem hiding this comment.
Made a clean solution by adding a function to the qos trait. This way it is not ugly and is computed same way as before.
| @@ -48,10 +48,6 @@ where | |||
|
|
|||
| let swqos = Arc::new(SwQos::new( | |||
There was a problem hiding this comment.
Maybe in separate PR, but this Arc is not needed here, SwQos clearly must be Clone cause all it's attributes are Clone.
There was a problem hiding this comment.
We eventually need to move it into an Arc but yeah it can be done way later.
KirillLykov
left a comment
KirillLykov
left a comment
There was a problem hiding this comment.
My understanding is that this PR moves 4 attributes (max_staked_connections etc) from SwQos to SwQosConfig which sort of makes sense.
alexpyattaev
force-pushed
the
streamer_configs
branch
from
cc5f599 to
15f1a0c
Compare
refactor streamer configs
4 participants
Problem
Summary of Changes