Last Call Review of draft-ietf-pim-source-discovery-bsr-07
review-ietf-pim-source-discovery-bsr-07-genart-lc-bryant-2018-01-09-00

Request Review of draft-ietf-pim-source-discovery-bsr
Requested rev. no specific revision (document currently at 08)
Type Last Call Review
Team General Area Review Team (Gen-ART) (genart)
Deadline 2018-01-10
Requested 2017-12-20
Other Reviews Secdir Last Call review of -07 by Liang Xia (diff)
Opsdir Last Call review of -07 by Joel Jaeggli (diff)
Review State Completed
Reviewer Stewart Bryant
Review review-ietf-pim-source-discovery-bsr-07-genart-lc-bryant-2018-01-09
Posted at https://mailarchive.ietf.org/arch/msg/gen-art/GVoGxGWpuYdmAVGAz9zA9svUpXU
Reviewed rev. 07 (document currently at 08)
Review result Ready with Nits
Draft last updated 2018-01-09
Review completed: 2018-01-09

Review
review-ietf-pim-source-discovery-bsr-07-genart-lc-bryant-2018-01-09

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>

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

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

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

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.

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.

   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.

A Group Source Holtime ( typo Holdtime)

  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. 

  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

  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.

   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.

   with type 0 assigned to the "Source Group
   Holdtime" TLV.  

SB> use of zero is a common cause of uninitialized variable error.