Hi Warren, I have just submitted revision 10 with the updates.
Thanks, Tim On 2/24/21, 3:18 PM, "Warren Kumari" <war...@kumari.net> wrote: On Wed, Feb 24, 2021 at 3:22 PM Tim Evens (tievens) <tiev...@cisco.com<mailto:tiev...@cisco.com>> wrote: Hi Warren, Thank you so much for the review. We agree with those changes. We have made the requested changes, but we cannot submit them until after Mar-8th. Until then, I have attached a text diff output. You can also see the changes at https://github.com/TimEvens/draft-ietf-grow-bmp-loc-rib. You can compare tag revisions. Awesome, thank you very much. Please let me know LOUDLY once you've submitted, and I'll kick off IETF LC. It will probably have to wait until just after IETF ends, so that people can pay attention... Thank again for the quick turn around, W Thanks, Tim On 2/22/21, 9:27 AM, "Warren Kumari" <war...@kumari.net<mailto:war...@kumari.net>> wrote: 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<http://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 -- The computing scientist’s main challenge is not to get confused by the complexities of his own making. -- E. W. Dijkstra
_______________________________________________ GROW mailing list GROW@ietf.org https://www.ietf.org/mailman/listinfo/grow
