Last Call Review of draft-ietf-ccamp-layer0-types-01
review-ietf-ccamp-layer0-types-01-yangdoctors-lc-wilton-2019-07-25-00

Request Review of draft-ietf-ccamp-layer0-types-01
Requested rev. 01 (document currently at 02)
Type Last Call Review
Team YANG Doctors (yangdoctors)
Deadline 2019-07-05
Requested 2019-06-19
Requested by Daniele Ceccarelli
Authors Haomian Zheng, Young Lee, Aihua Guo, Victor Lopezalvarez, Daniel King
Draft last updated 2019-07-25
Completed reviews Yangdoctors Last Call review of -01 by Robert Wilton (diff)
Assignment Reviewer Robert Wilton
State Completed
Review review-ietf-ccamp-layer0-types-01-yangdoctors-lc-wilton-2019-07-25
Posted at https://mailarchive.ietf.org/arch/msg/yang-doctors/BJt3vl-jNMGeHqVyEq5yJDKVw7c
Reviewed rev. 01 (document currently at 02)
Review result Ready with Nits
Review completed: 2019-07-25

Review
review-ietf-ccamp-layer0-types-01-yangdoctors-lc-wilton-2019-07-25

I have reviewed this document as part of the YANG doctors directorate's 
ongoing effort to review all IETF documents being processed by the IESG.  These 
comments were written with the intent of improving the operational aspects of the 
IETF drafts. Comments that are not addressed in last call may be included in AD reviews 
during the IESG review.  Document editors and WG chairs should treat these comments 
just like any other last call comments. 

Nits:

1) Doc title: Perhaps change to "A YANG Data Model for TE Layer 0 Types", also changing the short name from "Layer0 Types" to "TE Layer 0 Types".

2) Abstract:
 "in YANG" => "in the YANG"
 
 Add the comment to the abstract:
 
   The YANG data model in this document conforms to the Network
   Management Datastore Architecture defined in RFC 8342.
 
3) Section 1.1 is probably not required, or at least the document doesn't appear to use RFC2119 language.

4) Section 1.3. Prefix and Module naming:
The module description states that the types are for TE.
Hence, would a module name of "ietf-te-layer0-types", with a module prefix of "te-l0-types" be better?

5) Section 3.1 A lot of the types have "flexi-grid" in the type name by "flex-grid" in the description.  Perhaps it would be appropriate to change "flex-grid" to "flexi-grid" or "flexible-grid"?

YANG Module:

(1) The main comment is that you should define typedefs in a couple of places:

(1-i) For the dwdm-n frequency definition (used by leafs dwdn-n in groupings wson-link-label and wson-path-label, and leaf flexi-n in grouping flexi-grid-link-label):

The description for this typedef (which can be removed from the description in the leaves) should probably be:
 "The given value 'N' is used to determine the nominal
  central frequency.
 
  The nominal central frequency, 'f' is defined by,
    f = 193.1 THz + N x 0.00625 THz,
  where 193.1 THz is the ITU-T 'anchor frequency' for
  transmission over the C band";


(1-ii) For the cwdn-n frequency definition (used by leafs cwdm-n in groupings wsol-link-label and wson-path-label):

The description for this typedef (which can be removed from the description in the leaves) should probably be:
 "The given value 'N' is used to compute the channel
  wavelength as per the formula:
    Wavelength (nm) = 1471 + N x 20";

The typedef's reference statement should be "ITU-T G.694.2" (perhaps also with the date it is published as MM/YYYY)

(1-iii) It might also be worth defining a typedef for the subcarrier-dwdn-n leaf, even though the type is only used is one place.


Minor comments:

(2) I note that your sample values in frequency-thz are not in the canonical value (e.g. the canonical value would be "193.125" rather than "193.12500").  I think that this is OK, but wanted to point it out.

(3) Your fequency-Ghz is to 5 fractional digits, e.g. allowing "193125.12345" Ghz.  I.e. this is 1000 times more precise than the Thz value.  Is this intentional?  If you keep it as it is, then I would consider change the example value to "193125.0".

(4) "flexi Grid node" => "Flexi-grid node"

(5) "identity random-wavelength-assignment" => Check line length of description, probably could be expanded.

(6) "Layer0 grid type." => "Layer 0 grid type"

(7) "Termination type." => "Termination type"

(8) In several places "Termination" at end of description => "termination"

(9) "identity term-phys" => description to "Physical layer termination", indent all description and reference strings to 2 spaces after the begining of the word 'description'/'reference' word.  Also need to fix in "identity fec-type" reference statement, 

(10) "Section Layer Termination" => "Section layer termination"

(11) Some of the reference statements should be expanded from just the RFC number to also the RFC name.

(12) The description of subcarrier-dwdn-n leaf-list should probably be something like:

description
  "List of subcarrier channels for the super channel.

   The given value 'N' is used to determine the nominal
   central frequency.
 
   The nominal central frequency, 'f', is defined by,
    f = 193.1 THz + N x 'channel spacing (THz)',
  where 193.1 THz is the ITU-T 'anchor frequency' for
  transmission over the C band";  
 
reference "ITU-T G.694.1";

(13) leaf "priority" description "priority" => "Priority"

(14) grouping flexi-grid-channel description, "where M is an integer greater or equal to 1" => "where M is a positive integer"

(15) "onsidered" => "considered"

(16) Remove extra space in hte URI in the IANA considerations.

Thanks,
Rob