-
Notifications
You must be signed in to change notification settings - Fork 221
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
Quic: Ack overflow probes, 10x idle_timeout buffer #4188
base: main
Are you sure you want to change the base?
Conversation
|
313c8c7
to
e906f57
Compare
e906f57
to
91712cd
Compare
Fix the commit message please |
91712cd
to
a22bb2a
Compare
a22bb2a
to
918742b
Compare
@@ -945,7 +945,7 @@ dynamic_port_range = "8900-9000" | |||
idle_timeout_millis = 10000 | |||
|
|||
# Max delay for outgoing ACKs. | |||
ack_delay_millis = 50 | |||
ack_delay_millis = 25 |
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.
Why are we changing the max ack delay in the default.toml?
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.
Using this branch mostly for testing rn, but just worth noting 25 is what the QUIC spec recommends. Directionally, this may contribute to our under-delivery issue (although unlikely)
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'm not sure we have an under-delivery issue :-)
But PRs that add tests or change testing behavior should not change production config, as a general rule.
a262bca
to
82a9521
Compare
This PR adds probe points related to ACK behavior, and a new metric for when the ACK buffer overflows.