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

Move MUST not send bursts up in the pacing section #3664

Merged
merged 2 commits into from May 30, 2020

Conversation

ianswett
Copy link
Contributor

I prefer to have MUSTs towards the top of sections when possible so they're not missed.

This is a follow-on from #3630

Comment on lines -883 to -885
controller. For example, a pacer might distribute the congestion window over
the smoothed RTT when used with a window-based controller, or a pacer might use
the rate estimate of a rate-based controller.
Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with leaving this out, but I'm interested in why you did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was redundant to the text you added below.

@janaiyengar
Copy link
Contributor

I'm not a fan of this change -- I think there's value in having that text below where we talk about how bursts can occur. That said, I don't feel strongly about it, so I'm not going to fight for it.

@martinthomson martinthomson added -recovery editorial An issue that does not affect the design of the protocol; does not require consensus. labels May 20, 2020
@janaiyengar janaiyengar merged commit 72e6455 into master May 30, 2020
@janaiyengar janaiyengar deleted the ianswett-pacing-must branch May 30, 2020 02:35
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 this pull request may close these issues.

None yet

3 participants