Hi Haomian, Sorry, I had missed your email over Christmas. Replying back to the original email to capture the other aliases. Please see inline ... > -----Original Message----- > From: last-call <last-call-bounces@xxxxxxxx> On Behalf Of Zhenghaomian > Sent: 25 December 2019 08:20 > To: Rob Wilton (rwilton) <rwilton@xxxxxxxxx>; yang-doctors@xxxxxxxx > Cc: last-call@xxxxxxxx; draft-ietf-ccamp-layer0-types.all@xxxxxxxx; > ccamp@xxxxxxxx > Subject: Re: [Last-Call] Yangdoctors last call review of draft-ietf-ccamp- > layer0-types-03 > > 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:noreply@xxxxxxxx] > 发送时间: 2019年12月14日 0:50 > 收件人: yang-doctors@xxxxxxxx > 抄送: last-call@xxxxxxxx; draft-ietf-ccamp-layer0-types.all@xxxxxxxx; > ccamp@xxxxxxxx > 主题: 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. [RW] I agree that they should be consistent between the two drafts. My aim was to make them as readable as possible, but this is purely a stylistic thing. > > 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. [RW] I think that defining frequency-ghz is probably still helpful, it was only frequency-thz that I was suggesting removing, effectively encouraging everyone to standardize on using frequency-ghz for layer 0 properties/considerations. > > 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. [RW] Okay, keeping this to 3 dpi is fine. > > 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... > [RW] Identities are just names of things. So, it if the bandwidth value that clients and implementations care about, then I would expect them to need to write code to translate between the identity names and numerical values. E.g. what does a client configuring the device primarily care about here. Is it that the bandwidth is "otu2" or that it the bandwidth is "10.70G"? > 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... [RW] The model can always be extended to support 6.25/3.125 in future. In either solution this would require a backwards compatible change to the model. My hunch is that having this as a numerical value is more useful for clients than having it as a named identity. I.e. I think that the argument for using a numerical value here is stronger than for channel bandwidth above. > > 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. [RW] These should be resolved with whatever the conclusion is above. > > 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'. [RW] Yes. > [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? [RW] Yes, I think that I would rename all of them "l0-" Thanks, Rob > [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. -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
