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

Be clearer about persistent congestion #4429

Merged
merged 2 commits into from Dec 8, 2020
Merged

Be clearer about persistent congestion #4429

merged 2 commits into from Dec 8, 2020

Conversation

janaiyengar
Copy link
Contributor

@janaiyengar janaiyengar commented Dec 8, 2020

@msvoelker rightly noted lack of clarity (here), which this PR attempts to fix.

There's a new MUST here, but I think it was again implicit. EDIT: Removed this MUST since @martinthomson notes correctly that this is a definition, not an action.

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.

The MUST is weird. The other change is fine.

@@ -994,11 +994,11 @@ congestion without depending on PTO expiration.

### Establishing Persistent Congestion

A sender establishes persistent congestion after the receipt of an
acknowledgement if at least two ack-eliciting packets are declared lost, and:
A sender MUST establish persistent congestion after the receipt of an
Copy link
Member

Choose a reason for hiding this comment

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

MUST isn't necessary here.

Suggested change
A sender MUST establish persistent congestion after the receipt of an
A sender establishes persistent congestion after the receipt of an

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@martinthomson : Can you clarify why you think so? If you are thinking about enforceability, this document has several other unenforceable MUSTs, especially in the congestion controller.

@ianswett ianswett added -recovery editorial An issue that does not affect the design of the protocol; does not require consensus. labels Dec 8, 2020

* all packets, across all packet number spaces, sent between the send times of
two ack-eliciting packets are declared lost;
these two packets are declared lost;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we keep disagreeing about this, but I think paralellism is important. Above, they're called "two ack-eliciting packets" and below we drop 'ack-eliciting', which I find confusing.

If you don't want to restate ack-eliciting, then I'd suggest "if two packets that are ack-eliciting are declared lost,"...

A sender establishes persistent congestion after the receipt of an
acknowledgement if at least two ack-eliciting packets are declared lost, and:
A sender MUST establish persistent congestion after the receipt of an
acknowledgement if two ack-eliciting packets are declared lost, and:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
acknowledgement if two ack-eliciting packets are declared lost, and:
acknowledgement if two packets which are ack-eliciting are declared lost, and:


* all packets, across all packet number spaces, sent between the send times of
two ack-eliciting packets are declared lost;
these two packets are declared lost;

* the duration between the send times of these two packets exceeds the
persistent congestion duration ({{pc-duration}}); and
Copy link
Member

Choose a reason for hiding this comment

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

down in the third bullet, maybe s/both packets/these two packets/

@janaiyengar janaiyengar merged commit 354a6e7 into master Dec 8, 2020
@janaiyengar janaiyengar deleted the jri/these branch December 8, 2020 23:47
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