Re: [pim] Roman Danyliw's No Objection on draft-ietf-pim-igmp-mld-yang-13: (with COMMENT)

[Xufeng Liu <[e-mail]>]

Hi Roman,

Thanks for the review. We have posted an updated revision
https://tools.ietf.org/html/draft-ietf-pim-igmp-mld-yang-14, to address
these issues.

Regards,
- Xufeng

On Wed, May 29, 2019 at 9:55 AM Roman Danyliw via Datatracker <
[e-mail]> wrote:

> Roman Danyliw has entered the following ballot position for
> draft-ietf-pim-igmp-mld-yang-13: No Objection
>
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
>
>
> Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-pim-igmp-mld-yang/
>
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> (1) I support Ben Kaduk’s DISCUSS about privacy considerations.
>
> (2) Thanks for Section 1.3.  This upfront cross-reference was very helpful!
>
> (3) Section 2.2.
>    On the other hand, operational state parameters are not so widely
>    designated as features, as there are many cases where the defaulting
>    of an operational state parameter would not cause any harm to the
>    system, and it is much more likely that an implementation without
>    native support for a piece of operational state would be able to
>    derive a suitable value for a state variable that is not natively
>    supported.
>
> I struggled to understand this very dense single sentence paragraph.
> -- What does it mean for “operational state parameters are not so widely
> designated as features”?

[Xufeng]: In the modeling language YANG, an optional section (that can be a
group of parameters) may be marked as a YANG “feature”, meaning that this
section is “optionally” supported by an implementation. If an
implementation supports this “feature”, the name and status of this feature
are sent to the client. In the design of this model, we do not use many
such “features” for operational states. The result is that an
implementation will have to return the values of the states, without the
option of not implementing them.


> -- What is a “defaulting of an operational state
> parameter”?

[Xufeng]: As mentioned in the previous paragraph, an implementation needs
to return many of the states specified in this model without the vendor’s
choice. Usually the states that an implementation would want to opt-out are
these states that are not critical for the protocol to operate, and the
implementation usually has used a default value. Therefore, the
implementation may always return the default value when the client ask for
such a state, hence the phrase “defaulting of an operational state
parameter”.

If better wording can make this paragraph more comprehensible, please
advise.

>
> (4) Section 3.  Does the sentence “This model augments the core routing
> data
> model specification in [RFC8349]” suggest that this draft should update
> [RFC8349]?
>
[Xufeng]: The suggestion is logically correct, but the current practice for
YANG model document is not to update the base model [RFC8349], but to make
a reference to it. I think that it is because of the modular and extensible
natures of YANG, there could be hundreds of models augmenting a base model,
both inside and outside of IETF.

>
> (5) Reference Nits
> ** Section 1.  I would have expected a reference to NMDA (i.e.,
> [RFC8342]).
> This is done later in the draft but not the first time it is mentioned
> (here).
>
[Xufeng]: Added.

>
> ** idnits returned the following valid reference issues:
>
>   == Missing Reference: 'I-D.ietf-netconf-yang-push' is mentioned on line
>      178, but not defined
>
[Xufeng]: Added.


>
>   == Missing Reference: 'RFC 8446' is mentioned on line 1910, but not
> defined
>
[Xufeng]: Added.


>
>   == Missing Reference: 'RFC8341' is mentioned on line 1912, but not
> defined
>
[Xufeng]: Added.


>
>   == Unused Reference: 'RFC5246' is defined on line 2085, but no explicit
>      reference was found in the text
>
[Xufeng]: Removed.


>
>   == Unused Reference: 'RFC6536' is defined on line 2099, but no explicit
>      reference was found in the text
>
[Xufeng]: Removed.


>
>   == Unused Reference: 'RFC5790' is defined on line 2145, but no explicit
>      reference was found in the text
>
[Xufeng]: Fixed.


>
>   ** Downref: Normative reference to an Informational RFC: RFC 3569
>
[Xufeng]: Fixed as Alvaro suggested.


>
>   ** Obsolete normative reference: RFC 5246 (Obsoleted by RFC 8446)
>
[Xufeng]: Removed


>
>   ** Obsolete normative reference: RFC 6536 (Obsoleted by RFC 8341)
>
[Xufeng]: Removed


>
> (6) Editorial Nits
>
> ** Section 2.1.  The sentence “Even though there is no protocol specific
> notifications are defined in this model, the subscription and push
> mechanism
> defined in [I-D.ietf-netconf-subscribed-   notifications] and
> [I-D.ietf-netconf-yang-push]” doesn’t parse.  Likely remove the “are”.
>
[Xufeng]: Fixed


>
> ** Section 2.1.  Per the sentence “Depending on the implementation choices,
> some systems may not allow some of the advanced parameters   configurable”,
> what is a “advanced parameters configurable”?  I think there is a typo
> there.
>
[Xufeng]: Fixed.


>
> **  Section 2.3.  Per “The current document defines …”, shouldn’t this
> just be
> “The document defines …” since when published as an RFC there would be no
> notion of versioning as suggested by “current”?
>
[Xufeng]: Edited.


>
> ** Section 2.3.  s/supports and only supports/only supports/
>
[Xufeng]: Fixed.


>
> ** Section 4.  Typo.  s/refered/referred/
>
[Xufeng]: Fixed.


>
> ** Section 4. Missing space.  /Querier.In RFC3376,/Querier. In RFC3376,/
>
[Xufeng]: Fixed.