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

Discussion in quic-recovery of delayed ack being 25ms does not seem to relate to anything #3075

Closed
jrudolph opened this issue Oct 3, 2019 · 3 comments · Fixed by #3077
Closed
Assignees
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus.

Comments

@jrudolph
Copy link

jrudolph commented Oct 3, 2019

This paragraph didn't seem to relate to anything else in the document:

A shorter delayed ack time of 25ms was chosen because longer delayed acks can delay loss recovery and for the small number of connections where less than packet per 25ms is delivered, acking every packet is beneficial to congestion control and loss recovery.

Is it left-over from a previous version?

@ianswett ianswett added -recovery editorial An issue that does not affect the design of the protocol; does not require consensus. labels Oct 3, 2019
@ianswett
Copy link
Contributor

ianswett commented Oct 3, 2019

Thanks for pointing this out.

The intent was to explain why 25ms was chosen, given it's smaller than most TCP implementations default delayed ack time.

There were more exceptions before, but now there's only the one, which makes the preceding paragraph awkward as well.
"The majority of constants were derived from best common practices among widely deployed TCP implementations on the internet. Exceptions follow."

ianswett added a commit that referenced this issue Oct 3, 2019
The recommendation for a 25ms delayed ack time is in transport, not recovery, so remove it from recovery.

Should this paragraph be moved to transport?

Fixes #3075
@ianswett
Copy link
Contributor

ianswett commented Oct 3, 2019

I realized that the 25ms recommendation is in transport, so this is just out of place now.

@jrudolph
Copy link
Author

I realized that the 25ms recommendation is in transport, so this is just out of place now.

Ah, I see, I remember I went looking for it in transport, but I think I looked for "delayed ack" and didn't come up with anything fitting. I've got no concrete suggestion how to resolve it but moving the notice to transport in some form would probably make sense.

@ianswett ianswett self-assigned this Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants