Hi, I was asked to review the draft-ietf-rtgwg-yang-vrrp document, here are my comments: - overall, the module is in a good shape, the common compilers are able to parse it and do not complain about anything - the I-D text is very brief - some more detailed text about the scope, relationship to other (augmented) modules and use cases for the module would be welcome. Similarly, I would appreciate more detailed description of the specific sections of the module. The use cases can be demonstrated on configuration data examples which are now completely missing. - it seems to me that at least some of the leafs in notifications are supposed to be mandatory, maybe not only in notifications, but I'm not expert in this area. - e.g. the interface leaf in vrrp-virtual-router-error-event notification, the leaf is then also used in path expression for vrid-v4 (vrid-v6) leafref - module imports ietf-interfaces and ietf-ip, thus it MUST contain normative references to RFC 7223 and RFC RFC 7277 - in I-D's reference section as well as in the module (as other imported modules are already referenced) - unify the new-master-reason-type enums' names: (master-)preempted vs master-no-response - the local module prefix SHOULD be used instead of no prefix in all path expressions (e.g. vrid-v4, vrid-v6) - consider changing type of the version leaf in vrrp-common-attributes grouping to identityref - that way it would be possible only to augment the current module for any future version of the protocol, enumeration is limiting - vrrp-v3-attributes grouping seems as an addition to the vrrp-common-attributes (at least it is always used together with vrrp-common-attributes). Do you see (e.g. in some other module) a use case to instantiate vrrp-common-attributes without vrrp-v3-attributes? If not, the vrrp-v3-attributes should be placed into vrrp-v3-attributes grouping. It is also question if it needs in that case a separate grouping, the accept-mode leaf could be placed directly into the common grouping just with the when condition. - having a key of the "network" list named "network" (in vrrp-ipv4-attributes/track) seems as a bad design, try to rename the list or its key, also the descriptions of the network list and its container networks are not very clear. - the names in the comments behind the closing brackets inside the vrrp-ipv4-attributes/track container are wrong (e.g. track-interfaces instead of interfaces or track-networks instead of networks) - the previous 2 notes also apply to vrrp-ipv6-attributes grouping - vrrp-state-attributes/up-time - uptime is usually interpreted as a time duration, but here it is used as a moment in time, so if it is not defined this way in VRRP protocol, consider renaming - vrrp-state-attributes/last-event - are the events really so generic that the type here must be a string? Maybe having identities for them could help readability and understandability. Radek