Re: [core] [Gen-art] Genart last call review of draft-ietf-core-new-block-10

[Lars Eggert <[e-mail]>]

Pete, thank you for your review and thank you all for the following discussion. I have entered a Discuss ballot for this document based on my own review.

Lars

On 2021-4-25, at 0:33, Pete Resnick via Datatracker <[e-mail]> wrote:
> 
> 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.
> 
> 
> 
> _______________________________________________
> Gen-art mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/gen-art