Hi,
[inline]
On 29 Apr 2021, at 7:52, mohamed.boucadair@xxxxxxxxxx wrote:
Hi Colin,
Thank you for the review.
Please see inline.
Cheers,
Med
-----Message d'origine-----
De : Colin Perkins via Datatracker [mailto:noreply@xxxxxxxx]
Envoyé : mercredi 28 avril 2021 23:55
À : tsv-art@xxxxxxxx
Cc : core@xxxxxxxx; draft-ietf-core-new-block.all@xxxxxxxx; last-
call@xxxxxxxx
Objet : Tsvart last call review of draft-ietf-core-new-block-11
Reviewer: Colin Perkins
Review result: Ready with Issues
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.
Thank you for preparing such a clearly written, precise,
specification. On the whole, this is very good. I just have some
minor issues to consider.
[Med] Thanks.
Section 4.1 says “To indicate support for Q-Block2 responses, the
CoAP client MUST include the Q-Block2 Option in a GET or similar
request (FETCH, for example), the Q-Block2 Option in a PUT or similar
request, or the Q-Block1 Option in a PUT or similar request so that
the server knows that the client supports this Q-Block
functionality”
– It would be useful to enumerate what are “similar” requests.
[Med] Argh, I thought we added an example as we did for GET. Thanks
for catching this. We can cite POST or PATCH.
Thanks.
Section 5: “Such messages must not be treated by the client as a
fatal error“
- I was surprised this is not a normative MUST NOT.
[Med] We don't use the normative language here as we though this is
implicitly covered by the behavior in 4.3 where we indicate that the
client retransmits the missing blocks when such error is received.
Yes, it probably is implicitly covered.
My underlying concern here is that the draft seems a little inconsistent
about when it uses the RFC 2119 normative language and when it
doesn’t. There are a few places where I see “must not” and wonder
why it’s not “MUST NOT”, and this was the most striking example.
Section 7.1: “For faster transmission rates, NSTART will need to be
increased from 1. However, the other CON congestion control
parameters will need to be tuned to cover this change. This tuning
is out of scope of this document as it is expected that all requests
and responses using Q-Block1 and Q-Block2 will be Non-confirmable
(Section 3.2).” - The way this is phrased is difficult to parse.
I can interpret it as saying that the transmission rate *does* need
to be faster, so implementations need to increase NSTART and tune the
other parameters.
Alternatively, I can interpret this as saying that *if* the
transmission needs to be faster, then NSTART and the other parameters
need to be tuned in some as-yet-unspecified way. The text would
benefit from being rephrased to clarify which meaning is intended.
What happens when NSTART is increased beyond 1, and how the other
parameters are tuned, is unclear. The text would be better if it
either cross-referenced to the definition of how the parameters are
to be tuned, or explicitly stated that this is not yet supported and
will need to be defined in some future extension.
[Med] Updated as follows:
OLD:
Congestion control for CON requests and responses is specified in
Section 4.7 of [RFC7252]. For faster transmission rates, NSTART
will
need to be increased from 1. However, the other CON congestion
control parameters will need to be tuned to cover this change.
This
tuning is out of scope of this document as it is expected that all
requests and responses using Q-Block1 and Q-Block2 will be Non-
confirmable (Section 3.2).
NEW:
Congestion control for CON requests and responses is specified in
Section 4.7 of [RFC7252]. In order to benefit from faster
transmission rates, NSTART will need to be increased from 1.
However, the other CON congestion control parameters will need to
be
tuned to cover this change. This tuning is not specified in this
document given that the applicability scope of the current
specification assumes that all requests and responses
using Q-Block1 and Q-Block2 will be Non-confirmable (Section 3.2).
Clearer, thanks.
In Section 7.2, I’m not convinced by the argument to set
MAX_PAYLOAD
to 10 for similar reasons to RFC 6928. The types of link layer that
CoAP runs over are very different to those measured to support the
increase in TCP’s initial window. An argument based on typical
properties of links and buffer space in networks used by CoAP would
be more convincing (I accept that using MAX_PAYLOAD of 10 is not
going to do any significant harm, even if it is higher than optimal).
[Med] Actually we set it to 10 as the applicability scope of this spec
is DOTS which runs in environments similar to those of 6928. Please
see Section 3.2.
This would be a lot clearer if Section 3.2 were cross-referenced, and a
reminder of the limited applicability of this specification was added to
Section 7.2.
Section 7.2 also notes that “PROBING_RATE and other transmission
parameters are negotiated between peers”. It would be appropriate
to
give some guidance on what are the bounds for safe values that can be
negotiated for these parameters.
[Med] I'm afraid this is out of the scope of this spec. The intent of
this note is to provide an example of an application that negotiates
these parameters. Some of these details can be found in in
rfc8782#section-4.5.2 mentioned in the text you quoted.
Makes sense, although I wonder if the text in Section 7.2 might be more
clearly written “The DOTS application uses customised defaults for
PROBING_RATE and other transmission parameters, as discussed in Section
4.5.2 of [RFC8782], that are negotiated between peers”?
Section 7.2 says:
As the sending of many payloads of a single body may itself cause
congestion, it is RECOMMENDED that after transmission of every
set of
MAX_PAYLOADS payloads of a single body, a delay is introduced of
NON_TIMEOUT before sending the next set of payloads to manage
potential congestion issues.
and the following paragraph has guidance for reducing MAX_PAYLOADS if
persistent congestion occurs “for at least a 24 hour period and it
is
known that there are no other network issues over that period”.
It’s
not clear how an implementation will know about other network issues,
[Med] An example is a DDoS attack. Made this change: s/about other
network issues/about other network issues (e.g., DDoS attacks)
Makes sense, thanks.
and I would suggest that even if there are such issues, backoff would
be appropriate given persistent congestion.
Finally, is there are mechanism for gradually recovering MAX_PAYLOADS
to its original value, if persistent loss ceases for some period?
[Med] This is covered by the configuration refresh/negotiation
mechanism. The peers will refresh the configuration parameters
following, for example, I-D.bosh-dots-quick-blocks.
Might be worth stating that in the draft?
Cheers,
Colin
--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call
