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

Recovery RFCEditor #3: Why reset min_rtt #4891

Merged
merged 6 commits into from May 11, 2021
Merged

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented May 6, 2021

  1. [rfced] Does the following improve the readability of the paragraph? 

Current:
   Endpoints SHOULD set the min_rtt to the newest RTT sample after
   persistent congestion is established. This is to allow a connection
   to reset its estimate of min_rtt and smoothed_rtt after a
   disruptive network event (Section 5.3), and because it is possible
   that an increase in path delay resulted in persistent congestion
   being incorrectly declared.

Perhaps:
   Endpoints SHOULD set the min_rtt to the newest RTT sample after
   persistent congestion is established because it is possible that
   an increase in path delay resulted in persistent congestion being
   incorrectly declared. This also allows a connection to reset its
   estimate of min_rtt and smoothed_rtt after a disruptive network
   event (Section 5.3).

@ianswett
Copy link
Contributor Author

ianswett commented May 6, 2021

I find this easier to read, but I think it may be subtly different, so I put it into a separate PR.

Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

It doesn't change the meaning, but it does feel like the first sentence is trying to span too many topics. I like the reordering, but I'd split it out.

rfc9002.md Outdated Show resolved Hide resolved
Co-authored-by: Mike Bishop <mbishop@evequefou.be>
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

Careful with these, not to pick up bad stuff.

rfc9002.md Outdated Show resolved Hide resolved
rfc9002.md Outdated Show resolved Hide resolved
ianswett and others added 2 commits May 7, 2021 09:23
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Co-authored-by: Martin Thomson <mt@lowentropy.net>
Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

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

This is good, thank you !

rfc9002.md Outdated
of min_rtt and smoothed_rtt ({{smoothed-rtt}}) after a disruptive network event,
and because it is possible that an increase in path delay resulted in persistent
congestion being incorrectly declared.
congestion is established. This helps when an increase in path delay
Copy link
Member

Choose a reason for hiding this comment

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

Helps what?

The key is that it avoids repeatedly entering persistent congestion if the path RTT has genuinely increased.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, PTAL

rfc9002.md Outdated Show resolved Hide resolved
@janaiyengar janaiyengar merged commit e556c48 into master May 11, 2021
@janaiyengar janaiyengar deleted the ianswett-rfceditor3 branch May 11, 2021 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants