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

Congestion control section in recovery should be self-contained #3088

Closed
jrudolph opened this issue Oct 12, 2019 · 2 comments · Fixed by #3463
Closed

Congestion control section in recovery should be self-contained #3088

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

Comments

@jrudolph
Copy link

It starts like this:

QUIC's congestion control is based on TCP NewReno {{?RFC6582}}.

The next sections below roughly describe the behavior of different phases of congestion control. More concrete pseudo-code is only given in the Appendix later on but it's not linked from the original section.

On first reading, I missed the Appendix and felt the text lacking for concrete enough explanations to implement the feature. The question is what to describe where and what the authoritative definitions should be. If you know the NewReno RFC by heart you can probably make the right inferences to figure out how everything should work in comparison but is that how it should be?

An alternative (more convenient for the reader / skimmer) approach could be to mention NewReno only in passing and then provide a complete and self-contained description of the algorithms in the congestion control section directly, maybe even inlining the pseudo-code. If that is deemed to verbose and would bore readers that know enough about those algorithms already, it would be good to provide some guidance how to read the algorithm and what the assumptions are in the introductory paragraph about congestion control.

@ianswett
Copy link
Contributor

The intent is that the text is complete without reading the referenced RFCs or pseudocode, so I think this section needs more text and possibly some inlined pseudo-code.

@ianswett ianswett added -recovery editorial An issue that does not affect the design of the protocol; does not require consensus. labels Oct 12, 2019
@ianswett ianswett changed the title Congestion control section in recovery migth be easier to understand if it was self-contained Congestion control section in recovery should be self-contained Oct 14, 2019
ianswett added a commit that referenced this issue Feb 15, 2020
Clarifies a few points and moves the initial window recommendation from the pseudocode to the text.

Fixes #3088
@ianswett
Copy link
Contributor

Hi @jrudolph I wrote #3463 which fixes some obvious errors I found, but I think more could be done.

Can you take a look and tell me what other points you'd like added or moved up from the pseudocode?

@ianswett ianswett self-assigned this Feb 15, 2020
martinthomson pushed a commit that referenced this issue Mar 4, 2020
Clarifies a few points and moves the initial window recommendation from the pseudocode to the text.

Fixes #3088
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