Thanks, all those changes seem good to me. If I am asked to review the next version for the IESG telechat, I expect to say "Ready".
Regards Brian On 27/06/2017 20:06, Zhangmingui (Martin) wrote: > Hi Brian and Donald, > > Thanks a lot for the comments. Please see the responses as inline below. > >> -----Original Message----- >> From: Donald Eastlake [mailto:d3e...@gmail.com] >> Sent: Tuesday, June 27, 2017 11:03 AM >> To: Brian Carpenter >> Cc: gen-art@ietf.org Review Team; >> draft-ietf-trill-mtu-negotiation....@ietf.org; >> tr...@ietf.org >> Subject: Re: Genart last call review of draft-ietf-trill-mtu-negotiation-05 >> >> Hi Brian, >> >> Thanks for the comments. See below. >> >> On Fri, Jun 23, 2017 at 9:32 PM, Brian Carpenter >> <brian.e.carpen...@gmail.com> wrote: >>> Reviewer: Brian Carpenter >>> Review result: Ready with Issues >>> >>> Gen-ART Last Call review of draft-ietf-trill-mtu-negotiation-05 >>> >>> 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 >>> <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>. >>> >>> Document: draft-ietf-trill-mtu-negotiation-05.txt >>> Reviewer: Brian Carpenter >>> Review Date: 2017-06-24 >>> IETF LC End Date: 2017-06-28 >>> IESG Telechat date: 2017-07-06 >>> >>> Summary: Ready with issues >>> -------- >>> >>> Minor issues: >>> ------------- >>> >>>> 2. Link-Wide TRILL MTU Size >>> ... >>>> ...RBridges MUST support the Extended L1 Circuit-Scoped >>>> (E-L1CS) flooding scope LSP (FS-LSP). They use that flooding to >>>> exchange their maximally supportable value of "Lz". >>> >>> Where does that value come from? Is it configured, derived from the >>> interface in some way, or discovered? >> >> It's somewhat similar to the originatingL1LSPBufferSize which is talked >> about in >> Section 5 of RFC 7780, except that there is no reason to worry about >> coordinating across the TRILL campus. How about adding wording something >> like: >> >> The originatingSNPBufferSize for a port is the minimum of the following >> two quantities, but not less than 1470 bytes: (1) the maximum MTU of the port >> and (2) the maximum LSP size that the TRILL IS-IS implementation can handle, > > [Mingui] OK. > >> >>>> 2.1. Operations >>>> >>>> Lz is reported using a originatingSNPBufferSize TLV that MUST occur >>>> in fragment zero of the RBridge's E-L1CS FS-LSP. An >>>> originatingSNPBufferSize APPsub-TLV occurring in any other fragment >>>> is ignored. >>> >>> Is that really what you mean? I thought Lz was an optional extra. So I >>> think you mean: >>> >>> 2.1. Operations >>> >>> Lz MAY be reported using a originatingSNPBufferSize TLV that occurs >>> in fragment zero of the RBridge's E-L1CS FS-LSP. An >>> originatingSNPBufferSize APPsub-TLV occurring in any other fragment >>> MUST be ignored. > > [Mingui] OK. > >> >> Yes, the "MUST" was just in reference to being in fragment zero, not that it >> has >> to occur, so your wording seems better. >> >>>> 3. Link MTU Size Testing >>> ... >>>> Step 0: >>> ... >>>> b) Link MTU size is set to 1470, lowerBound is set to 1470, >>>> upperBound is set to the link-wide Lz, linkMtuSize is set to >>>> [(lowerBound + upperBound)/2] (Operation "[...]" returns the >>>> fraction-rounded-up integer.). >>> >>> This is confusing. "linkMtuSize" was defined as a local variable. >>> But what is "Link MTU size"? Is that another local variable? >>> If so, how is it different from "linkMtuSize"? >>> It is used again in part 2) of step 2 below. >> >> I don't want to say anything about that before checking with the primary >> author. > > [Mingui] As specified in the text leading the Steps, "linkMtuSize" is a local > integer variable for a specific RBridge while "link MTU size" is not a > variable but a value that is agreed by two connected RBridges. To avoid the > confusion, I changed "linkMtuSize" to "X" and add the text to explain that > link MTU size is a value that is agreed by two connected RBridges. > >> >>> Also, I assume "Lz" is the value previously agreed among the nodes, >>> but that should be made clear to the reader. > > [Mingui] Agree. Added the word "agreed" in Section 2. > >>> >>> Nits: >>> ----- >>> >>>> 1. Introduction >>> ... >>>> topology. While in this document, a new RECOMMENDED link-wide >> minimum >>>> inter-RBridge MTU size, Lz, is specified. By calculating a using Lz >>>> as specified herein, link-scoped PDUs can be formatted greater than >>>> the campus-wide Sz up to the link-wide minimum acceptable inter- >>>> RBridge MTU size potentially improving the efficiency of link >>>> utilization and speeding link state convergence. >>> >>> I cannot parse those two sentences. What does the "While" refer to? >>> What does "By calculating a using Lz" mean? >> >> I believe the sentences should be >> >> "... In this document, a new RECOMMENDED link-wide minimum inter-RBridge >> MTU size, Lz, is specified. By calculating and using Lz as specified herein, >> ..." > > [Mingui] OK. > >> >>>> 3. Link MTU Size Testing >>> ... >>>> b) Link MTU size is set to 1470, lowerBound is set to 1470, >>>> upperBound is set to the link-wide Lz, linkMtuSize is set to >>>> [(lowerBound + upperBound)/2] (Operation "[...]" returns the >>>> fraction-rounded-up integer.). >>> >>> This would be easier to understand: >>> >>> 3. Link MTU Size Testing >>> ... >>> b) Link MTU size is set to 1470, lowerBound is set to 1470, >>> upperBound is set to the link-wide Lz, linkMtuSize is set to >>> [(lowerBound + upperBound)/2], rounded up to the nearest >> integer. >> >> OK. >> >>> Repeat this in the following two cases; a normal reader will not >>> remember the rounding rule: > > [Mingui] Changed three occurrences in the document. > > >>> >>> ... >>> 1) If RB1 fails to receive an MTU-ack from RB2 after k tries: >>> >>> upperBound is set to linkMtuSize and linkMtuSize is set to >>> [(lowerBound + upperBound)/2], rounded up to the nearest >> integer. >>> >>> 2) If RB1 receives an MTU-ack to a probe of size linkMtuSize from >>> RB2: >>> >>> link MTU size is set to linkMtuSize, lowerBound is set to >>> linkMtuSize and linkMtuSize is set to [(lowerBound + >>> upperBound)/2], rounded up to the nearest integer. >> >> That seems reasonable. >> >>> -- >> >> Thanks, >> Donald >> =============================== >> Donald E. Eastlake 3rd +1-508-333-2270 (cell) >> 155 Beaver Street, Milford, MA 01757 USA d3e...@gmail.com > > > > Thanks, > Mingui > _______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
