Last Call Review of draft-ietf-core-senml-etch-05
review-ietf-core-senml-etch-05-iotdir-lc-kovatsch-2019-09-02-00

Request Review of draft-ietf-core-senml-etch
Requested rev. no specific revision (document currently at 07)
Type Last Call Review
Team Internet of Things Directorate (iotdir)
Deadline 2019-09-02
Requested 2019-08-21
Requested by Éric Vyncke
Authors Ari Keränen, Mojan Mohajer
Draft last updated 2019-09-02
Completed reviews Opsdir Last Call review of -05 by Carlos Pignataro (diff)
Iotdir Last Call review of -05 by Matthias Kovatsch (diff)
Secdir Last Call review of -05 by Christian Huitema (diff)
Genart Last Call review of -05 by Robert Sparks (diff)
Comments
I would appreciate a review of this document with the IoT point of view as it targets constrained device.

Please share the review with the ietf@ietf.org mailing list as it is a Last Call review.

Thank you for reviewing it.

-éric
Assignment Reviewer Matthias Kovatsch 
State Completed
Review review-ietf-core-senml-etch-05-iotdir-lc-kovatsch-2019-09-02
Posted at https://mailarchive.ietf.org/arch/msg/Iot-dir/v9eiRPRGgb9qithBe-fdhEpWshE
Reviewed rev. 05 (document currently at 07)
Review result Ready with Nits
Review completed: 2019-09-02

Review
review-ietf-core-senml-etch-05-iotdir-lc-kovatsch-2019-09-02

Dear authors and list members

Here is my review for draft-ietf-core-senml-etch-05 from the IoT perspective.

## Summary

draft-ietf-core-senml-etch-05 defines new media types and their semantics for two new SenML patch document formats (JSON and CBOR, resp.). The complexity added to implementations that can already handle SenML is marginal and straight-forward. Hence, I do not see any issue for constrained devices. The explicit media types help in IoT scenarios, where machines communicate with machines.

There are a few minor issues that can be solved by the authors alone. Hence, I marked the result as "Ready with Nits".

It would be good to get the help from an expert on Windows Clipboard Formats and Macintosh Uniform Type Identifiers, as no good guidelines are available to check these IANA considerations. (This issue appears to be recurrent also for other specs.)

## Technical comments

* P4 (3.1): I am missing assertions such as "Values in a Fetch Record MUST be ignored."
  * What should happen when a Patch Record does not have a value?

* P5 §3: The record must not be added when the value is null. (behavior not described formally enough)

* P7: "Windows Clipboard Name" --> Microsoft and for instance HTML spec use "Windows Clipboard Format"
  * Okay, the sting itself is the Windows Clipboard Format Name...
  * The long string with spaces ("SenML FETCH/PATCH format") is a bit weird for this purpose, no?
    * I also had the problem to find proper guidelines for Windows Clipboard Formats; are there any?
  * No Macintosh Uniform Type Identifier?

## Additional comment

* As already discussed with one of the authors, an implication for LwM2M is probably that these patch documents must not be used with Executable Resources (one might try to execute multiple resources at once with a PATCH method). The application of a Patch Pack is then not idempotent anymore. Furthermore, it is unclear what the value should be when the LwM2M Executable Resource does not take arguments.
* If executing multiple resources atomically is an important use case, I think we need another iteration to deal with the state vs RPC issue ("use PATCH to call function(s) without arguments by giving a new state?!")

## Editorial comments

* P1 §1 (Abstract), P2 last §: "iPATCH, PATCH, and FETCH" --> "FETCH, PATCH, and iPATCH"
  * It is easier on the brain if the order is kept consistent...

* P2 §6: "hence full name" --> "hence the unique identifiers" ?
  * RFC 8428 does not define or contain "full name", but "globally unique identifier for the resource"

* P3 §1: "The semantics of the ..."
  * Creates question about semantics for FETCH
  * Better to reverse sentences and start with "The rest of the document uses the term "(i)PATCH" to refer to both methods, as the semantics of the new media types are the same for the CoAP PATCH and iPATCH methods."

* P3 §1: ", that can be used with the" --> ", which ..."
* P3 §3: "Also the following ..." --> to many "also", just "The following ..."

* P4 §2 (3.1): "... when resolved, match resolved names" --> "identifiers"
  * names when resolved are resolved names, hence unclear what is compared
  * P2 calls them "full names"
  * See above, should be something like "globally unique identifier for the resource"
* P4 §8: Add example for records with name and time
  * Would be good to quickly show what "resolved form of records" means
* P4 §9 (3.2): Add statement that SenML patch documents are always idempotent, hence PATCH and iPATCH are equivalent?
  * Basically move the last sentence to the beginning and give explanation for "(i)PATCH".

* P5 §2: "When the name" --> "When the resolved name" ?

Kind regards,
Matthias