Re: Genart telechat review of draft-ietf-ippm-twamp-yang-11

[Mahesh Jethanandani <[e-mail]>]

Hi Pete,

> On Jun 19, 2018, at 9:02 PM, Pete Resnick <[e-mail]> wrote:
> 
> Reviewer: Pete Resnick
> Review result: Ready with Issues
> 
> 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 wait for direction from your
> document shepherd or AD before posting a new version of the draft.
> 
> For more information, please see the FAQ at
> 
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
> 
> Document: draft-ietf-ippm-twamp-yang-11
> Reviewer: Pete Resnick
> Review Date: 2018-06-19
> IETF LC End Date: 2018-04-27
> IESG Telechat date: 2018-06-21
> 
> Summary:
> 
> Ready with maybe Issues, but probably just Nits. Not my area of expertise by
> any means, but the document looks generally solid. Could definitely use a bit
> of copy editing.
> 
> Major issues:
> 
> None.
> 
> Minor issues:
> 
> I don't think there are any issues here. However, some of the things I've got
> as Nits in the below section could amount to actual issues if I've
> misunderstood what you meant. The editorial suggestions I give below should be
> fine if they are nits, but do make sure that I haven't identified a real issue.
> 
> Nits/editorial comments:
> 
> 3.1 - s/The test session name that MUST be identical/The test session name,
> which MUST be identical (Unless you mean something really weird that I don't
> think you mean. If you don't see the difference, then trust me, you mean
> "which", not "that”.)

You mean in Section 3.3. How about s/The test session name that MUST be identical with the/The test session name MUST be identical to the/?

> 
> 4.1 -
> 
> OLD
>   Specifically, mode-preference-chain lists the
>   mode and its corresponding priority, expressed as a 16-bit unsigned
>   integer, where zero is the highest priority and subsequent integers
>   increase by one.
> 
> This is a bit confusing. I think you mean:
> 
> NEW
>   Specifically, mode-preference-chain lists the mode and its
>   corresponding priority, expressed as a 16-bit unsigned integer.
>   Values for the priority start with zero, the highest priority, and
>   subsequent priority value increases by one.

I can see why this can be confusing. How about ...

NEW
   Specifically, mode-preference-chain lists the
   mode and its corresponding priority as a 16-bit unsigned
   integer. Values for the priority start with zero, the highest priority, and 
   decreasing priority value is indicated by every increase of value by one.

> 
> OLD
>   In turn, each ctrl-connection holds a list of test-session-request.
>   test-session-request holds information associated with the Control-
>   Client for this test session.
> 
> A bit awkward. I suggest:
> 
> NEW
>   In turn, each ctrl-connection holds a test-session-request list. Each
>   test-session-request holds information associated with the
>   Control-Client for this test session.

Ok.

> 
> OLD
>   The Control-Client is also responsible for scheduling TWAMP-Test
>   sessions so test-session-request holds information related to these
>   actions (e.g. pm-index, repeat-interval).
> 
> The word "so" in there is weird. Do you mean "therefore", or "such that", or
> something else? I just had a bit of trouble understanding what you meant.

We meant “therefore”. Will make the change.

> 
> 4.2 - In the penultimate paragraph, change "key-id" to either "The key-id" or
> "The KeyID”.

Will change it to “The key-id”.

> 
> Please note: I did not thoroughly review the YANG in section 5.2 or the
> examples in Section 6 or Appendix A. I gave them a quick run through, but did
> not check for complete consistency with the rest of the text. The below two
> items are simply things I happened to spot because I was looking at particular
> pieces of the module.
> 
> 5.2 -
> 
>           leaf priority {
>             type uint16;
>             description
>               "Indicates the Control-Client Mode preference priority
>                expressed as a 16-bit unsigned integer, where zero is
>                the highest priority and subsequent values
>                monotonically increasing.";
>           }
> 
> I am almost positive that you don't mean "monotonically increasing". I'm
> guessing you mean "increase by one”.

Will update this description to match the comment you made above or whatever we agree to.

> 
>              Depending on the Modes available in the TWAMP Server
>              Greeting message (see Fig. 2 of RFC 7717), the
>              this Control-Client MUST choose the highest priority
>              Mode from the configured mode-preference-chain list.";
> 
> Typo: "the this Control-Client”

Will fix it to say “the Control-Client”.

Thanks.

> 
> 

Mahesh Jethanandani
[e-mail]