Re: [Gen-art] Genart last call review of draft-ietf-pim-source-discovery-bsr-07

[Stig Venaas <[e-mail]>]

Hi, thanks for the detailed review. I've tried to address most of
these comments, please see inline.

On Tue, Jan 9, 2018 at 12:16 PM, Stewart Bryant
<[e-mail]> wrote:
> Reviewer: Stewart Bryant
> Review result: Ready with Nits
>
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
>
> For more information, please see the FAQ at
>
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>
> Document: draft-ietf-pim-source-discovery-bsr-07
> Reviewer: Stewart Bryant
> Review Date: 2018-01-09
> IETF LC End Date: 2018-01-10
> IESG Telechat date: 2018-01-25
>
> Summary: A well written document, but which could usefully be polished to make
> it even better.
>
> Major issues: None
>
> Minor issues:
>
>    E.g., if
>    some information needs to be sent every 60 seconds and a triggered
>    PFM is about to be sent 20 seconds before the next periodic PFM was
>    scheduled, the triggered PFM might include the periodic information
>    and the next periodic PFM can then be scheduled 60 seconds after
>    that, rather than 20 seconds later.
>
> SB> I am not sure is the complexity of this optimization is worth the
> SB> gain. In general optimizations are a source of implementation bugs
> SB>

Agree in general, but I'm trying to leave this open to the
implementation. In my implementation it was easier to include all the
information than just the triggered message. From protocol perspective
it should not be necessary to restrict this.

> Nits/editorial comments:
>
> In the Abstract  RP need expanding.

Done

> topology changes, including frequest link or router failures. (frequent
> incorrectly spelled)

Done

> no bandwidth left for prune message (message should be plural)

Done

> If all the TLVs in the
> message are boundary TLVs, then the message is effectively ignored.
> SB> Ignored or discarded?
>
> Then a couple of lines later:
> If the message was discarded or all the
> TLVs were ignored, then no message is forwarded.
> SB> You have both terms (ignored and discarded) which confuses me.

My thinking is that one can discard a message by basically throwing it
away without processing it. While a TLV is something that can be
ignored, not discarded. In the text above what you quoted I also talk
about when a message might be discarded, and when TLVs should be
ignored.

> In 3.4.1 you pull 60s out of a hat. All becomes clearer later. It would be
> better for the reader if you explained that this was your choice of default
> timer before first use.

I added text that the behavior, and the value of 60s, is similar to
what is defined for the No-Forward bit in RFC 5059.

>    if the most significant bit in the type field is set (the type value
>    is larger than 32767) and this system does not support the type, then
>    that particular type should be omitted from the forwarded messages.
> SB> What the authors describe is fine and correct, but it might be
> SB> cleaner to call the top bit the "omit bit" or some memorable name.
> SB>  Of course the fact that it is the top bit of a TLV type make make such
> SB> a definition strange.

I've now changed this to define a "Transitive Bit", and making the
type space 15 bits.

> A Group Source Holtime ( typo Holdtime)

Thanks

>   The PFM messages containing the GSH
>    TLV are periodically sent for as long as the multicast source is
>    active, similar to how PIM registers are periodically sent.  The
>    default announcement period is 60 seconds,
>
> SB> This should be called out earlier in the text to show why you keep
> SB> using the 60s value.

Earlier I only mentioned 60s related to a new neighbor coming up,
which is unrelated to this value, and one example.

>   The holdtime for the source is by default 210
>    seconds.  Other values MAY be configured, but the holdtime MUST be
>    either zero, or larger than the announcement period.  It is
>    RECOMMENDED to be 3.5 times the announcement period.  A source MAY be
>    announced with a holdtime of zero to indicate that the source is no
>    longer active.
>
> SB> This would be clearer if you started with the 3.5 AP recommendation
> SB> and then got to 210s

Done

>   When a new PIM neighbor is detected, or an existing neighbor changes
>    GenID, an implementation MAY send a triggered PFM message containing
>
> SB> GenID seems to be used but not defined.

Done

>    This document requires the assignment of a new PIM message type for
>    the PIM Flooding Mechanism (PFM).
>
> SB> It is normal to be more explicit in describing the IANA action.
>
>    IANA is also requested to create a
>    registry for PFM TLVs,
>
> SB> It is usual to specify the namespace for the registry.

I now included the exact name and initial content of the registry.

>    with type 0 assigned to the "Source Group
>    Holdtime" TLV.
>
> SB> use of zero is a common cause of uninitialized variable error.

Right. I now reserved 0, and specified 1 for this TLV.

I'll submit a new revision today. Have another look if you like and
let me know if you have any further comments.

Thanks,
Stig