Re: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-layer0-types-03

[Zhenghaomian <[e-mail]>]

Hi, Robert, 

Again, thank you very much for your detailed review and comments. The authors had a few iterations of discussions and give the update for this document as follow. 

Model update can be found as the pull request: https://github.com/ietf-ccamp-wg/draft-ietf-ccamp-layer0-types/pull/3 ; you (and other experts) are welcome to follow up in this thread. 
An updated draft have been locally created, with the diff files attached; 
For detailed comments response/discussion, please see inline starting with '[Authors]';

Please help review if the comments are addressed, any discussion/calls are welcome and can be arranged by chairs, thank you.  

Best wishes,
Haomian (on behalf of all authors & contributors)

-----邮件原件-----
发件人: Robert Wilton via Datatracker [mailto:[e-mail]] 
发送时间: 2019年12月14日 0:50
收件人: [e-mail]
抄送: [e-mail]; [e-mail]; [e-mail]
主题: Yangdoctors last call review of draft-ietf-ccamp-layer0-types-03

Reviewer: Robert Wilton
Review result: Almost Ready

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.
[Authors] done in -04.

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]."
[Authors] Discussion needed: we need to be careful on changing this, as currently the layer0-types and layer1-types are in consistent format. Need to change in both sides or don't change any of them.

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.
[Authors] done in -04.

4) typo in the Security Considerations "layer0 => layer 0", and also in the title of section 3.
[Authors] done in -04.

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.
[Authors] done in -04.

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).
[Authors] done in -04.

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.
[Authors] done in -04.

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.
[Authors] We have checked all the typedef, and agreed on removing frequency-thz and frequency-ghz together, as they are not referenced in later groupings. Need to further check whether the modules who import this type module would use such typedef. May need to add back if the answer is yes. Moreover, like flexi-n, another typedef flexi-m is added and referenced in this module.

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 ...
[Authors] Discussion needed: currently the channel spacing is 6.25GHz, and we reserve the 3.125GHz for proof-of-future.  According to Singapore discussion, WG agreed on 3.125 with 3 fraction digits. We prefer to keep the current format given the following considerations: 
[Authors] Consideration 1: Technically it won't make much sense for Layer 0 to come down to 1.5625G, as layer 1 is providing the granularity at ODUk (<100Gbps) which is make very good use of such spectrums. 
[Authors] Consideration 2: furthermore, if 1.5625G happens in the future, can we keep fraction digits as 3 and specify the channel spacing as '1.562G'? There should not be difficulties in understanding, and we solve the problem forever.
[Authors] Consideration 3: the channel spacing is not binded with the frequency on the grid, but a number... So maybe we can change the dependency on the grid as well.

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.
[Authors] done in -04.

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.

[Authors] Discussion needed: we hope this proposal is different with the ones on 'identity->enumeration' but maybe push back by extensibility, could you please confirm? Gbps is used for data plane and should be accurate enough to be described in the YANG module. We are open to the proposal, but need to understand the difference and how it affects the configuration. After that we can discuss whether decimal 64 is good, Gbps or Mbps, etc...

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";
}
[Authors] Discussion needed: probably reject, see the consideration about extensibility on layer1-types. 6.25/3.125 GHz is coming...

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.
[Authors] Discussion needed: probably reject, see the consideration about extensibility on layer1-types.

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.
[Authors] Confirmation firstly: probably a mis-spelling, propose to 'l0-label-range-info'.
[Authors] Discussion secondly: there are multiple identities/typedef started with 'layer0-' and it should be better to keep consistency in naming format, do we rename all of them or none of them?
[Authors] priority updated in -04.

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

Typos: "girds" => "grids", "attrtibutes" => "attributes"
[Authors] done in -04,

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.
[Authors] done in -04, probably some compilation problems and will double check per update.