-
Notifications
You must be signed in to change notification settings - Fork 204
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
eliminate the difference between when having no RTT sample and having one #3525
Conversation
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 think this is a nice simplification, and one I've considered before, but I think we need to be careful to ensure an actual RTT measurement always erases any historical or default values.
@ianswett Thank you for the review. Would you ming checking if the updated text has what you have suggested? |
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
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.
Thank you, @kazuho, this is a good fix. This also fixes the case where the 2 x initialRtt is smaller than granularity.
Co-Authored-By: Jana Iyengar <jri.ietf@gmail.com>
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.
Just a few editorial suggestions.
Co-Authored-By: Jana Iyengar <jri.ietf@gmail.com>
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 think Jana's suggestions LGTM.
Co-Authored-By: Jana Iyengar <jri.ietf@gmail.com>
Co-Authored-By: Jana Iyengar <jri.ietf@gmail.com>
@janaiyengar @ianswett Thank you for the suggestions. I've applied them. |
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.
LGTM - thanks, @kazuho! Can you resolve the merge conflict?
As pointed out in #3524, I think that we should assume same amount of RTT variance when there is no RTT sample and when there is one sample, because variance is unknown in both cases.
This PR makes that change while keeping the initial PTO being the same (i.e. 1 second), and slightly simplifies the timeout logic.
Fixes #3524.