-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ENH] Multi-dimensional admission control. #3579
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
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
rust/mdac/src/scorecard.rs
Outdated
rules: Vec<Rule>, | ||
estimate_thread_count: usize, | ||
) -> Self { | ||
assert!(estimate_thread_count > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NonZeroUsize.
rust/mdac/src/scorecard.rs
Outdated
#[test] | ||
fn basics() { | ||
let metrics = TestMetrics::default(); | ||
// NOTE(rescrv): We diverge from the upstream behavior here. If a tag doesn't match a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we clarify what upstream is please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
rust/frontend/frontend_config.yaml
Outdated
request_timeout_ms: 60000 | ||
circuit_breaker: | ||
requests: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the default be higher?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the default to make it not enabled by default.
rust/frontend/src/frontend.rs
Outdated
} | ||
|
||
impl Frontend { | ||
pub fn new(sysdb_client: Box<sysdb::SysDb>) -> Self { | ||
let scorecard_enabled = Arc::new(AtomicBool::new(false)); | ||
let scorecard = Arc::new(Scorecard::new(&(), vec![], 128)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the thread count be provided by config or inferred from the env?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
128 is sufficient for everything we need. I think inferring it would do us strictly worse if we get it wrong as it's an upper bound.
This PR creates a multi-dimensional "scorecard" that tags traffic and a circuit breaker that rejects excess traffic.