[RTG-DIR]Re: [CCAMP]draft-ietf-ccamp-dwdm-if-param-yang-13 ietf last call Rtgdir review

[Roberto Manzotti <[e-mail]>]

Hello Dhruv,

first of all thanks for your detailed review and sorry for taking long 
to reply.

We have uploaded since the review two new revision of the document:
- 14: was addressing most of the Opsdir and YANG Doctor review comment, 
and some of your comment that was in common
- 15: just uploaded to datatracker is addressing almost all of your 
comments with two exception that we are tracking with github issue for 
the resolution in the next revision.

Please find in-line below marked with "authors>" how we addressed your 
comments.

Thanks
Regards
Roberto on behalf of co-authors


On 8/11/25 11:08, Dhruv Dhody via Datatracker wrote:
> Document: draft-ietf-ccamp-dwdm-if-param-yang
> Title: A YANG data model to manage configurable DWDM optical interfaces
> Reviewer: Dhruv Dhody
> Review result: Has Issues
>
> # RTGDIR review of draft-ietf-ccamp-dwdm-if-param-yang
>
> Hello,
>
> I have been selected as the Routing Directorate reviewer for this draft. The
> Routing Directorate seeks to review all routing or routing-related drafts as
> they pass through the IETF last call and IESG review, and sometimes on special
> request. The purpose of the review is to assist the Routing ADs. For more
> information about the Routing Directorate, please see
> https://wiki.ietf.org/en/group/rtg/RtgDir
>
> Although these comments are primarily for the use of the Routing ADs, it would
> be helpful if you could consider them along with any other IETF Last Call
> comments that you receive, and strive to resolve them through discussion or by
> updating the draft.
>
> Document: draft-ietf-ccamp-dwdm-if-param-yang
> Reviewer: Dhruv Dhody
> Review Date: 2025-08-11
> IETF LC End Date: Unknown
> Intended Status: Proposed Standard
>
> ## Summary:
>
> * I have some minor concerns about this document that I think should be
> resolved before publication.
>
> ## Minor
>
> - Suggest adding JSON examples in the document.
authors> agree to add some Json example in the appendix, still pending 
and tracked with github issue:
https://github.com/ietf-ccamp-wg/draft-ietf-ccamp-dwdm-if-param-yang/issues/2 

>
> - Incomplete section 3.3, as indicated in the editor note
authors> We received the same comment also by OPSDIR, Section 3.3 has 
been fixed since rev. 14 and simplified by pointing
the reader to the parameters description in the YANG model.
>
> - While the introduction has some description of key fields, it would be much
> better to include a separate section that explains the key design elements in
> the YANG model with YANG tree snippets.
authors> this is still pending and tracked by github issue:
https://github.com/ietf-ccamp-wg/draft-ietf-ccamp-dwdm-if-param-yang/issues/25 

>
> - Section 5, please describe the sensitivities that are specific to
> current-wdm-if-parameters (and not just state that they exist).
authors> actually after better reading the rfc8407bis we think that 
there are no sensitive writable data in the meaning described in 
rfc8407bis, so we replaced the text with
"There are no particularly sensitive writable data nodes."
>
> ## YANG Model
>
> - To fix the YANG formatting, please run the command "pyang --ietf --lint -f
> yang --keep-comments --yang-line-length 69"
> authors> fromatting fixed in rev. 15 using the suggested pyang command 
> and attributes 
>
> - Update the description in the revision statement to "Initial revision."
authors> Fixed in rev. 15
>
> - There are some comments (such as "//    uses wdm-if-fec-tca-thresholds;" and
> comment before wdm-if-tca-types) that should be removed.
authors> already fixed as part of OPSDIR review in rev. 14
>
> - Update the description for "rx-power-tca" as it uses tx in the description.
authors> already fixed as part of OPSDIR review in rev. 14
>
> - tca-name: In the description, it gives an example 'Low TX Power', is this
> just a human-readable string for more information, or does it have any other
> significance?
authors> yes it is a human readable description of the scope of the 
specific TCA;
in rev. 15 we extended the description to better clarify and provided a 
more meaningful example
>
> - raise-threshold and clear-threshold are of type int32, is that suitable for
> all wdm-if-tca-types? I see the use of decimal for some thresholds in
> RFC9093bis. Should there be any checks for the values for the raise and clear
> threshold? For instance, what happens if raise==clear? What if only
> clear-threshold is set without raise-threshold? Also, I would have liked some
> examples where these things could be explained better.
authors> the type has been already changed to l0-types:decimal-5
in rev, 14.  In rev. 15 we also improved the definition of 
raise/clear-threshold:
- both raise and clear are now mandatory
- added conditions to avoid clear==raise
- expanded both description to better clarify the behavior
- Added a TCA example in the appendix
>
> - I also suggest writing a better description statement for raise-threshold and
> clear-thresold. The current clear-threshold should also include clear
> description on how to handle when raise-threshold > clear-threshold etc.
authors> as indicated also in the previous bullet, we have improved the 
description of raise and clear threshold
>
> - q-margin, cur-q-factor: is int32 correct type? Should this be decimal64?
authors> the type of this two leaf has been changed to 
l0-types:decimal-2-or-unknown since rev.14
>
> - explicit-transceiver-mode-id: Add a length statement to bound the string.
authors> adding "length 64" as part of rev. 15 update
>
> - In configured-mode, it is 'rw' and it is referring to mode-id, which is 'ro';
> this is against the rules of RFC 7950. I am sure this would show up in
> yanglint. Please make sure you have run all validation tools. ````
>             leaf configured-mode {
>               type union {
>                 type empty;
>                 type leafref {
>                   path "../../supported-modes/supported-mode/mode-id";
>                 }
>               }
>
>      augment /if:interfaces/if:interface:
>        +--rw wdm-interface
>           +--ro supported-modes!
>           |  +--ro supported-mode* [mode-id]
>           |     +--ro mode-id                           string
> ````
authors> was tracked also from YANG doctor review, it has been fixed in 
rev.15 by following YANG doctor suggestion to use a weak reference by 
adding "require-instance false;" to the leafref. In fact the issue was 
showing up in yanglint, while now yanglint check is passing w/o errors.
>
> ## Nits
>
> - Marking all five authors as editors is a bit unusual. It is useful to mark
> the person who is holding the pen as editor or none at all.
authors> Fixed in rev. 15 by expanding the first usage in both the 
Abstract and the Introduction
>
> - DWDM does not have a * in the RFC Editor abbreviation list and thus needs to
> be expanded on first use.
authors> Fixed in rev. 15 by expanding the first usage in both the 
Abstract and the Introduction
>
> - s/Yang/YANG/g
authors> same comment received also from OPSDIR, already fixed in rev. 14
>
> - Remove references from Abstract.
authors> same comment received also from OPSDIR, already fixed in rev. 14
>
> - Expand on first use: FEC, QPSK, OSNR, QAM16, TCA, BER
authors> some already fixed in rev. 14, the remaining are fixed in rev. 
15 update
>
> - There are two copyright notices; please remove the one following the
> abstract. Update the year in the copyright of the YANG model.
authors> Removing the first note and fixed as part of rev. 15 update.
>
> - Avoid using the word draft (and memo, and RFC) when referring to the I-D,
> suggest using 'document' so that it makes sense even after publication as RFC.
authors> fixed all instance of "draft" and "memo" usage and changed to 
"document" as part of rev. 15 update.
>
> - Add reference for G.698.2
authors> sorry we have not found where the reference is missing, since 
the reference to G.698.2 is present already and was there also in rev, 13.
Could you share any specific instance in the document text where is 
missing?
>
> - s/as a mean to/as a means to/
authors> same comment received also from OPSDIR, already fixed in rev. 14
>
> - s/is the way and OpenROADM compliant/is the way, an OpenROADM compliant/
authors> same comment received also from OPSDIR, already fixed in rev. 14
>
> Thanks!
> Dhruv
>
>
> _______________________________________________
> CCAMP mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]