-
Notifications
You must be signed in to change notification settings - Fork 204
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
Add Definitions #1557
Add Definitions #1557
Conversation
draft-ietf-quic-recovery.md
Outdated
|
||
: The endpoint receiving QUIC packets and sending acknowledgements in | ||
response. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the above ones. In fact they may be more confusing than useful (sender of acks, for instance)
draft-ietf-quic-recovery.md
Outdated
: The endpoint receiving QUIC packets and sending acknowledgements in | ||
response. | ||
|
||
ACK-only: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this includes PADDING. Also, you'll want to fix all uses in the doc (ack-only, ack only)
draft-ietf-quic-recovery.md
Outdated
ACK-only: | ||
|
||
: Any packet containing only an ACK or ACK_ECN frame. The rest of this | ||
document uses "ACK frames" to refer to both ACK and ACK_ECN frames. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This second sentence doesn't seem to fit in this definition. I would separate it out into a new definition for now.
draft-ietf-quic-recovery.md
Outdated
In-flight: | ||
|
||
: Packets are considered in-flight because they have been sent | ||
and neither acknowledged or considered lost, and they are not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/or/nor/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it's or. But I changed considered to declared.
draft-ietf-quic-recovery.md
Outdated
|
||
In-flight: | ||
|
||
: Packets are considered in-flight because they have been sent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/because/when/
draft-ietf-quic-recovery.md
Outdated
Retransmittable: | ||
|
||
: Packets that contain frames besides ACK, ACK_ECN, or PADDING elicit | ||
an ACK from the receiver and are called retransmittable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should define retransmittable frames, and packets containing a retransmittable frame as a retransmittable packet.
draft-ietf-quic-recovery.md
Outdated
ACK-only: | ||
|
||
: Any packet containing only an ACK frame. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest being specific about ACK and ACK_ECN here
Retransmittable Packets: | ||
|
||
: Packets that contain retransmittable frames elicit an ACK from | ||
the receiver and are called retransmittable packets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both packets and frames aren't classified as retransmittable or not. It would be better if this used a different label.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinthomson : I don't understand your comment. Can you rephrase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The notion that a packet is retransmittable is a misnomer because it creates the impression that you can retransmit the packet. Similarly, we don't retransmit frames either. The distinction that is relevant to your uses of these terms is not whether the packet is able to be retransmitted, but whether the packet contains frames other than ACK/ACK_ECN/PADDING.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened issue #1595 for splitting up the terms, but I'd prefer do that separately.
aa14549
to
1ea3ce0
Compare
In-flight: | ||
|
||
: Packets are considered in-flight when they have been sent | ||
and neither acknowledged or declared lost, and they are not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late to the party, but:
Either/or, but Neither/nor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Mike, I fixed it in master
Add a separate definitions section, similar to the transport doc.