Hi Italo and Sergio,the YANG module now looks much better. You were right to discuss some changes on the mailing list but my suggestions should generally align with the YANG module guidelines. I have found a few typos (extra spaces):
" Deviation from the reference topology "; (multiple) By incorrectly formatted descriptions I meant for example: "pointer to the list set of ROADM optical impairments"; They should all ideally be valid English sentences. But except for these few minor nits, it looks okay now. Regards, Michal On 7. 7. 2025 16:06, Sergio Belotti (Nokia) wrote:
Hello Michal, Thanks for your comments. Based on your input we have uploaded a new https://datatracker.ietf.org/doc/draft-ietf-ccamp-optical-impairment-topology-yang/ : https://datatracker.ietf.org/doc/draft-ietf-ccamp-optical-impairment-topology-yang/ . Please see in line some comments as feedback to your review . Thanks Italo and Sergio-----Original Message----- From: Michal Vaško via Datatracker <noreply@xxxxxxxx> Sent: Thursday, April 24, 2025 2:24 PM To: yang-doctors@xxxxxxxx Cc: ccamp@xxxxxxxx; draft-ietf-ccamp-optical-impairment-topology- yang.all@xxxxxxxx; last-call@xxxxxxxx Subject: [CCAMP]Yangdoctors ietf last call review of draft-ietf-ccamp-optical- impairment-topology-yang-18 Document: draft-ietf-ccamp-optical-impairment-topology-yang Title: A YANG Data Model for Optical Impairment-aware Topology Reviewer: Michal Vaško Review result: On the Right Track This review follows my previous review of an older revision of this draft. Many of the issues I mentioned before were not addressed and so are repeated. - all top-level configurable presence containers in the augments: A rather odd way of configuring whether some operational information should be reported or not but I am not an expert in this area at all. Consider using individual features to express device capabilities or a lack of. Otherwise the user can always filter what data to read. Also, non-presence containers may simply not be populated if the device chooses not to do so for some reason but it should all be documented in its description.authors> It seems to us that the usage of feature it is a schema level not "per instance" that means the server should report if the feature is or not supported. In our case the container can be present or not depending "per instance".- leaves "roadm-path-impairments", "add-path-impairments", "drop-path-impairments": They are leaves referencing single list instances so a singular form should be used.authors> OK, but it is a single list instance but containing multiple impairment parameters (e.g. cd, pmd, pdl, etc.). We have updated the model based on some suggestions from discussion with NETMOD WG (with CCAMP WG in cc) https://mailarchive.ietf.org/arch/msg/netmod/ySY-i_Bqj-1qV4GKztP4cc36Eoc/ Also, the last 2 are used repeatedly 4 times ingroupings used in schema level of different depth so the relative leafrefs are different. Consider using absolute paths allowing both leaves to be put into a grouping and be reused.authors> The absolute path should anyway indicate network-id and node-id as keys for the network and node list. So you would have again relative path inside. see the following thread in the YANG doctor mailing list https://mailarchive.ietf.org/arch/msg/yang-doctors/8EHiqGRcAxJkv2OtSLcU9a7i848/- leaf "frequency-range-id", container "frequency-range" Nodes with duplicate definition in 3 cases of a choice, should be put into a grouping instead.Authors> OK, we have followed your suggestion and created "grouping frequency-range-with-identifier"- list "OMS-element": The description of the list and its leaf seem to be mixed up and should be improved. Also, the description mentions "ordered list" but the list is not user ordered, add the YANG statement.Authors> We have improved the description and eliminated "ordered" word from the description. Hope now everything is more clear.- leaf "is-allowed": Use just "allowed" and the description is redundantly specific explaining that 'true' value allows the functionality and 'false' turns it off, shorten.authors> We used is-allowed for consistency with RFC8795 which we are augmenting. About the description, I do not think it is redundant since just explain what functionality (connectivity from a specific transceiver) is or not allowed.- redundantly long identifiers repeating information from parent nodes: otsi-carrier-id -> carrier-id otsi-carrier-frequency -> carrier-frequency otsi-group-id -> id oms-element- uid -> uid roadm-path-impairments-id -> id explicit-transceiver-mode-id -> id transponder-id -> id transceiver-id -> id media-channel-id -> idauthors> Regarding OTSI-related identifiers, we are ok with your suggestion. For other identifiers, we think that using short identifiers like "id" may pose risks if data is exploited by humans during application development.- identifiers with capital letters: OMS-elements -> oms-elements OMS-element -> oms-elementAuthors> OK, we have also changed the OMS-attributes for consistency- concentratedloss: Identifier and string used throughout the module, unless an official term, should probably be concentrated loss or concentrated-loss.Authors> Ok, put concentrated-loss instead of concentratedloss- major text issues: Lots of descriptions are short and not formatted as sentences, need to be improved.Authors> It would be beneficial to have from you hints about what are the descriptions that are not clear enough for you. Take into account this draft is not a tutorial for Optical Impairments assuming the reader can have a certain level of familiarity with major of the used terms. Anyway based on your indications we will try to improve description.- minor text typos (full lines copied): "It allows defining for each spectrum badwidth the amplification stage of the optical amplifier."; " Deviation from the reference carrier powerAuthors> yes , fixed- other formatting issues can be solved automatically by printing the YANG module with yanglint or pyang.Authors> yes, done. We run "pyang -f yang"- JSON data examples in the appendices: The examples are not valid, use yanglint to validate all the examples and improve them until it reports no errors or warnings.Authors> We have aligned JSON examples with updated YANG data model and validated them with yanglint.
<<attachment: smime.p7s>>
-- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
