Re: [quicwg/base-drafts] Editorial recovery fixes (#1551)

[Martin Thomson <[e-mail]>]

martinthomson commented on this pull request.

A few suggestions.

> @@ -216,9 +215,9 @@ implemented in QUIC.
 
 An unacknowledged packet is marked as lost when an acknowledgment is received
 for a packet that was sent a threshold number of packets (kReorderingThreshold)
-after the unacknowledged packet. Receipt of the ack indicates that a later
-packet was received, while kReorderingThreshold provides some tolerance for
-reordering of packets in the network.
+and/or a threshold amount of time after the unacknowledged packet. Receipt of the
+ack indicates that a later packet was received, the reordering threshold

s/the ack/an acknowledgement/

Is this sentence missing an "If" at the start?  Maybe instead, "The reordering threshold provides some tolerance for reordering when acknowledgments indicate that packets were reordered."

> @@ -227,12 +226,13 @@ We derive this recommendation from TCP loss recovery {{?RFC5681}}
 reordering, causing a sender to detect spurious losses. Detecting spurious
 losses leads to unnecessary retransmissions and may result in degraded
 performance due to the actions of the congestion controller upon detecting
-loss. Implementers MAY use algorithms developed for TCP, such as TCP-NCR
-{{?RFC4653}}, to improve QUIC's reordering resilience, though care should be
-taken to map TCP specifics to QUIC correctly. Similarly, using time-based loss
-detection to deal with reordering, such as in PR-TCP, should be more readily
-usable in QUIC. Making QUIC deal with such networks is important open research,
-and implementers are encouraged to explore this space.
+loss and spurious retransmissions. Implementers MAY use algorithms developed
+for TCP, such as TCP-NCR {{?RFC4653}}, to improve QUIC's reordering resilience,
+though care should be taken to map TCP specifics to QUIC correctly.
+
+QUIC implementations may use time-based loss detection to deal with reordering,
+such as in PR-TCP. Making QUIC deal with such networks is important open
+research, and implementers are encouraged to explore this space.

MAY?  Cite and expand PR-TCP?

This paragraph looks like a good candidate for excision.  It's not really necessary for this document to encourage research.

> @@ -249,7 +249,7 @@ retransmittable packets are not acknowledged during this time, then these
 packets MUST be marked as lost.
 
 An endpoint SHOULD set the alarm such that a packet is marked as lost no earlier
-than 1.25 * max(SRTT, latest_RTT) since when it was sent.
+than 1.125 * max(SRTT, latest_RTT) since when it was sent.

This isn't really editorial.

> @@ -628,7 +629,7 @@ are as follows:
   this packet, but it is not retransmittable.
 
 * is_handshake_packet: A boolean that indicates whether a packet contains
-  handshake data.
+  a CRYPTO frame in a long header packet.

Maybe say " A boolean that indicates whether the packet contains cryptographic handshake messages critical to the completion of the QUIC handshake.  In this version of QUIC, this includes any packet with the long header that includes a CRYPTO frame."

> @@ -653,7 +654,7 @@ Pseudocode for OnPacketSent follows:
 
 ### On Receiving an Acknowledgment
 
-When an ACK frame is received, it may acknowledge 0 or more packets.
+When an ACK frame is received, it may acknowledge 0 or more new packets.

... it could newly acknowledge any number of packets.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1551#pullrequestreview-136482465