[GROW] AD Review of draft-ietf-grow-bmp-local-rib

[Warren Kumari <[e-mail]>]

Hi authors and WG,

Thank you for this document, I believe that allowing BMP to share Loc-RIB
is clearly a good thing.

I do have a few comments/nits that addressing now should help the IETF
LC and IESG eval go more smoothly.

Please SHOUT loudly once you've had a chance to address these.

AD Review of draft-ietf-grow-bmp-local-rib
--------------------------------------------

1: "As shown in Figure 2, Locally originated section 9.4 of [RFC4271]"
I'm unable to parse this - changing "As shown in Figure 2, Locally
originated" into "As shown in Figure 2, Locally Originated into Loc-RIB,
..." doesn't fix it, because the figure doesn't really "show what Sec 9.4
of RFC4271" says.
Perhaps something like: "Figure 2 (Locally Originated into Loc-RIB)
illustrates how redistributed or otherwise originated routes get installed
into the Loc-RIB based on the decision process selection in [RFC4271]"


2: In Section 1.1 the document says things like: "The current method
introduces the need..."
Once the document is published, the phrase "the current method" seems
incorrect, but I don't have a better suggestion...

3: "Locally sourced routes MUST be conveyed using the Loc-RIB instance peer
type."
Should this be "locally sourced BGP routes"? It would be silly to think
that this might carry e.g OSPF only routes, but you have a MUST, so
important to be explicit.
This also seems to conflict with "The F flag indicates that the Loc-RIB is
filtered". Perhaps that above is better worded something like:
"If locally sourced routes are communicated using BMP, they MUST be
conveyed using the Loc-RIB instance peer type." ?

4: " The Loc-RIB contains all routes selected by the BGP protocol Decision
Process section 9.1 of [RFC4271]."
Similar to #1 - perhaps this is just missing a "in section of..."? Still
needs rewording.

5: "These routes include those learned from BGP peers via its Adj-RIBs-In
post-policy, as well as routes learned by other means section 9.4 of
[RFC4271]."
Similar -- I suspect that there was an errant search and replace which
clobbered some text?

6: "Peer AS: Set to the BGP instance global or default ASN value."
Erm, what's this default ASN value?

7: "5.1.  Per-Peer Header"
I think that this section needs a pointer to RFC7854 Sec 4.2.

8: "Capabilities MUST include 4-octet ASN"
s/include 4/include the 4/

9: "For example, prefix 10.0.0.0/8 is updated "
Please use RFC5737 examples instead.


Nit:
1: "This is overly complex for such a simple application that only needed
to have access to the Loc-RIB."
s/needed/needs/

2: It can greatly reduce time to troubleshoot and resolve issues if
operators had the history of Loc-RIB changes.
s/had/have/

3: "BGP Instance: it refers to an"
s/it//



-- 
Perhaps they really do strive for incomprehensibility in their specs.
After all, when the liturgy was in Latin, the laity knew their place.
-- Michael Padlipsky