Early Review of draft-ietf-pim-igmp-mld-snooping-yang-05
review-ietf-pim-igmp-mld-snooping-yang-03-yangdoctors-early-rahman-2018-06-28-01
Request | Review of | draft-ietf-pim-igmp-mld-snooping-yang |
---|---|---|
Requested revision | No specific revision (document currently at 20) | |
Type | Early Review | |
Team | YANG Doctors (yangdoctors) | |
Deadline | 2018-07-03 | |
Requested | 2018-06-11 | |
Requested by | Stig Venaas | |
Authors | Hongji Zhao , Xufeng Liu , Yisong Liu , Mahesh Sivakumar , Anish Peter | |
I-D last updated | 2018-10-17 | |
Completed reviews |
Yangdoctors Early review of -05
by Reshad Rahman
(diff)
Yangdoctors Last Call review of -12 by Reshad Rahman (diff) Rtgdir Last Call review of -13 by Himanshu C. Shah (diff) Secdir Last Call review of -15 by Stephen Farrell (diff) Genart Last Call review of -12 by Brian E. Carpenter (diff) Yangdoctors Last Call review of -19 by Reshad Rahman (diff) |
|
Comments |
Hi We plan to start a pim WGLC on this draft but would like to have an early YANG doctor review first. Thanks, Stig |
|
Assignment | Reviewer | Reshad Rahman |
State | Completed | |
Request | Early review on draft-ietf-pim-igmp-mld-snooping-yang by YANG Doctors Assigned | |
Reviewed revision | 05 (document currently at 20) | |
Result | Ready w/issues | |
Completed | 2018-10-17 |
review-ietf-pim-igmp-mld-snooping-yang-03-yangdoctors-early-rahman-2018-06-28-01
YANG Doctor review of draft-ietf-pim-igmp-mld-snooping-yang-05 (by Reshad Rahman) This is my YD review of -05, in June 2018 I had done an early review of -03. 1 module defined in this draft: - ietf-igmp-mld-snooping@2018-10-11.yang Errors are shown at https://datatracker.ietf.org/doc/draft-ietf-pim-igmp-mld-snooping-yang/ but that seems to be tools related (yang modules can’t be found) All major issues raised in previous review have been addressed. Main issues/questions: - igmp-snooping-instances (mld- also) are top level containers, I believe they should be under rt:control-plane-protocol (RFC8349). I should have raised this in the previous review. - Should there be a dependency on draft-ietf-pim-igmp-mld-yang, e.g. should we allow IGMP/MLD snooping configuration only if IGMP/MLD is enabled (leaf “enable”) or supported (feature-mld and feature-igmp)? E.g. I don’t think it makes sense to configure igmp-snooping if igmp is not supported. Minor comments, suggestions and nits: - Section 2, I don’t see the point of last sentence “This document provides freedom….”. - Section 2.1 has a reference to draft-dsdt-nmda-guidelines which has expired - YANG indentation is off, please correct - Comment “replace with IANA namespace” applies to what line? - In YANG module, s/Refrence/Reference/ (1 instance) - Leaf “type” in snooping-instances, why not have an identity for type (l2vpn and bridge only for now). Right now there’s an enum which is defined twice (for mld and igmp) - Similar comment as above for leaf “host-filter-mode” - Send-query: s/topo/topology/, s/param/parameter/ - For all feature definitions, add references (RFC + relevant sections) - Leaf exclude-lite, in description should there an explanation or reference of what exclude lite means? - Grouping statistics-sent-received: 1. remove -sent-received from name 2. Description of counters, some have messages and some don’t 3. For all counters for messages, add a reference for the messages 4. Counter names, e.g. “pim” is too short, it should be something along the lines of num-pim-messages or num-pim. 5. In pim description s/pim/PIM/ - RPCs clear-xxx-snooping-groups, rename to clear-xxx-snooping-cache (as per description)? - Security Considerations, you should also mention the xxx-snooping-instance leaf nodes under vlan and l2vpn - Examples (Appendix A) 1. Indentation is off, please correct 2. Were the examples validated with a tool? You can use yanglib 3. bridge-mrouter-interface should be “eth1/1” instead of “1/1”. Likewise for interface, bridge-outgoing-interface 4. The example is for bridge, there should be an example for l2vpn also - Discrepancy in affiliation of Mahesh (1st page v/s author’s addresses).