Support simple QOS -- with QOS trait refactoring #8437
Conversation
| const WAIT_FOR_CONNECTION_TIMEOUT: Duration = Duration::from_secs(1); | ||
| debug!("spawn quic server"); | ||
| let mut last_datapoint = Instant::now(); | ||
| let unstaked_connection_table: Arc<Mutex<ConnectionTable>> = Arc::new(Mutex::new( |
There was a problem hiding this comment.
logic moved to different QOS implementations as it is QOS specific.
| tasks | ||
| } | ||
|
|
||
| fn prune_unstaked_connection_table( |
| )) | ||
| } | ||
|
|
||
| fn compute_max_allowed_uni_streams(peer_type: ConnectionPeerType, total_stake: u64) -> usize { |
| MaxStreamError, | ||
| } | ||
|
|
||
| #[derive(Clone)] |
There was a problem hiding this comment.
No longer needed as replaced by the Qos trait and ConnectionContext.
| } | ||
| } | ||
|
|
||
| fn handle_and_cache_new_connection( |
There was a problem hiding this comment.
The logic of handling adding to connection cache is moved to QosController::try_cache_connection
| } | ||
| } | ||
|
|
||
| async fn prune_unstaked_connections_and_add_new_connection( |
There was a problem hiding this comment.
connection caching part is moved to swqos.rs
| } | ||
|
|
||
| /// Calculate the ratio for per connection receive window from a staked peer | ||
| fn compute_receive_window_ratio_for_staked_node(max_stake: u64, min_stake: u64, stake: u64) -> u64 { |
| return; | ||
| } | ||
|
|
||
| let params = get_connection_stake(&new_connection, &staked_nodes).map_or( |
There was a problem hiding this comment.
The logic is refactored into the QOS implementations. Essentially it does the following 3 things:
build the connection context,
try cache the connection cache.
handle the connection.
| qos.on_stream_closed(&context); | ||
| } | ||
|
|
||
| let stable_id = connection.stable_id(); |
There was a problem hiding this comment.
Logic encapuslated in Qos's remove_connection handler.
| } | ||
|
|
||
| enum ConnectionTableType { | ||
| pub(crate) enum ConnectionTableType { |
There was a problem hiding this comment.
made pub(crate) as needed by both type of QOS
| assert_eq!(stats.open_connections.load(Ordering::Relaxed), 0); | ||
| } | ||
|
|
||
| #[test] |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #8437 +/- ##
========================================
Coverage 83.1% 83.2%
========================================
Files 840 842 +2
Lines 367669 368120 +451
========================================
+ Hits 305719 306342 +623
+ Misses 61950 61778 -172 🚀 New features to boost your workflow:
|
| (t, receiver, server_address, cancel) | ||
| } | ||
|
|
||
| fn setup_simple_qos_quic_server_with_params( |
There was a problem hiding this comment.
for testing simple Qos server
| } | ||
|
|
||
| impl SwQos { | ||
| fn cache_new_connection( |
There was a problem hiding this comment.
logic moved from previous handle_and_cache_new_connection with the caching part only without handling the connection.
| } | ||
| } | ||
|
|
||
| fn prune_unstaked_connection_table( |
There was a problem hiding this comment.
Moved from previous quic.rs's prune_unstaked_connection_table
| } | ||
| } | ||
|
|
||
| async fn prune_unstaked_connections_and_add_new_connection( |
There was a problem hiding this comment.
moved from previous quic.rs's prune_unstaked_connections_and_add_new_connection
| } | ||
|
|
||
| impl QosController<SwQosConnectionContext> for SwQos { | ||
| fn derive_connection_context(&self, connection: &Connection) -> SwQosConnectionContext { |
There was a problem hiding this comment.
Logic moved from previous quic.rs's setup_connection's code on determining the connection stake and peer type
| } | ||
|
|
||
| #[allow(clippy::manual_async_fn)] | ||
| fn try_cache_connection( |
There was a problem hiding this comment.
From previous quic.rs's setup_connection on handling caching the connection.
|
|
||
| #[cfg(test)] | ||
| pub mod test { | ||
| use super::*; |
There was a problem hiding this comment.
Test cases moved from previous quic.rs specific to SWQOS
lijunwangs
requested review from
KirillLykov,
alexpyattaev,
bw-solana and
sakridge
core/src/voting_service.rs
Outdated
| }; | ||
|
|
||
| // Convservatively allow 50 TPS per validator. | ||
| pub const MAX_VOTES_PER_SECOND: u64 = 50; |
There was a problem hiding this comment.
Why is this defined here? I'd define this in the streamer itself. Having cross-crate deps makes reasoning harder.
There was a problem hiding this comment.
I think it really belongs to here as it is this service controlling the streamer service. The streamer is generic and has no idea about vote or not.
There was a problem hiding this comment.
By this logic maybe this belongs in TPU? Voting service is the client, not the server in this relationship.
Also I'd probably cut this down to 20 or so - there is no reasonable way we'd have to cast over 10 votes per second.
There was a problem hiding this comment.
Right! I moved it to tpu and updated the value to 20/s.
minimum one stream per throttle window for simple QOS WIP streamer_support_qos_as_trait WIP refactoring QUIC streamer cleaning up code WIP refactoring continued refactoring continued refactoring clean up code fmt code Fixed some comp issues Fixed some comp issues Ignore manual async warning as it is intentional Moved more swqos functions out
added unit tests for simple QOS make test exact rename test name
lijunwangs
force-pushed
the
streamer_support_qos_as_trait.rb
branch
from
744e899 to
1be7ec2
Compare
3 participants
Problem
Vote using QUIC requires a simpler QOS instead of stake weighted as we want to allow all staked nodes to be able to vote and there is no need to differentiate the count of votes per vote interval based on stakes.
Summary of Changes
There should be 0 impacts on existing SwQOS implementations as it is pure refactoring it.
I have annotated the code on major functions being refactored.
Fixes #