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

[Julien Meuric <[e-mail]>]

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
>