Overall: - no compiler warnings; passes --ietf check as well Normative Sections: sec 1: - YANG version cited should be RFC 7950, not 6020. YANG 1.1 is used in this document.\ sec 4: Relationship with RFC 7317 - this section should say how overlapping configuration objects interact. Does setting 1 field (e.g., /system/ntp/enabled) change the other field at the same time (e.g., /ntp/enabled) It seems the intent is to ignore and deprecate /system/ntp if /ntp is implemented. This section should explain. sec 5: - this section is supposed to have citations for every imported YANG module used in the ietf-ntp YANG module YANG Module Issues: 1 - Indexing used in /ntp/associations list key "address local-mode isConfigured"; a) will there really be multiple instances per address for different association-modes? The list description-stmt should explain. b) There can be configured values (isConfigured key) and learned entries for the same address and association-modes? Why isn't NMDA used instead? (i.e. intended and operational datastores used instead of the isConfigured list key?) 2 - Purpose of /ntp/access-rules There is no explanation for what it means to configure an entry here that points at an ACL entry in /acls/acl; The description statement needs to specify the details. Doesn't the ACL module just work? How does the /ntp/access-rules/access-rule list change behavior? 3 - Reinvent Key Management It does not seem like a good idea for every protocol to duplicate key management functionality. The draft should explain why other YANG modules related to this work are not relevant here 4 - Reference statements There are no reference statements used in any objects or typedes Definitions which are intended to match a definition in NTP should include a reference-stmt Minor Issues: - typedef names should be singular - access-mode not access-modes - association-mode not association-modes - grouping comman-attributes should be common-attributes - mixed-case naming should not be used (isConfigured) - indentation used in module needs a lot of corrections - some descriptions have incorrect tense (e.g., "NTP is enable") - port definition should be based on inet:port-number, not uint16 OLD: type uint16 { range "123 | 1025..max"; } NEW: type inet:port-number { range "123 | 1025..max"; } - /ntp/authentication/trusted-keys Why is this list needed? Seems a leaf (e.g., trusted-key (true/false) added to the authentication-keys list is sufficient - /ntp/clock-state : some leafs should have a units-stmt instead of specifying the units in the description-stmt (could also apply elsewhere) - Examples should use line continuation convention e.g.: OLD: 10-10-2017 07:33:55.300 Z+05:30 NEW: 10-10-2017 07:33:55.300 Z+05:30\ - a spell checker should be used; There are many description-stmts with spelling errors