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

Recovery-31: Minor editorial issues found in AD review #4196

Closed
gloinul opened this issue Oct 12, 2020 · 3 comments · Fixed by #4213 or #4214
Closed

Recovery-31: Minor editorial issues found in AD review #4196

gloinul opened this issue Oct 12, 2020 · 3 comments · Fixed by #4213 or #4214
Assignees
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus. ietf-lc An issue that was raised during IETF Last Call.

Comments

@gloinul
Copy link
Contributor

gloinul commented Oct 12, 2020

This issue will list a number of minor editorial things that should be checked and likely addressed with some editorial fixes:

  1. Section 5.3: "These delays are computed using the ACK Delay field of the ACK frame as described in Section 19.3 of [QUIC-TRANSPORT]." This text implies that the computation of the delay is described in Section 19.3 of QUIC-TRANSPORT. However, I don't think it is described the calculation, only the definition of the field. Please reformulate to imply only field definition not calculation.
  2. Section 5.3: "On the first RTT sample for the network path, that sample is used as rtt_sample. This ensures that the first measurement erases the history of any persisted or default values." I think this intended to cover cases where path changes are detected. But it is not well expressed. Does this need a reformulation and a reference into QUIC-TRANSPORT where it states when such a reset should occur?
  3. Section 6.1: "Either its packet number is kPacketThreshold smaller than an acknowledged packet" Nitpicking, assuming no PN gaps in transmission. Does that need a note?
  4. Section 6.2.4: "An endpoint MAY send up to two full-sized datagrams containing ack-eliciting packets, to avoid an expensive consecutive PTO expiration due to a single lost datagram or transmit data from multiple packet number spaces." This last part, I assume intended to generate additional datagrams to prevent single loss to impact. However, due to coalescing this is unclear in this context.
  5. Section 7: "Endpoints can unilaterally choose a different algorithm to use, such as Cubic ([RFC8312])." I think this sentence should be explicit that in needs to be sender-based algorithms. I think we need to have the requirement on performing congestion control on the sender unless there has been explicit agreement with the receiver that they will do it.
  6. Section 7.2: "Endpoints SHOULD use an initial congestion window of 10 times the maximum datagram size (max_datagram_size), limited to the larger of 14720 bytes or twice the maximum datagram size." "maximum datagram size" is not used in Transport there need to be alignment here of what name to use. Transport has Maximum Packet Size, which appears to match the definition in the appendix of max_datagram_size.
@gloinul gloinul changed the title Editorial minor issues in receovery-31 found in AD review. eceovery-31: Editorial minor issues in found in AD review. Oct 12, 2020
@gloinul gloinul changed the title eceovery-31: Editorial minor issues in found in AD review. Receovery-31: Minor editorial issues found in AD review Oct 12, 2020
@larseggert larseggert added the ietf-lc An issue that was raised during IETF Last Call. label Oct 13, 2020
@martinthomson martinthomson changed the title Receovery-31: Minor editorial issues found in AD review Recovery-31: Minor editorial issues found in AD review Oct 13, 2020
@janaiyengar janaiyengar self-assigned this Oct 13, 2020
@janaiyengar
Copy link
Contributor

Thanks for your careful review, @gloinul -- these are good finds! I've addressed your comments below and in #4213. I'll address point 6 in a separate PR.

  1. Section 19.3 has the following text, which does tell how to compute acknowledgement delay from the ACK Delay field:
ACK Delay:
    A variable-length integer encoding the acknowledgement delay in microseconds; see Section 13.2.5. 
    It is decoded by multiplying the value in the field by 2 to the power of the ack_delay_exponent transport
    parameter sent by the sender of the ACK frame; see Section 18.2. Compared to simply expressing the
    delay as an integer, this encoding allows for a larger range of values within the same number of bytes,
    at the cost of lower resolution.

I've replaced "computed" with "decoded" in the recovery document to make it clearer.

  1. Coalescing applies to packets, not to datagrams. We've explicitly stated that a sender could send 2 datagrams to avoid any effects of coalescing. I think the text is pretty clear on this: "An endpoint MAY send up to two full-sized datagrams containing ack-eliciting packets".

  2. I've added "sender-side" to make the controller more explicit. To your point about making this a sender requirement, that is covered in the transport doc (end of Section 13.3):

Upon detecting losses, a sender MUST take appropriate congestion control action.
The details of loss detection and congestion control are described in [QUIC-RECOVERY].

@larseggert
Copy link
Member

Based on #4213 and #4214, labeling this as "editorial"

@larseggert larseggert added the editorial An issue that does not affect the design of the protocol; does not require consensus. label Oct 15, 2020
This was linked to pull requests Oct 15, 2020
@gloinul
Copy link
Contributor Author

gloinul commented Oct 15, 2020

I think these issues are resolved once the two PRs are completely ready.

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. ietf-lc An issue that was raised during IETF Last Call.
Projects
None yet
3 participants