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.

Sign up for GitHub

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

Jump to bottom

Limit delayed ack timer to 1 RTT #1663

Merged
merged 3 commits into from
Aug 28, 2018
Merged

Limit delayed ack timer to 1 RTT #1663

merged 3 commits into from
Aug 28, 2018

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented Aug 15, 2018 •
edited
Loading

Partly addresses #1662

nibanks
nibanks approved these changes Aug 15, 2018
View reviewed changes
Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

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

LGTM

@mirjak
Copy link
Contributor

mirjak commented Aug 15, 2018

I guess it could still makes sense to also add a reference to the recovery draft there.

martinthomson
martinthomson approved these changes Aug 15, 2018
View reviewed changes
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, you are roaming into 6919-bis territory here :)

draft-ietf-quic-transport.md Outdated
containing only an ACK frame per received packet that contains frames other than
an ACK frame. Packets containing non-ACK frames MUST be acknowledged
immediately or when a delayed ack timer expires. The delayed ack timer should
never delay an ACK for longer than an RTT, which ensures an ACK frame is sent
Copy link
Member

Choose a reason for hiding this comment

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

"should never" or "MUST NOT"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or "does not," if you're making an assertion about the outcome of this strategy.

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -recovery labels Aug 27, 2018
@ianswett ianswett merged commit 3f7fc29 into master Aug 28, 2018
@martinthomson martinthomson deleted the ianswett-delayed-ack branch October 26, 2018 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeBishop MikeBishop MikeBishop left review comments

@nibanks nibanks nibanks approved these changes

@mirjak mirjak mirjak approved these changes

@martinthomson martinthomson martinthomson approved these changes

@janaiyengar janaiyengar Awaiting requested review from janaiyengar

Assignees
No one assigned
Labels
-recovery design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ianswett @mirjak @martinthomson @MikeBishop @nibanks