Subject: [RTG-DIR] Rtgdir last call review of draft-ietf-teas-yang-te-types-06

From: Ines Robles via Datatracker <[email-masked]>
Date: Tue, 02 Apr 2019 00:38:02 -0700

Reviewer: Ines Robles
Review result: Has Nits

The routing directorate will, on request from the working group chair, perform
an “early” review of a draft before it is submitted for publication to the
IESG. The early review can be performed at any time during the draft’s lifetime
as a working group document. The purpose of the early review depends on the
stage that the document has reached.

As this document is in working group last call, my focus for the review was to
determine whether the document is ready to be published. Please consider my
comments along with the other working group last call comments.

For more information about the Routing Directorate, please see
​http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Document: draft-ietf-teas-yang-te-types-06.txt
Reviewer: Ines Robles
Review Date: 02/04/2019
Intended Status: Standards Track

Summary:

I believe the draft is technically good. This document is well written and
clear to understand. The draft is quite complete.

The document defines a collection of common data types and groupings in YANG
data modeling language. These derived common types and groupings are intended
to be imported by modules that model Traffic Engineering (TE) configuration and
state capabilities.

I found some minor nits and have some minor questions, it would be nice if they
could be addressed.

Major issues: Not found

Minor issues: Not found

Nits/editorial comments:

- NBMA could be expanded in pag 14  NBMA (Non-broadcast multiple-access
network) or maybe added into Acronyms and Abbreviations section. Same for SF
(pag 32), SD (Pag 33), APS (pag 35) and PM (pag 46)

-In Pag. 43 identity oduc --> identity oducn ?

- In pag 45, default forward --> default "forward" ?

- Pag 58 ihe --> the  , ranage --> range

Questions:

- In te-admin-status (pag. 12) is the status "unknown" not applicable in here?

- In te-recovery-status (pag 17)  the status reversion-succeeded woud be not
necessary in this case, cause we have recovery-succeeded status available?

- In pag 25, the lsp-state-type does not have a reference is that ok?, because
it is used as base for other parameters.

- In general, I see some parameters that do not have reference specified, it is
because the reference is already specified in the base, for example, pag 25,
objective-function-type has reference, but then of-minimize-load-path,
of-maximize-residual-bandwith do not have it. Is it because by default is the
one of the base, in this case RFC4657? or it is because they are defined in
this document? But for example lsp-protection-type is used as base and does not
have reference added.

-In pag. 34, in the description for protection-external-commands, should
include in the description the word "base", since it is used as base.

- In pag. 55 the description "RFC 3209 and others", should be added additional
rfcs, instead of "others"?

Thanks for this document,

Ines.

---

From: Tarek Saad <[email-masked]>
Date: Tue, 9 Apr 2019 00:23:15 +0000

Hi Ines,

Thank you very much for the detailed review and comments. 
We've uploaded new versions of the draft that will address them (latest version -08).
Diff that shows changes is @ https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht?url1=https://tools.ietf.org/id/draft-ietf-teas-yang-te-types-06.txt&url2=https://tools.ietf.org/id/draft-ietf-teas-yang-te-types-08.txt

Please see inline for further responses and do let us know if there are further comments.

On 4/2/19, 3:39 AM, "Teas on behalf of Ines Robles via Datatracker" <[email-masked] on behalf of [email-masked]> wrote:

    Reviewer: Ines Robles
    Review result: Has Nits

    

    Summary:

    
    
    Major issues: Not found
    
    Minor issues: Not found
    
    Nits/editorial comments:
    
    - NBMA could be expanded in pag 14  NBMA (Non-broadcast multiple-access
    network) or maybe added into Acronyms and Abbreviations section. Same for SF
    (pag 32), SD (Pag 33), APS (pag 35) and PM (pag 46)
[TS]: Added into acronyms, and expanded when necessary.
  	
[TS]: fixed.
    
[TS]: fixed.
    
[TS]: fixed.
    
    Questions:
    
    - In te-admin-status (pag. 12) is the status "unknown" not applicable in here?
[TS]: We think it's useful so to identify a status that is not explicitly defined. We have added it.

    - In te-recovery-status (pag 17)  the status reversion-succeeded woud be not
    necessary in this case, cause we have recovery-succeeded status available?
[TS]: We have added reversion-succeeded to explicitly indicate successful completion of reversion.

    - In pag 25, the lsp-state-type does not have a reference is that ok?, because
    it is used as base for other parameters.
[TS]: Yes, in general the intention was the derived from base indentities would be explicitly referenced.
    
[TS]: Yes, as mentioned earlier, identities derived would be explicitly referenced. That said, I went ahead and made sure missing references are added.
    
    -In pag. 34, in the description for protection-external-commands, should
    include in the description the word "base", since it is used as base.
[TS]: addressed as per suggestion.
    
    - In pag. 55 the description "RFC 3209 and others", should be added additional
    rfcs, instead of "others"?
[TS]: yes, replaced others by the intended RFCs.

Regards,
Tarek

    Ines.
    
    _______________________________________________
    Teas mailing list
    [email-masked]
    https://www.ietf.org/mailman/listinfo/teas

---

From: Ines Robles <[email-masked]>
Date: Wed, 10 Apr 2019 19:22:32 +0300

Hi Tarek,

Thank you, I agree with your comments.

Best regards,

Ines.

On Tue, Apr 9, 2019 at 3:23 AM Tarek Saad <[email-masked]> wrote:

> Diff that shows changes is @
> https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht?url1=https://tools.ietf.org/id/draft-ietf-teas-yang-te-types-06.txt&url2=https://tools.ietf.org/id/draft-ietf-teas-yang-te-types-08.txt
>
> On 4/2/19, 3:39 AM, "Teas on behalf of Ines Robles via Datatracker" <
> [email-masked] on behalf of [email-masked]> wrote:
>
>     Reviewer: Ines Robles
>     Review result: Has Nits
>
> [TS]: Yes, as mentioned earlier, identities derived would be explicitly
> referenced. That said, I went ahead and made sure missing references are
> added.
>