Hi Mahesh,
thank you for the thorough review and the most detailed and thoughtful comments, suggestions. I'll work on the update in steps and will consult with you to address all issues you've pointed to.
Best regards,
Greg
On Sun, Apr 22, 2018 at 10:14 PM, Mahesh Jethanandani <mjethanandani@xxxxxxxxx> wrote:
Reviewer: Mahesh Jethanandani
Review result: Not Ready
Document reviewed: draft-ietf-stamp-yang-01
Status: Not ready.
Summary:
This document specifies the data model for implementations of Session-Sender
and Session-Reflector for Simple Two-way Active Measurement Protocol (STAMP)
mode using YANG.
Minor
- Please update references of RFC6020 with RFC7950, as this is a YANG 1.1 model.
- Please add a separate paragraph or section of notes to the RFC editor,
instead of scattering it across the document. RFC Editors appreciate not having
to look for instructions all over the document. - Also what does “ref to STAMP
draft” imply? This draft, some other draft, or RFC? - Please update references
to NMDA architecture document.
Major:
If the data model is for STAMP, how is that RFC (or I-D) not referenced
anywhere in the model? At the minimum most of data nodes should have references
to the sections that describe them in the STAMP RFC.
Please fix indentation issues in the model. I see use of 1 space, 2 spaces, 4
spaces and even 8 spaces in the model. It makes reading of the model very
difficult.
If statements carry into a subsequent line, consider indenting the second line.
s/stamp/STAMP/g
This document is NOT NMDA compliant. The whole idea of NMDA is that you do not
need a separate -state container. NDMA does not advocate having a separate
container with exactly the same nodes from config. An example is the nodes
‘packet-padding-size’ and ‘interval’, which are duplicated in both the config
and -state container. On the other hand ‘duplicate-packets’ and
‘reordered-packets’ make sense. Once the duplicate nodes have been removed,
rather than calling the container stamp-state, call it stamp-statistics, or
something else that does not imply the same meaning as pre-NMDA models
containers did.
Section 2.1
- The draft says it “describes all the parameters of the stamp data model.” I
do not think you are describing the parameters of the STAMP data model. What
you are describing is the block diagram (Fig 1) or the containers within the
model, not the individual parameters.
Section 2.1.1
Once the -state container has been updated/removed, update the second paragraph.
s/referenced by session-id STAMP/referenced session by the session-id of the
STAMP/
Section 3
I am not sure on the use of MAY. Is it that the use of IP and UDP port is
sufficient? If so, when it is okay? There is also a use of ‘may’ w.r.t. to
STAMP test sessions. How are the two mays related?
Finally, even if one was to use 5-tuple as a key, which includes a DSCP value
that can be remarked, how would one uniquely identify a session?
Section 3.1
Please add a informative reference to the Tree Diagram RFC.
Section 3.2
Please add references to any RFCs referenced in the model up front, before the
model.
Remove the comment “//namespace need to be assigned by IANA”. Instead update
the IANA section with the information of what namespace needs to be assigned.
Could a shorter prefix be used, e.g. “stamp”?
For reference to RFC 6991 and 8177, please add the title of those RFCs.
For contact, please add the name of individual authors instead of using a
mailing list, per Section 4.8 of rfc6087bis.
The description is very sparse. Consider adding more text. Also the document is
missing the IETF Copyright statement, per section 4.8 of rfc6087bis.
The revision statement should say something like “Initial version” instead of
00, as those numbers are not relevant once the document is published. Also
consider replacing “” for the reference statement with something like “RFC
XXXX: STAMP Data Model”, and have a note of the RFC editor to replace XXXX with
the assigned RFC number.
Consider updating descriptions for each leaf. A description “Packets received”
for ‘rcv-packets’ is not descriptive. Also add references to sections in the
STAMP RFC.
Why is there a need for feature statements for ‘session-sender’ and
‘session-reflector’. Would there be a case that a customer would configure a
‘session-reflector’ without a ‘session-sender’ or vice-versa?
What is ’type enable’? Did the authors mean ‘type boolean”?
Would it not be simpler to have ‘number-of-packets’ or ‘repeat’ as a choice
statement, where the choice is between a set number of test packets or
‘forever’? Same for the other union statements.
Consider using ‘derived-from’ or ‘derived-from-or-self’ in when statements,
instead of a string comparison.