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

is_retransmittable semantics #591

Closed
gloinul opened this issue Jun 6, 2017 · 3 comments
Closed

is_retransmittable semantics #591

gloinul opened this issue Jun 6, 2017 · 3 comments
Assignees
@ianswett
Labels
-recovery design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@gloinul
Copy link
Contributor

gloinul commented Jun 6, 2017

In draft-ietf-quic-recovery-03 there appear to me to be an overloading of the use of the "is_retransmittable" flag. It appears to be used both for the purpose to indicate if there is a frame that can and need be retransmitted and if that frame should be counted as lost and impact congestion control. I think these two purposes should be separated and clarified as being two different ones. The reason is that if we create extensions that will not require retransmission of frames but needs to be included in the congestion control response a significant rewrite would be needed. I also think it would clarify things for some of the control frames also if they should be included or not, even if not regularly retransmitted.
@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -recovery labels Jun 6, 2017
@martinthomson martinthomson changed the title Recovery: Overloading of is_retransmittable Overloading of is_retransmittable Jun 20, 2017
@mnot mnot changed the title Overloading of is_retransmittable is_retransmittable semantics Jun 21, 2017
@ianswett
Copy link
Contributor

Agreed that these concepts need to be separated.

I replaced is_retransmittable with is_ack_only in PR #780, because padding is not retransmittable, but does count towards bytes in flight. My intent is to make everything that is not an ack only packet count towards bytes_in_flight.

But I think there's still some more editorial work to clarify the separation you describe.

@gloinul
Copy link
Contributor Author

gloinul commented Mar 9, 2018

This is still not clear in -10, will keep it open.

@ianswett
Copy link
Contributor

The plan is to rename this to needs_acknowledgement as well, because QUIC rarely retransmits frames and never retransmits packets.

ianswett added a commit that referenced this issue Mar 15, 2018
@ianswett
No more Retransmittable
0e51def 
Fixes Issue #591
@ianswett ianswett mentioned this issue Mar 15, 2018
Merged
ianswett added a commit that referenced this issue Mar 17, 2018
@ianswett
No more Ackable (#1211)
ae251f6 
* No more Retransmittable

Fixes Issue #591

* Marten's comment

* Hyphenation and suggestions

* Update draft-ietf-quic-recovery.md

* Congestion-controlled

* One more

* No more ackable

* Update draft-ietf-quic-recovery.md

* Update draft-ietf-quic-recovery.md

* Update draft-ietf-quic-recovery.md

* Update draft-ietf-quic-recovery.md

* Back to retransmittable

* retrasnmittable
@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ianswett ianswett

Labels
-recovery design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@martinthomson @mnot @gloinul @ianswett