Thank you very much Matthias for the time spent on this extensive review.
I appreciate
-éric
On 02/09/2019, 15:09, "ietf on behalf of Matthias Kovatsch via Datatracker" <ietf-bounces@xxxxxxxx on behalf of noreply@xxxxxxxx> wrote:
Reviewer: Matthias Kovatsch
Review result: Ready with Nits
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
