Re: [trill] RtgDir review: draft-ietf-trill-mtu-negotiation-02

[Alia Atlas <[e-mail]>]

Julien,

Thank you very much for your review!

Mingui,

Thank you for addressing the comments.

Regards,
Alia

On Tue, May 10, 2016 at 7:16 AM, Mingui Zhang <[e-mail]>
wrote:

> Hi Julien,
>
> Thanks for pointing it out. That typo will be fixed together with other
> comments we would receive.
>
> Thanks,
> Mingui
>
> > -----Original Message-----
> > From: Julien Meuric [mailto:[e-mail]]
> > Sent: Tuesday, May 10, 2016 4:34 PM
> > To: Mingui Zhang
> > Cc: [e-mail]; [e-mail];
> [e-mail];
> > [e-mail]
> > Subject: Re: RtgDir review: draft-ietf-trill-mtu-negotiation-02
> >
> > Hi Mingui,
> >
> > This version seems to address my comment.
> >
> > Please note a new typo on page 8: s/any LPS/any LSP/
> >
> > Thanks,
> >
> > Julien
> >
> >
> > May. 10, 2016 - [e-mail]:
> > > Hi Julien,
> > >
> > > I have uploaded the 04 version. Please take a look.
> > >
> > > Thanks,
> > > Mingui
> > >
> > >> -----Original Message-----
> > >> From: Julien Meuric [mailto:[e-mail]]
> > >> Sent: Wednesday, May 04, 2016 6:36 PM
> > >>
> > >> Hi Mingui,
> > >>
> > >> I have looked at the diff of your update and have noticed that several
> > "should"
> > >> have been replaced by "need[s] to": I appreciate that you acknowledge
> > >> my comment on this, but none of them being RFC 2119 keyword, I still
> > >> doubt this matches Standards Track expectations.
> > >>
> > >> I caught a nit in section 3: s/is sent to/is set to/
> > >>
> > >> Regards,
> > >>
> > >> Julien
> > >>
> > >>
> > >> May. 03, 2016 - [e-mail]:
> > >>> Hi Julien,
> > >>>
> > >>> The updated version has just been uploaded.
> > >>>
> > >>> Thanks,
> > >>> Mingui
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Julien Meuric [mailto:[e-mail]]
> > >>>> Sent: Monday, May 02, 2016 3:51 PM
> > >>>>
> > >>>> Hi Mingui,
> > >>>>
> > >>>> About the "updated RFCs" issue below, my point is: "please make
> > >>>> sure the text and the hearder are complete". I just had a quick
> > >>>> look at those references, you know better than me which are actually
> > relevant.
> > >>>>
> > >>>> I am looking forward to the updated version.
> > >>>>
> > >>>> Thanks,
> > >>>>
> > >>>> Julien
> > >>>>
> > >>>>
> > >>>> Apr. 29, 2016 - [e-mail]:
> > >>>>> Hi Julien,
> > >>>>>
> > >>>>> Thanks for the comments! Much appreciated!
> > >>>>>
> > >>>>> Please see in-lines below. An updated version will soon be
> > >>>>> uploaded to resolve
> > >>>> the comments.
> > >>>>>
> > >>>>> Thanks,
> > >>>>> Mingui
> > >>>>>
> > >>>>>> -----Original Message-----
> > >>>>>> From: Julien Meuric [mailto:[e-mail]]
> > >>>>>> Sent: Thursday, April 28, 2016 4:34 AM
> > >>>>>>
> > >>>>>> 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://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir
> > >>>>>>
> > >>>>>> Although these comments are primarily for the use of the Routing
> > >>>>>> ADs, it would be helpful if you could consider them along with
> > >>>>>> any other IETF Last Call comments that you receive, and strive to
> > >>>>>> resolve them through discussion or by updating the draft.
> > >>>>>>
> > >>>>>> Document: draft-ietf-trill-mtu-negotiation-02.txt
> > >>>>>> Reviewer: Julien Meuric
> > >>>>>> Review Date: April 27, 2016
> > >>>>>> IETF LC End Date: April 5, 2016
> > >>>>>>
> > >>>>>> Intended Status: Standards Track
> > >>>>>>
> > >>>>>>
> > >>>>>> *Summary:*
> > >>>>>> I have some minor concerns about this document that I think
> > >>>>>> should be resolved before publication.
> > >>>>>>
> > >>>>>> *Comments:*
> > >>>>>> Even though it requires to browse some other TRILL (normative)
> > >>>>>> documents, the mechanism itself is simple and well described.
> > >>>>>> Nevertheless, the specification deserves some improvement when it
> > >>>>>> comes to obligations and options: this was part of my expectation
> > >>>>>> after I realized it was upgrading a short section of the base
> > >>>>>> document (RFC 6325), which needs to be emphasized as well.
> > >>>>>
> > >>>>> In the abstract, it's already mentioned as "optional updates" to
> > >>>>> RFC 6325. I think
> > >>>> "Updates: 6325 " needs to appear in the page header as well.
> > >>>>>
> > >>>>>>
> > >>>>>> *Minor Issues:*
> > >>>>>> - The document is ST and references RFC 2119. There a some "MUST"
> > >>>>>> and one "SHOULD", many of them inherited from specifications out
> > >>>>>> of the referenced documents. On the other side, "must" and
> "should"
> > >>>>>> are commonly used. This MUST be brought up to ST expectations.
> > >>>>>
> > >>>>> The usage of "must" and "should" will be updated as needed. I have
> > >>>>> checked all
> > >>>> those appearances. The changes would be editorial.
> > >>>>>
> > >>>>>> - The document claims to only update RFC 7177. It seems however
> > >>>>>> that it also updates RFC 6325 (section 4.3.2), RFC 7176 and maybe
> > >>>>>> even RFC
> > >> 7780.
> > >>>>>
> > >>>>> Actually, I don't think this document updates RFC7176. It simply
> > >>>>> makes use of
> > >>>> the MTU Sub-TLV defined in RFC 7176.
> > >>>>> The specification about the originatingL1LSPBufferSize of an
> > >>>>> unreachable
> > >>>> RBridge is a slight update to RFC7780. This will be emphasized.
> > >>>>>
> > >>>>>> That should be either acknowledged or clarified. The 4th
> > >>>>>> paragraph of the introduction tries to tackle that topic, but it
> > >>>>>> is not clear enough in defining the position of the document with
> > >>>>>> respect to previously
> > >>>> defined mechanisms.
> > >>>>>
> > >>>>> The updated to RFC 6325 will be emphasized in this paragraph. The
> > >>>>> backward
> > >>>> compatibility will be moved to here as well. It will help the
> positioning.
> > >>>>>
> > >>>>>> - The 3rd paragraph of the introduction duplicates the beginning
> > >>>>>> of the following section 2. A possible way to limit this
> > >>>>>> repetition effect may be to summarize that part of the
> introduction.
> > >>>>>
> > >>>>> Yes, summarization is the proper way.
> > >>>>>
> > >>>>>> - Section 3 specifies an algorithm. Even if it does not rely on a
> > >>>>>> formal language, consistency would be valuable. My mind compiler
> > >>>>>> would
> > >>>> suggest:
> > >>>>>>         * "If" is followed by "then" only once: "then" keyword to
> > >>>>>> be
> > >> dropped?
> > >>>>>>         * The algorithm relies on a break/stop or continue
> > >>>>>> principle; as such, the instance of "Else" at the end should be
> > >>>>>> replaced by "<line
> > >>>>>> break> 4) Repeat Step1";
> > >>>>>>         * "is set to" and "<--" seem to be similar: please pick
> one or
> > clarify;
> > >>>>>>         * to improve readability, I would drop the double naming
> > >>>>>> introduced with X,
> > >>>>>> X1 and X2 and rely on explicit variable names all along, as in
> > >>>>>> the
> > >>>>>> text: e.g., "linkMtuSize" instead of X, "lowerBound" for X1 and
> > >> "upperBound"
> > >>>> instead of X2.
> > >>>>>
> > >>>>> Sure. Explicit names will be used for the sake of readability.
> > >>>>>
> > >>>>>>
> > >>>>>> *Nits:*
> > >>>>>
> > >>>>> The following nits will be fixed as suggested.
> > >>>>>
> > >>>>>> ------
> > >>>>>> "Updates" field in header
> > >>>>>> ---
> > >>>>>> - I think the "RFC" acronym should appear.
> > >>>>>> - The list may be extended with RFC RFC 6325, RFC 7176 and maybe
> > >>>>>> even RFC 7780.
> > >>>>>> ------
> > >>>>>> Abstract
> > >>>>>> ---
> > >>>>>> - s/campus wide MTU feature/campus-wide MTU feature/
> > >>>>>> - s/campus wide capability/campus-wide capability/
> > >>>>>> - s/link local packets/link-local packets/
> > >>>>>> - s/link local MTUs/link-local MTUs/
> > >>>>>> - "It updates RFC..." duplicates header: either to drop or make
> > >>>>>> more specific to point to precise sections/mechanisms.
> > >>>>>> ------
> > >>>>>> Section 1.
> > >>>>>> ---
> > >>>>>> - s/link scope PDUs can/link-scoped PDUs can/
> > >>>>>> ------
> > >>>>>> Section 2.
> > >>>>>> ---
> > >>>>>> - s/campus wide Sz MTU/campus-wide Sz MTU/
> > >>>>>> - s/area wide scope/area-wide scope/
> > >>>>>> - s/domain wide scope/domain wide scope/
> > >>>>>> - s/L1 Circuit Scoped/L1 Circuit-Scoped/
> > >>>>>> - "limited to 1470 to 65,535 bytes": I cannot parse it, is it
> > >>>>>> meant to be a
> > >> range?
> > >>>>>> ------
> > >>>>>> Section 4.
> > >>>>>> - OLD
> > >>>>>> "while RB1 normally ignores link state information for any IS-IS
> > >>>>>> unreachable RBridge RB2, originatingL1LSPBufferSize is an
> exception."
> > >>>>>>       NEW
> > >>>>>> "while in most cases RB1 ignores link state information for IS-IS
> > >>>>>> unreachable RBridge RB2, originatingL1LSPBufferSize is
> meaningful."
> > >>>>>> [current wording suggests it is adding an exception to a
> > >>>>>> mandatory behavior, which AFAIU it does not]
> > >>>>>
> > >>>>> OK. Will update the words.
> > >>>>>
> > >>>>>> ------
> > >>>>>> Section 7.
> > >>>>>> ---
> > >>>>>> - "tested size can be advertised": "can" is to be addressed as
> > >>>>>> part of the loose RFC 2119 wording comment.
> > >>>>>
> > >>>>> Will use the word "MAY" instead.
> > >>>>>
> > >>>>>> ------
> > >>>>>> Section 8.
> > >>>>>> ---
> > >>>>>> - "value [...] had been reported": "reported" puzzles me, maybe
> > "tested"
> > >>>>>> was meant?
> > >>>>>> - The 3rd paragraph "For an Lz-ignorant [...] link-wide Lz."
> > >>>>>> should be moved up to become the second paragraph, so as to
> > >>>>>> clarify what an
> > >>>> Lz-ignorant is.
> > >>>>>
> > >>>>> OK. It will be moved up.
> > >>>>>
> > >>>>>> - "The extension of TRILL MTU negociation...": this is an
> > >>>>>> explicit positioning which should be mentioned earlier in the I-D.
> > >>>>>
> > >>>>>
> > >>>>> OK. This will be moved to the introduction.
> > >>>>>
> > >>>>>> ------
> > >>>>>> Section 10.
> > >>>>>> ---
> > >>>>>> - RFC 7180 bis is now RFC 7780.
> > >>>>>
> > >>>>> Yes, this will be updated.
> > >>>>>
> > >>>>>> ---
> > >>>>>>
> > >>>>>> Regards,
> > >>>>>>
> > >>>>>> Julien
> > >>>>>
>