Last Call Review of draft-ietf-ccamp-layer1-types-04
review-ietf-ccamp-layer1-types-04-yangdoctors-lc-wilton-2019-12-13-00

Request Review of draft-ietf-ccamp-layer1-types-03
Requested rev. 03 (document currently at 05)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2019-12-04
Requested 2019-11-12
Requested by Daniele Ceccarelli
Authors Haomian Zheng, Italo Busi
Draft last updated 2019-12-13
Completed reviews Yangdoctors Last Call review of -04 by Robert Wilton (diff)
Comments
This draft is connected with https://datatracker.ietf.org/doc/draft-ietf-ccamp-layer0-types/. Would it be possible to have them reviewed together?
Assignment Reviewer Robert Wilton
State Completed
Review review-ietf-ccamp-layer1-types-04-yangdoctors-lc-wilton-2019-12-13
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/jxBtvfelcHCwSANvH2UJtBgaWnc
Reviewed rev. 04 (document currently at 05)
Review result Almost Ready
Review completed: 2019-12-13

Review
review-ietf-ccamp-layer1-types-04-yangdoctors-lc-wilton-2019-12-13

Comments on the document:

1) Please check the English grammar for the earlier paragraphs.

2) If you change the prefix (as in the YANG comments below) then there are references in the document that will also need to be updated (e.g. section 3, and the IANA section).

3) Security section, "onsidered" => "considered"


Comments on the YANG module:

1) I would suggest changing the YANG prefix to "l1-types" to help keep it short (particularly for the identities).

2) I would suggest making the module "yang-version 1.1;", because the behaviour of YANG 1.1 is better specified.  In fact, I would recommend that all IETF YANG modules are defined as YANG 1.1.

3) As per my comments in the layer-0-types module, I would suggest using a decimal64 for the tributary-slot-granularity, or perhaps even a uint32 if it is specified in Mega rather than Giga.  Also, should the units be Ghz?

4) For all the ODU-type definitions, I would recommend splitting the description from the reference.  I also don't think that the "which is categorized as standards track is necessary/useful.  Or if you don't want to keep it, then I would retain it only for types that are not standard."

E.g. 
OLD:

 identity ODU0 {
   base odu-type;
   description
     "ODU0 protocol (1.24G), RFC7139/ITU-T G.709, which is
      categorized as standards track .";
 }

NEW:
 identity ODU0 {
   base odu-type;
   description "ODU0 protocol (1.24G)"
   reference "RFC7139/ITU-T G.709";
 }

5) Some of the ODU-types don't have references.  I would be good to add references to everything that you can (e.g. also the client-signal identities)

6) For the description of ETH-10Gb-WAN, it might be helpful to also specify the actual rate (9.95Gb/s) in the description, since it may not be obvious.

7) Looking at how they are used, it might be better for otn-label-range-type to be represented as a enum.

8) In grouping otn-label-range-info:
 - I suggest making range type an enum rather than identity.
 - Perhaps add a when statement to the "tsg" leaf, so that it can only be populated when the enum is a "trib-slot"?  Or otherwise the description is unclear.
 - I wasn't sure how familiar "tsg" is, and perhaps it would be better in its expanded form.
 - Please add more description to Priority.

9) In grouping otn-label-start-end:
 - Probably expand the "tpn" and "ts" names?
 - Probably define typedefs for "tpn" and "ts" uint16's, since they are used in multiple grouping definitions.
 
10) In grouping otn-label-hop:
 - Expand "tpn" name and use the typedef (as per previous comment).
 - Expand "tsg" name
 - Expand the ts-list name, and perhaps enhance the description to state that values must be in non-overlapping ascending ordering.   Or you could borrow some text for section 9.2.4 of RFC 7950.
 
11) In grouping "otn-label-step" the "default" statements don't really have any effect.  I think that either you need to add a default statement to the choice, or you should delete the default statements under the tpn and ts leaves.

12) identity coding-func.
- All of these identities should be before all of the groupings.
- They also need to have their indentation fixed (e.g., "pyang -f yang --yang-line-length 69")
- Are the references to MEF 63 the right choice, or should these references be to IEEE Std 802.3?

13) identity optical-interface-func
- Are the references to MEF 63 the right choice, or should these references be to IEEE Std 802.3?
- "base identity" => "Base identity"
- Do you definitely want the "clause-XX" as part of each identity name?  Is it possible that these clauses could ever change or be renumbered?

14) identity service-performance-metric:
- Rather than "list of service-specific ..." perhaps "Base identity for service specific ..."
- Perhaps change "One-way-..." to "One-Way-...", it looks strange to have a single character not capitialized.
- Probably remove the hypens from the descriptions?

15) identity network-performance-metric
- Should any identities be defined here?
- Rather than "list of network-specific ..." perhaps "Base identity for network specific ..."