Reviewer: Pete Resnick Review result: Ready with Issues I am the assigned Gen-ART reviewer for this draft. The General Area Review Team (Gen-ART) reviews all IETF documents being processed by the IESG for the IETF Chair. Please treat these comments just like any other last call comments. For more information, please see the FAQ at <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. Document: draft-ietf-core-new-block-10 Reviewer: Pete Resnick Review Date: 2021-04-24 IETF LC End Date: 2021-04-28 IESG Telechat date: Not scheduled for a telechat Summary: The document looks pretty solid to me. There is one item I marked as a minor "issue", but it may simply be an editorial item that confused me; I figured I'd call it an issue just in case so it doesn't get left to the last minute to look at. Do note that I have not reviewed the examples for correctness; I simply don't have the expertise to be convinced I'd do it right. Major issues: None. Minor issues: In section 4.4: I find this paragraph confusing: The requested missing block numbers MUST have an increasing block number in each additional Q-Block2 Option with no duplicates. The server SHOULD respond with a 4.00 (Bad Request) to requests not adhering to this behavior. So, given the SHOULD in the second sentence, it appears that the MUST in the first sentence doesn't apply to the server (i.e., to enforce this), but rather to the client doing the request. You should probably say it that way. Also, the SHOULD in the second sentence is not entirely clear to me: Are you saying that the server can choose to use some other response code, or are you saying that the server can accept the request and do something interesting with it? Below is an attempt to fix it, but might not be correct depending on what you mean: The client MUST use an increasing block number in each additional Q-Block2 Option when requesting missing block numbers, and MUST request no duplicates. The server MUST reject requests not adhering to this behavior and SHOULD respond with a 4.00 (Bad Request) to such requests. There are other places in the document that use MUST with regard to what needs to be in a piece of data (see for example sections 4.5 and 4.6), but don't make it clear who is responsible for enforcing that MUST (the client or the server). You should read through the entire document for MUSTs (or SHOULDs) like that and make sure it's clear from the context. Nits/editorial comments: In section 4.3: In several response code definitions: The token used MUST be any token that was received in a request using the same Request-Tag. That doesn't really parse well. I think you either mean "The token used MUST be a token" or you mean "The token used can be any token". Specific response codes: 4.00 (Bad Request) This Response Code MUST be returned if the request does not include neither a Request-Tag Option nor a Size1 Option but does include a Q-Block1 option. Either change "neither...nor" to "either or", or change "does not include" to "includes". 4.02 (Bad Option) Either this Response Code (in case of Confirmable request) or a reset message (in case of Non-confirmable request) MUST be returned if the server does not support the Q-Block Options. That sort of buries a MUST requirement for the Non-confirmable case inside this requirement for a Response Code. I suggest instead: This Response Code MUST be returned for a Confirmable request if the server does not support the Q-Block Options. (A reset message is sent in case of Non-confirmable request.) In section 4.4: The passive here is not great form, particularly because it doesn't name the actor: It is permissible to set the M bit to request this... How about instead: The client MAY set the M bit to request this... Maybe that's obvious, since the client does the requesting, but I think the non-passive form is easier to read. In the second to last paragraph: If the server transmits a new body of data (e.g., a triggered Observe notification) with a new ETag to the same client as an additional response, the client should remove any partially received body held for a previous ETag for that resource as it is unlikely the missing blocks can be retrieved. I'm ambivalent about whether that "should" ought to be uppercased, but I just wanted to make sure you intended the lowercase. In section 7.2: For the server receiving NON Q-Block1 requests, it SHOULD send back a 2.31 (Continue) Response Code on receipt of all of the MAX_PAYLOADS payloads to prevent the client unnecessarily delaying. Otherwise... When you say "Otherwise", Do you mean, "For other payloads"? Either way, you should probably change that to make it clear. -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
