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

Request Review of draft-ietf-ccamp-layer0-types-02
Requested rev. 02 (document currently at 08)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2019-12-04
Requested 2019-11-12
Requested by Daniele Ceccarelli
Authors Haomian Zheng, Young Lee, Aihua Guo, Victor Lopez, Daniel King
Draft last updated 2019-12-13
Completed reviews Yangdoctors Last Call review of -01 by Robert Wilton (diff)
Yangdoctors Last Call review of -03 by Robert Wilton (diff)
Rtgdir Last Call review of -06 by Emmanuel Baccelli (diff)
Genart Last Call review of -07 by Theresa Enghardt (diff)
Comments
This draft is connected with https://datatracker.ietf.org/doc/draft-ietf-ccamp-layer1-types/. Would it be possible to have them reviewed together?
Assignment Reviewer Robert Wilton 
State Completed
Review review-ietf-ccamp-layer0-types-03-yangdoctors-lc-wilton-2019-12-13
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/4vzKJFJKBJQX7Gcb-Gz2NWLV-SI
Reviewed rev. 03 (document currently at 08)
Review result Almost Ready
Review completed: 2019-12-13

Review
review-ietf-ccamp-layer0-types-03-yangdoctors-lc-wilton-2019-12-13

Comments on the document:

1) I would delete the "overview" paragraph at the top of section 2, and just promote section 2.1 as section 2.


2) 2.1. Layer 0 Types Module Contents:

The descriptions are good, but I would suggest formatting these as a table, or more tightly link the definition to it's description.

E.g. 

"Operational-mode: A type that represents operational mode as defined in [ITU-Tg6982]."

Instead of:


"Operational-mode:

A type that represents operational mode as defined in [ITU-Tg6982]."


3) I would define the module as YANG version "1.1" (because the language behaviour is generally better specified) and then reference only RFC 7950 in the introduction.

4) typo in the Security Considerations "layer0 => layer 0", and also in the title of section 3.

5) I have suggested changing the module prefix to "l0-types" rather than layer2-types.  If you make this change then the IANA considerations would need to be updated, along with section 1.2.


Comments on the YANG module:

1) I would suggest changing the YANG prefix to "l0-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) It is actually necessary to define frequency-thz at all?  I think that the discussion in the WG suggested that it might be better if the frequencies are always defined in Ghz and then converted in the client as necessary.

4) frequency-ghz: Is 3 fraction digits sufficient for future expansion?  E.g. It would seem to support a flex grid 3.125Ghz channel spacing, but not 1.5625 ...

5) We should aim for consistency of the identity names in the layer-1 types module.  E.g. perhaps OTU, ODU and OPU should be capitalized.

6) The models uses identities for bandwidth, but I wonder whether defining a numerical typedef might be a better choice (e.g. more efficient and perhaps easier for programs to work with).  Here, I have constrained the values that are allowed, but they could also be unconstrained:

E.g. 
typedef channel-bandwidth {
  type decimal64 {
    fraction-digits 2;
    range
      "2.66|10.70|11.04|11.09|11.27|11.31|43.01|44.57|44.58|100.00 ... max"
    description
      "Bandwidth carried by a single wavelength channel"
  }
  units "Gb/s";
}

Or another alternative would be to use Mb/s, which would then allow them to be specified as a union of specific values and an arbitrary bandwidth value.


7) Same comment as for bandwidth applies to the channel spacing identities.  I.e. I wonder whether these wouldn't be better defined using a decimal64 type.

E.g. 
typedef dwdm-channel-spacing {
  type decimal64 {
    fraction-digits 2;
    range
      "12.5|25|50|100"
    description
      "Bandwidth carried by a single wavelength channel"
  }
  units "Ghz";
}

8) Same comment as for bandwidth and dwdm-channel-spacing could also be applied to flexi-grid-channel-spacing, flexi-slot-width-granularity, cwdm-channel-spacing.

9) In grouping layer0-label-range-info
I would rename this grouping to l0-layer-range-info, change "layer0" => "layer 0" in the description.  Also "priority" could do with a more detailed description as to what it means, etc.

10) In grouping flexi-grid-label-start-end, I think that the type should be "flexi-n" rather than "int16".

Typos: "girds" => "grids", "attrtibutes" => "attributes"

Spacing/indenting needs to be fixed:
 - In "grouping wson-label-hop", just before case cwdm
 - In "grouping flexi-grip-label-hop", should have a blank line before, and "case super" block/fields indentation doesn't look right.
 - In some of the typedef definitions, the "{" should move from the start of the following line to the typedef line.
 In general, as a starting point, after all other markups have been made then it might be a good idea to use pyang to format the YANG file for you, e.g., "pyang -f yang --yang-line-length 69", but probably with some more blank lines, otherwise it is a bit dense.