Reviewer: Joerg Ott Review result: Ready with Nits This document has been reviewed as part of the transport area review team's ongoing effort to review key IETF documents. These comments were written primarily for the transport area directors, but are copied to the document's authors and WG to allow them to address any issues raised and also to the IETF discussion list for information. When done at the time of IETF Last Call, the authors should consider this review as part of the last-call comments they receive. Please always CC tsv-art@xxxxxxxx if you reply to or forward this review. The Internet draft describes a revision of the HTTP/2 framing and optimized representation, revising RFC 7540. The document is in a very good and essentially ready to go, possibly subject to a few considerations and clarifications discussed below. Best, Jörg Section 5.1.1., 3rd para: When a stream transitions out of the "idle" state, all streams that might have been initiated by that peer with a lower-valued stream identifier are implicitly transitioned to "closed". Should one say "that might have been but were not initiated by that peers" to make clear that this applies only to un-used streams? Section 5.3.1 Maybe update the section heading to read "Background on Priority in HTTP/2 as per RFC7540"? Section 6.5: The document implies that SETTINGS frames are ACKed in the order they are sent. Should one make this explicit? Last para of 6.5, top of page 37: "If the sender of a SETTINGS frame does not receive an acknowledgement within a reasonable amount of time, it MAY issue a connection error (Section 5.4.1) of type SETTINGS_TIMEOUT." Should one provide some intuition on what "reasonable" could mean? An example? It seems, an HTTP/2 sender of a SETTINGS frame could know when this was passed to the TCP layer and may have some insights on application layer RTTs. Section 6.9.1: Concerning flow control, should one say something about avoiding silly windows? Section 8.1.1, p52 1st para: Probably just me, but I am mildly confused by the statement about messages having a non-zero content-length field but still no data in the data frames. The reference section 6.4 of the HTTP semantics draft didn't resolve that. Section 8.2.1, p53: parse error? "When a request message violates one of these requirements, an implementation SHOULD generate a Section 15.5.1 of 400 (Bad Request) status code [HTTP], [...]" Section 8.4.2, 4th para: "A client can use the SETTINGS_MAX_CONCURRENT_STREAMS setting to limit the number of responses that can be concurrently pushed by a server. Advertising a SETTINGS_MAX_CONCURRENT_STREAMS value of zero disables server push by preventing the server from creating the necessary streams. This does not prohibit a server from sending PUSH_PROMISE frames; clients need to reset any promised streams that are not wanted. Confused: it disables server push, but the server can send PUSH_PROMISE frames and then the client is supposed to reset a stream it wasn't willing to accept or capable of accepting? Which stream number would the server legitimately put into the PUSH_PROMISE? There are a few cases where I am wondering if normative language should be used. Examples include -- 5.4.2, 2nd para, "sends an RST_STREAM frame" -- 6.8, p43, 1st para, "the server can send another GOAWAY" -- 6.8, p43, 2nd para, "the sender can discard frames" -- 9.1, 3rd para, "clients can create" There may be further occurrences of "can" or similar. -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
