Reviewer: Ebben Aries Review result: On the Right Track 2 modules in this draft: - ietf-rsvp@xxxxxxxxxxxxxxx - ietf-rsvp-extended@xxxxxxxxxxxxxxx No YANG compiler errors or warnings (pyang 2.0, yanglint 1.1.16, confdc 6.6.1) Module ietf-rsvp@xxxxxxxxxxxxxxx: - Remove WG Chairs from contact information per https://tools.ietf.org/html/rfc8407#appendix-B - Use of 'state' containers. It is stated in Section 2.3 that 'Derived state data is contained under a "state" container...'. My only comments here are: a) Should use caution when using 'state' containers in NMDA compliant modules. Rob put together a nice doc here that I won't reiterate: https://github.com/netmod-wg/FAQ/wiki/NMDA-Modelling-FAQ Using such nomenclature locks you into r/o nodes only. b) In some cases, the hierarchy is a bit redundant (statistics/state). Other NMDA compliant modules will not introduce another 'state' hierarchy for instance (see ietf-interfaces) - leaf 'packet-len' is abbreviated while most other leafs are not - authentication-key is of type string. Is this leaf mean to be clear-text? There is nothing associated with this type that would imply special treatment in handling. - crypto-algorithm: Are all identities from ietf-key-chain supported? - Pluralization on counters: e.g. 'in-error' vs. 'in-errors'. Maintain consistency with other modules (see ietf-interfaces) - Normative references are missing for some of the modules imported. These must be added per https://tools.ietf.org/html/rfc8407#section-3.9 Module ietf-rsvp-extended@xxxxxxxxxxxxxxx: - Remove WG Chairs from contact information per https://tools.ietf.org/html/rfc8407#appendix-B - Use of 'state' containers. It is stated in Section 2.3 that 'Derived state data is contained under a "state" container...'. My only comments here are: a) Should use caution when using 'state' containers in NMDA compliant modules. Rob put together a nice doc here that I won't reiterate: https://github.com/netmod-wg/FAQ/wiki/NMDA-Modelling-FAQ Using such nomenclature locks you into r/o nodes only. b) In some cases, the hierarchy is a bit redundant (statistics/state). Other NMDA compliant modules will not introduce another 'state' hierarchy for instance (see ietf-interfaces) - Pluralization on counters: e.g. 'in-error' vs. 'in-errors'. Maintain consistency with other modules (see ietf-interfaces) - 9 features are defined with no 'if-feature' statements. Where are these feature conditions meant to be used? - Normative references are missing for some of the modules imported. These must be added per https://tools.ietf.org/html/rfc8407#section-3.9 General comments on the draft/modules: - The state container and list key designs appear very familiar to that of OpenConfig modules however not consistent with IETF module design. Consistency is key from each producing entity (e.g. IETF in this case) - The draft mentions RPCs and Notifications however none are defined within the modules - Section 2.3: Has examples of RPCs and Notifications that are non-existant in the modules - Abstract: s/RVSP/RSVP/ - Abstract: s/remote procedural/remote procedure/ - Section 2: s/extensiion/extension/ - Section 4: Update the security considerations section to adhere to https://tools.ietf.org/html/rfc8407#section-3.7 and https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines - Various (missing) wording/pluralization cleanup throughout that I'll defer to the RFC Editor. A once over proofread should solve this.
