Re: [yang-doctors] YANG Doctors review for draft-ietf-rtgwg-yang-vrrp

[Radek Krejčí <[e-mail]>]

Hi Xufeng,
comments are inline

Dne 15.3.2017 v 22:31 Xufeng Liu napsal(a):
> Hi Radek,
>
> Thanks for the thorough review. We have submitted an updated revision https://datatracker.ietf.org/doc/draft-ietf-rtgwg-yang-vrrp/ to address the issues.
>
> Regards,
> - Xufeng
>
>> -----Original Message-----
>> From: Radek Krejčí [mailto:[e-mail]]
>> Sent: Thursday, December 22, 2016 9:27 AM
>> To: [e-mail]
>> Cc: [e-mail]; Benoit Claise (bclaise) <[e-mail]>
>> Subject: YANG Doctors review for draft-ietf-rtgwg-yang-vrrp
>>
>> 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.
> [Xufeng] We have added Section 2 to describe the model, and Appendix A to show a configuration data example.

It is better now, however, few notes:

- mixing the tree format with columns to show borders between the modules in section 2.2 seems weird. If you want to use the tree format, use prefixes as pyang or yanglint do.

- in section 2.5, use the notification names to clearly connect the text here with the specific notification

- I don't thing that it is necessary to print the complete tree in the section 3. Instead, show the currently hidden nodes in sections 2.x (and add the notifications part of the tree into section 2.5)

>
>> - 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
> [Xufeng] 
> Fixed.

- in vrrp-virtual-router-error-event notification, I believe that vrid-v4 and vrid-v6 are mutually exclusive, and also you may want to have one of them to be always present, right? If so, put them into the choice:

choice vrid {
  mandatory true; /* if one of the leafs always expected */
  leaf vrid-v4 { ... }
  leaf vrid-v6 { ... }
}

>
>> - unify the new-master-reason-type enums' names: (master-)preempted vs
>> master-no-response
> [Xufeng] Is there any particular suggestions?

personally I prefer shorter names, so 'not-master', 'priority', 'preempted' and 'no-response'

>
>> - 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)
> [Xufeng] Fixed.

still the same issue in {grouping}[vrrp-ipv4-attributes]/track/networks/network (and the same for ipv6 grouping)

>> - the previous 2 notes also apply to vrrp-ipv6-attributes grouping
> [Xufeng] Fixed.

Radek