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.

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

tighten the minimal streams per 100ms for staked node #738

Conversation

lijunwangs
Copy link

Problem

The small staked's minimal streams per second is too generous. The one barely be entitled to 10 PPS can get 80 making it effective in QOS.

Summary of Changes

This change achieve the following results which matches master's behavior

1, On the tpu forward port where unstaked connections is 0, set the staked node's minimal PPS to 10 instead of 80
2. On the tpu port, set the staked nodes's minimal streams per 100MS to the the one of unstaked connection's + 1 in 100MS. This encourage more staked vs unstaked.

Fixes #

@lijunwangs lijunwangs requested a review from t-nelson April 11, 2024 00:53
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 81.6%. Comparing base (be83469) to head (da12adc).

Additional details and impacted files
@@           Coverage Diff           @@
##            v1.17     #738   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         806      806           
  Lines      219284   219303   +19     
=======================================
+ Hits       179014   179072   +58     
+ Misses      40270    40231   -39     

@lijunwangs lijunwangs merged commit c38894e into anza-xyz:v1.17 Apr 11, 2024
33 checks passed
Percentage::from(MAX_UNSTAKED_STREAMS_PERCENT)
.apply_to(MAX_STREAMS_PER_100MS)
.saturating_div(MAX_UNSTAKED_CONNECTIONS as u64)
};

let min_staked_streams_per_100ms = if max_unstaked_connections == 0 {

Choose a reason for hiding this comment

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

max_unstaked_connections == 0 please tell me that this isn't a sentinel for tpufwd port

Copy link
Author

Choose a reason for hiding this comment

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

It is. But what it really means is also on that port we allow 0 unstaked connections and only staked ones. So literally it is correct. It does not matter if it is used for forward or not.

Choose a reason for hiding this comment

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

i know what it means. that's not the problem. the problem is that it is extremely brittle logic that can be broken from other areas of the code base with no knowledge of its existence. we cannot have code like this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants