[yang-doctors] 答复: Yangdoctors last call review of draft-ietf-ccamp-layer1-types-04

[Zhenghaomian <[e-mail]>]

Hi, Robert, 

Thank you very much for your detailed review and comments. The authors had a few iterations of discussions and give the update as follow. 

Model update can be found as the pull request: https://github.com/haomianzheng/IETF-ACTN-YANG-Model/pull/52; 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 with '[Authors]';

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

Wish you a Merry Christmas and Happy new year. 

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

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

Reviewer: Robert Wilton
Review result: Almost Ready

Comments on the document:

1) Please check the English grammar for the earlier paragraphs.
[Authors] fixed some editorial problems, and will do again by every update; 

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

3) Security section, "onsidered" => "considered"
[Authors] done in -05.

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).
[Authors] done in -05. The only problem is to recognize it correctly as 'l1' instead of '11' or 'll', which has been raised before for 'l1csm' in ccamp.

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 -05.

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?
[Authors] The unit should be Gbps in layer 1 for rate; in the other document (layer0-type) the unit GHz is used for spectrum width. 
[Authors] There are many similar comments (identity->enum) in these documents. The authors decided to reject some of the proposals because of 1) enumeration is not possible to extend, but there can be new values in the future; 2) Specifically for this comment, the configuration of tsg is a type rather than a number, so decimal is not a good representation.

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";
 }
[Authors] done in -05. 
[Authors] Discussion need: We need to check with YANG doctor, on whether make RFC7963/G.sup43 as informational or normative. Prefer to be informational, but how should we specify in the model and in the reference section?


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)
[Authors] done in -05, for ODU-types and 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.
[Authors] done for ETH-10Gb-WAN/ETH-10Gb-LAN.

7) Looking at how they are used, it might be better for otn-label-range-type to be represented as a enum.
[Authors] We propose not to change, as enum is not good for extension.

8) In grouping otn-label-range-info:
 - I suggest making range type an enum rather than identity.
[Authors] We propose not to change, as enum is not good for extension.
 - 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.
[Authors] Discussion need: tpn-range/ts-range can both be different for different TSG values, refer to section 4.3 in this document to see detailed usage guidance.

9) In grouping otn-label-start-end:
 - Probably expand the "tpn" and "ts" names?
[Authors] as they are well-known term in OTN literature. Need to understand the principle. As the terms are OTN-specific, it is proposed to change to 'otn-tpn' and 'otn-ts' respectively. We also expand in the description.
 - Probably define typedefs for "tpn" and "ts" uint16's, since they are used in  multiple grouping definitions.
[Authors] done in -05.

10) In grouping otn-label-hop:
 - Expand "tpn" name and use the typedef (as per previous comment).
 - Expand "tsg" name
[Authors] changed accordingly to comments 9.
 - 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.
[Authors] Following proposal:
OLD:
        description
          "A list of available tributary slots ranging
           between 1 and 4095.
           For example 1-20,25,50-1000";
NEW: 
        description
          "A list of available tributary slots ranging
           between 1 and 4095. If multiple values or  
           ranges are given, they all MUST be disjoint 
           and MUST be in ascending order.
           For example 1-20,25,50-1000";


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.
[Authors] Discussion needed: the motivation is to set a default value of tpn/ts in each branch to 1 in the otn-label-step grouping, with the choice structure. Deleting the default statement would lose our restriction. We don't understand the comments on the option 'add a default statement to the choice'. Can you give more guidance? Moreover, if we put a default value under choice, we are not sure where this default value goes. 
[Authors] Discussion needed: we find the choice on 'tpn' or 'ts' is not determined in this grouping, instead it is specified in 'otn-label-range-info'. Help requested for modeling this representation.

12) identity coding-func.
- All of these identities should be before all of the groupings.
[Authors] done in -05, the indentation looks good so far.
- 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?
[Authors] Discussion needed: Need to check the reference rule: the MEF63 is the target we want to align, but IEEE 802.3 is the original one to track. MEF63 is referencing to IEEE 802.3 with corresponding clause, and it is propose not to repeat.

13) identity optical-interface-func
- Are the references to MEF 63 the right choice, or should these references be to IEEE Std 802.3? 
[Authors] Discussion needed: Need to check the reference rule: the MEF63 is the target we want to align, but IEEE 802.3 is the original one to track. MEF63 is referencing to IEEE 802.3 with corresponding clause, and it is propose not to repeat.
- "base identity" => "Base identity" 
[Authors] done in -05.
- 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?
[Authors] We adjusted as, "ETH-1000X-PCS-36" -> "ETH-1000X", "SX-PMD-clause-38"-> "SX-PMD-1000" (need to specify the rate to avoid duplication), refer to the updated model.


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?
[Authors] done in -05, as most of the parameters in YANG is lower case, so we changed everything to lower case.

15) identity network-performance-metric
- Should any identities be defined here?
- Rather than "list of network-specific ..." perhaps "Base identity for network specific ..."
[Authors] This identity is removed as no identities defined here, according to MEF63.