Subject: Re: RtgDir review: draft-ietf-l2vpn-vpls-mcast-14.txt

From: "Bocci, Matthew (Matthew)" <[email-masked]>
Date: Thu, 3 Oct 2013 13:44:42 +0000

Yakov

Thanks for your responses. Please see below.

On 30/09/2013 01:07, "Yakov Rekhter" <[email-masked]> wrote:

>Matthew,
>
>> Hello,
>> 
>> I have been selected as the Routing Directorate reviewer for this draft.
>> The Routing Directorate seeks to review all routing or routing-related
>> drafts as they pass through IETF last call and IESG review, and
>> sometimes on special request. The purpose of the review is to provide
>> assistance to the Routing ADs. For more information about the Routing
>> Directorate, please see
>> http://www.ietf.org/iesg/directorate/routing.html
>> 
>> Since IETF LC has ended for this draft, I am primarily providing
>>comments t=
>> o
>> aid the ADs, and I hope the authors will also take them into
>>consideration =
>> to improve
>> the readability  of the draft.
>
>Thanks for your review and comments.
>
>> Document: draft-ietf-l2vpn-vpls-mcast-14.txt
>> Reviewer: Matthew Bocci
>> Review Date: 26 September 2013
>> IETF LC End Date: date-if-known
>> Intended Status: Proposed Standard
>> 
>> Summary:
>> 
>> I have some minor concerns about this document that I think should
>> be resolved before publication.
>> 
>> Comments:
>> 
>> The draft appears to be complete and accurate form a technical
>>perspective.=
>> However,
>> the style of the draft and the density of the text make it quite
>>difficult =
>> to
>> read at times. It took several passes of some sections of the document,
>>and=
>> returning
>> to earlier sections, to properly follow the text. I believe this is
>>because=
>> the basic
>> multicast architecture could be better-explained earlier in the draft.
>> 
>> Major Issues:
>> 
>> No major issues found.
>> 
>> Minor Issues:
>> 
>> 1) The draft contains both introductory architectural / framework
>> descriptions, and more detailed specifications of the protocols required
>> to efficiently support multicast in VPLS.  However, the introductory
>>sections
>> (e.g. the overview in section 5) are presented in the form of extremely
>> dense text, and while
>> they seem to be trying to explain architectural concepts, they do not
>>use 
>> a single diagram to help the reader visualise the solutions in the
>>draft. 
>> In fact, the first
>> figures illustrating the protocol hierarchies and relationships do not
>> appear until the
>> label stacks are described in Section 13 on page 43.
>> 
>>  I think inserting some figures in section 5 to illustrate the
>>different 
>>  multicast schemes, and also moving the data forwarding section earlier
>>in 
>>  the document, would greatly help the reader.
>
>Wrt "inserting some figures in section 5", I would be glad to do this,
>provided you elaborate a bit on what specific figures you would like
>to insert.

I think that it would help to add a figure to each of the bullets in
section 5.1 (inclusive trees and selective trees) illustrating the
architecture of each (a couple of reference models would be helpful).

> 
>
>Wrt moving the data forwarding section earlier in the document,
>the placement of the data forwarding section follows the structure
>of rfc6513.

OK, but at least it would help if you added an illustration to the two
bullets I have referenced above, which shows the label stacks in each of
the cases. 

>
>> 2) Cross-references in the text use the title of the referenced
>>section, bu=
>> t do not
>> include a section number. Personally, I found that this made the draft
>>much=
>>  more difficult
>> to navigate than it should be. It would help to add section numbers to
>>each=
>>  instance of
>> a cross reference.
>
>This will be fixed as part of the RFC Editor process.

OK thanks

>
>> Nits:
>> 
>> Abstract, 2nd paragraph, 1st sentence:
>> s/solution/solutions
>
>Fixed.

Thanks

>
>> Section 3: This section would be clearer of it was in tabular form,
>> in a similar manner to other RFCs.
>
>I would prefer to keep it in the present form, as there are plenty
>of other RFCs that have similar form.

OK I guess it is a personal preference.

>  
>> You could also add short (one sentence) definitions of 'Inclusive
>>Trees', '=
>> Selective Trees', and 'Aggregation'.
>
>Will do.

Thanks

>
>> Sec 4. Introduction: 3rd para:
>> s/when large/when a large
>
>Fixed.
>
>> Sec 4. Introduction: 5th para:
>> s/RSVP-PE/RSVP-TE
>
>Fixed.
>
>> Sec 5.2. BGP-Based VPLS Membership Auto-Discovery: 1st para:
>> s/that have membership these/that have membership of these
>
>Fixed.
>
>> Sec 5.4 Advertising P-Multicast..., 2nd para:
>> Grammar: The decision to bind a set of VPLS instances or customer
>>multicast=
>>  group is
>> local to the ingress PE matter, / The decision to bind a set of VPLS
>>instan=
>> ces or
>> customer multicast groups is a local matter for the ingress PE ,
>
>Fixed.
>
>> Sec 5.4 Advertising P-Multicast..., 3rd para:
>> s/be included. / be included
>
>Fixed.
>
>> Section 5.5 Aggregation:
>> 
>> The following algorithm is hard to parse:
>> + ((Average number of VPLS instances on a PE / Aggregation Factor)
>>        * number of PEs) / (Average number of P-Multicast trees that
>>        transit a given P router)
>> 
>> I think this could be restated in a more readable manner. Just as an
>>exampl=
>> e:
>>       AveVPLS            NPE
>>       -------    X    --------
>>         Aggr          AvePTree
>> 
>>     Where: AveVPLS is the average number of VPLS instances on a PE
>>            Aggr is the aggregation factor
>>            NPE is the number of PEs
>>            AvePTree is the average number of P-multicast that transit a
>>giv=
>en P-router
>> 
>
>Will fix.

Thanks

>
>> Section 6: Inter-AS Inclusive P-Multicast..., 5th Paragraph
>> This paragraph mentions FEC128 and FEC 129. It would be better to call
>>thes=
>> e by their
>> formal names used in RFC4447 i.e. The PWid FEC and the Generalized PWid
>>FEC
>
>Will fix.

Thanks

Matthew
>Yakov.

---

From: Yakov Rekhter <[email-masked]>
Date: Thu, 3 Oct 2013 07:32:58 -0700

Matthew,

> architecture of each (a couple of reference models would be helpful).

I am not sure I agree. In fact, the concepts of inclusive and selective
trees have been "borrowed" from rfc6513/rfc6514, and neither of these
RFCs have any figures that illustrate inclusive/selective trees.

> bullets I have referenced above, which shows the label stacks in each of

The label stack is exactly the same for both inclusive and selective
trees. Section 13 shows the label stacks for inclusive trees
(both non-aggregate and aggregate), and since the label stack
is the same for both inclusive and selective trees, the figures
the label stack shown in these figures equally applies to 
both inclusive and selective trees.

Would the following new text in 13.1 address your concern ?

   While this section shows the label stack for inclusive trees, 
   since the label stack is exactly the same for both inclusive and
   selective trees, the label stack shown in this section is equally
   applicable to selective trees.

Yakov.

---

From: "Bocci, Matthew (Matthew)" <[email-masked]>
Date: Fri, 11 Oct 2013 15:21:53 +0000

Yakov

>RFCs have any figures that illustrate inclusive/selective trees.

I would certainly find it easier to read, to explain how those concepts
apply in VPLS scenarios, even if the original RFCs do not use diagrams. At
very least, if you have borrowed the terms, then a reference here to those
RFCs would be helpful.
>   applicable to selective trees.

OK

Matthew