Subject: [Gen-art] Gen-ART review of draft-ietf-core-etch-02

From: Christer Holmberg <[email-masked]>
Date: Tue, 20 Sep 2016 11:34:43 +0000

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>

Document:               draft-ietf-core-etch-02
Reviewer:               Christer Holmberg

Review Date:            20 September 2016
IETF LC End Date:        7 September 2016
IETF Telechat Date:     13 October 2016

Summary:

The document is well written, and is almost ready for publication as
standards track RFC. However,
I have a number of editorial issues that I¹d like the authors to address.

Major Issues:           None

Minor Issues:           None

Editorial Issues:

Q1 (Abstract):

The Abstract says:

          "The existing Constrained Application Protocol (CoAP) methods
only allow access to a
           complete resource, not to parts of a resource.²

I suggest to say ³The CoAP methods  defined in RFC 7252 only
allowŠ²Because as possible new CoAP methods
are defined in the future, the ³existing methods² statement may no longer
be true. This also makes it clear
that, when the document was written, RFC 7252 was the only spec you used
as input for existing methods.

Q2 (Abstract):

In general, I think the Abstract contains too much detail. Wouldn¹t it be
enough to keep the 1st paragraph, and
add the first paragraph of section 1?

Something like:   

          "The Constrained Application Protocol (CoAP) methods defined in
RFC 7252 only
           allow access to a complete resource, not to parts of a
resource.  In
           case of resources with larger or complex data, or in situations
where
           a resource continuity is required, replacing or requesting the
whole
           resource is undesirable.  Several applications using CoAP will
need
           to perform partial resource accesses.

           This specification defines the new Constrained Application
Protocol (CoAP) 
           methods, FETCH, PATCH and iPATCH, which are used to access and
update parts 
           of a resource.²

The rest of the Abstract text belongs e.g., in the Introduction section,
in my opinion.

Q3 (Introduction):

I think you need more background text before you start talking about the
methods. As suggested in Q2, move some
text from the Abstract to the Introduction.

Q4 (Section 2):

The text says: "Implementations MAY use a request body of any content type
with the FETCH method;²

First, I am not sure whether ³MAY² (uppercase) is appropriate here. What
about saying ³can² och ³may² (lowercase)?

Second, would it be better to say ³attach a body² instead of ³use a body²?

Third, I think it would be good to add text about "safe and idempotent²
also to the Security Considerations section.

Q5 (Section 2.2.2):

Please add reference for Content-Format option.

Q6 (Section 2.5):

Is there a reason this text is not in Section 4?

Q7 (Section 2.6):

First, why not simply calling the section ³Example² or ³FETCH Example²?
The same comment apply to
section 3.1 for PATCH/iPATCH examples.

Second, is the first paragraph really Example text? Isn¹t it normative
text that should be somewhere else?

Third, is a reference for JSON needed on first occurrence?

Fourth, I don¹t think you need to say ³might² in the "JSON document that
might be returned by GET² sentence. It¹s
an example, so simply say "JSON document returned by GET².

Q8 (Section 2 general):

Is there a reason there is no ³Error Handling² subsection for FETCH?

Q9 (Section 3):

As far as I know, HTTP (and CoAP, I assume) method names are case
insensitive. Even though it sounds cool and trendy, so
you really need to say ³iPATCH², and not ³IPATCH²? Implementers may not
thing that one MUST use lowercase ³i², which I
assume is not correct, and I am not aware of any other HTTP methods (or
SIP, RTSP etc methods) where one would mix
upper- and lower case in the method name.

Q10 (Section 4):

I think you should use another section name than ³Discussion².

---

From: peter van der Stok <[email-masked]>
Date: Wed, 28 Sep 2016 12:34:26 +0200

Hi Christer,

Thanks for your suggestions and comments.
A bit late reaction due to holidays.

See my answers below. It is not excluded that my co-author may refine 
(improve) my answers.

Peter

> as input for existing methods.

<pvds> True </pvds>
> The rest of the Abstract text belongs e.g., in the Introduction 
<pvds>
IMO, you are right, and your suggestion is an improvement
</pvds>
> text from the Abstract to the Introduction.
<pvds>
will do, but I like to keep the fetch and patch introductions separate 
as they are.
</pvds>
> about saying ³can² och ³may² (lowercase)?
<pvds>
We mean "MAY" as we can envisage different content formats for the 
future (e.g. one including query specifications) and we have a concrete 
case today.
</pvds>
> Second, would it be better to say ³attach a body² instead of ³use a 
<pvds> I have no opinion on this, may be Carsten? </pvds>
> also to the Security Considerations section.
<pvds>
That would go to section 1 before section 1.1; to provide the 
background.
</pvds>
> Please add reference for Content-Format option.
<pvds> section 5.10.3 to be added</pvds>
> Is there a reason this text is not in Section 4?
<pvds>
section 2.5 is about FETCH, while section 4 is about having 3 methods.
Renaming section 4 to "CoAP method set" may help?
<pvds>
> section 3.1 for PATCH/iPATCH examples.
<pvds>
The examples are really simple, and we did not want to raise too high 
expectations.
May be I am over sensitive on this point.
Just "<Method> example" will do as well.
</pvds>
> text that should be somewhere else?
<pvds>
I like the suggestion to put it before section 2.1 or generate a 
separate subsection.
</pvds>
> Third, is a reference for JSON needed on first occurrence?
<pvds>
Agree; good catch
</pvds>
> an example, so simply say "JSON document returned by GET².
<pvds>
agree
</pvds>
> Is there a reason there is no ³Error Handling² subsection for FETCH?
<pvds>
section 2.1 is sufficient in this case, because there are no issues that 
we are aware of.
</pvds>
> upper- and lower case in the method name.
<pvds>
Yes, the small "i" looks more sexy on iPATCH.
But I do understand your implementer arguments.
Unless Carsten sees this differently, I like to change to IPATCH as 
suggested.
</pvds>
> I think you should use another section name than ³Discussion².
<pvds>
"CoAP method set" is suggested under Q6.
</pvds>

---

From: Christer Holmberg <[email-masked]>
Date: Wed, 28 Sep 2016 16:17:36 +0000

Hi Peter,

I am happy with your reply, so I'll wait for the updated version of the draft :)

Regards,

Christer

-----Original Message-----
From: peter van der Stok [mailto:[email-masked]] 
Sent: 28 September 2016 13:34
To: Christer Holmberg <[email-masked]>
Cc: [email-masked]; [email-masked]
Subject: Re: Gen-ART review of draft-ietf-core-etch-02

Hi Christer,

Peter

Christer Holmberg schreef op 2016-09-20 13:34:
> standards track RFC. However, I have a number of editorial issues that 
> I¹d like the authors to address.
> 
> Major Issues:           None
> 
> Minor Issues:           None
> 
> Editorial Issues:
> 
> 
> Q1 (Abstract):
> 
> The Abstract says:
> 
>           "The existing Constrained Application Protocol (CoAP) 
> methods only allow access to a
> allowŠ²Because as possible new CoAP methods are defined in the future, 
> the ³existing methods² statement may no longer be true. This also 
> makes it clear that, when the document was written, RFC 7252 was the 
> only spec you used as input for existing methods.

<pvds> True </pvds>
> 
> 
> Q2 (Abstract):
> 
> In general, I think the Abstract contains too much detail. Wouldn¹t it 
> be enough to keep the 1st paragraph, and add the first paragraph of 
> section 1?
> 
> Something like:
> 
>           "The Constrained Application Protocol (CoAP) methods defined 
> in RFC 7252 only
>            case of resources with larger or complex data, or in 
> situations where
>            a resource continuity is required, replacing or requesting 
> the whole
>            resource is undesirable.  Several applications using CoAP 
> will need
>            methods, FETCH, PATCH and iPATCH, which are used to access 
> and update parts
>            of a resource.²
> 
> The rest of the Abstract text belongs e.g., in the Introduction 
> section, in my opinion.
<pvds>
IMO, you are right, and your suggestion is an improvement </pvds>
> 
> 
> Q3 (Introduction):
> 
> I think you need more background text before you start talking about 
> the methods. As suggested in Q2, move some text from the Abstract to 
> the Introduction.
<pvds>
will do, but I like to keep the fetch and patch introductions separate as they are.
</pvds>
> 
> 
> Q4 (Section 2):
> 
> The text says: "Implementations MAY use a request body of any content 
> type with the FETCH method;²
> 
<pvds>
We mean "MAY" as we can envisage different content formats for the future (e.g. one including query specifications) and we have a concrete case today.
</pvds>
> 
> Third, I think it would be good to add text about "safe and 
> idempotent² also to the Security Considerations section.
<pvds>
That would go to section 1 before section 1.1; to provide the background.
</pvds>
<pvds>
> section 3.1 for PATCH/iPATCH examples.
<pvds>
The examples are really simple, and we did not want to raise too high expectations.
May be I am over sensitive on this point.
Just "<Method> example" will do as well.
</pvds>
> text that should be somewhere else?
<pvds>
I like the suggestion to put it before section 2.1 or generate a separate subsection.
</pvds>
<pvds>
Agree; good catch
</pvds>
> 
> Fourth, I don¹t think you need to say ³might² in the "JSON document 
> that might be returned by GET² sentence. It¹s an example, so simply 
> say "JSON document returned by GET².
<pvds>
agree
</pvds>
> Is there a reason there is no ³Error Handling² subsection for FETCH?
<pvds>
section 2.1 is sufficient in this case, because there are no issues that we are aware of.
</pvds>
> insensitive. Even though it sounds cool and trendy, so you really need 
> to say ³iPATCH², and not ³IPATCH²? Implementers may not thing that one 
> MUST use lowercase ³i², which I assume is not correct, and I am not 
> aware of any other HTTP methods (or SIP, RTSP etc methods) where one 
> would mix
<pvds>
Yes, the small "i" looks more sexy on iPATCH.
But I do understand your implementer arguments.
Unless Carsten sees this differently, I like to change to IPATCH as suggested.
</pvds>
<pvds>