Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v1.18: Treat super low staked as unstaked in streamer QOS (backport of #701) #733

Merged
merged 1 commit into from
Apr 11, 2024
Merged

v1.18: Treat super low staked as unstaked in streamer QOS (backport of #701) #733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions streamer/src/nonblocking/quic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use {
crate::{
nonblocking::stream_throttle::{
ConnectionStreamCounter, StakedStreamLoadEMA, STREAM_STOP_CODE_THROTTLING,
ConnectionStreamCounter, StakedStreamLoadEMA, MAX_STREAMS_PER_MS,
STREAM_STOP_CODE_THROTTLING, STREAM_THROTTLING_INTERVAL_MS,
},
quic::{configure_server, QuicServerError, StreamStats},
streamer::StakedNodes,
Expand Down Expand Up @@ -498,10 +499,16 @@ async fn setup_connection(
stats.clone(),
),
|(pubkey, stake, total_stake, max_stake, min_stake)| {
let peer_type = if stake > 0 {
ConnectionPeerType::Staked(stake)
} else {
// The heuristic is that the stake should be large engouh to have 1 stream pass throuh within one throttle
// interval during which we allow max (MAX_STREAMS_PER_MS * STREAM_THROTTLING_INTERVAL_MS) streams.
let min_stake_ratio =
1_f64 / (MAX_STREAMS_PER_MS * STREAM_THROTTLING_INTERVAL_MS) as f64;
Comment on lines +504 to +505
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't this defined as "const literal" and then maybe const_assert or debug_assert? like:
https://github.com/anza-xyz/agave/blob/baed522d2/local-cluster/src/integration_tests.rs#L208

right now someone can change any of the two referenced constants unbeknown of how much it impacts this threshold here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention is really trying to get the max streams we allow in the throttle interval. So if the value is changed, we really want the change as well. But I think it makes sense to keep these constants together so people might realize the impact. I will address in master.

let stake_ratio = stake as f64 / total_stake as f64;
let peer_type = if stake_ratio < min_stake_ratio {
// If it is a staked connection with ultra low stake ratio, treat it as unstaked.
ConnectionPeerType::Unstaked
} else {
ConnectionPeerType::Staked(stake)
};
NewConnectionHandlerParams {
packet_sender,
Expand Down
4 changes: 2 additions & 2 deletions streamer/src/nonblocking/stream_throttle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ use {
};

/// Limit to 250K PPS
const MAX_STREAMS_PER_MS: u64 = 250;
pub const MAX_STREAMS_PER_MS: u64 = 250;
const MAX_UNSTAKED_STREAMS_PERCENT: u64 = 20;
const STREAM_THROTTLING_INTERVAL_MS: u64 = 100;
pub const STREAM_THROTTLING_INTERVAL_MS: u64 = 100;
pub const STREAM_STOP_CODE_THROTTLING: u32 = 15;
const STREAM_LOAD_EMA_INTERVAL_MS: u64 = 5;
const STREAM_LOAD_EMA_INTERVAL_COUNT: u64 = 10;
Expand Down
Loading