Re: [netconf] Shepherd review of draft-ietf-netconf-notification-capabilities

[Benoit <[e-mail]>]

Hi Kent,

Sorry it took us so long to update the draft.
Below are our answer. We believe we have addressed all comments.

Title:
- it would be best for the title to not use the “YANG-Push” term before 
it’s been imported.
How about this?
OLD: YANG Modules for describing System Capabilities and Yang-Push 
Notification Capabilities
NEW: YANG Modules describing Capabilities for Systems and Notifications 
for Datastore Updates
NEW2: YANG Modules describing Capabilities for Systems and Datastore 
Updates Notifications
BALAZS: OK, used New2

Be sure to update any change in the title in the YANG modules as well!
BALAZS: OK, used New2

Note: https://en.wikipedia.org/wiki/YANG#Standards-track_data_models has
a preference for "Data Models” over “Modules”, but I left that part alone
since it’s not unheard of either...


Abbreviated title:
- "YANG-Push Notification Capabilities” is not an accurate description...
BALAZS: OK, changed to System and Notification Capabilities



Abstract:

- module names should be in quotes when introduced
BALAZS: OK

- s/This document defines two YANG modules:/This document defines two 
YANG modules,"ietf-system-capabilities” and 
"ietf-notification-capabilities”.
BALAZS: OK

- s/YANG Instance Data/YANG Instance Data 
(I-D.ietf-netmod-yang-instance-file-format)/
BALAZS: OK

- s/"Subscription to YANG Datastores" (YANG-Push)/Subscription
to YANG Notifications for Datastore Updates (RFC 8641)/
BALAZS: OK

- Actually (in ref to the above two comments), the sentence structure in 
p2 seems
backwards. Better would be to say "The module can be used to report 
capability
information from the server at run-time or implementation-time, per 
I-D.ietf-netmod-yang-instance-file-format.”
BENOIT: idnits complains when we have references in the abstract. 
Removed the references

Section 1:

- Why isn’t the “Introduction” Section ‘1’ as is common in RFCs?
BALAZS: OK, changed order of sections

- Each term should be in double-quotes when first introduced, or use 
hanging text like other RFCs do.
BALAZS: OK

- don’t capitalize terms unless they appear at the beginning of a sentence,
unless they’re capitalized throughout the entire document consistently!
Which they’re not here.v Personally, I can never capitalize them 
consistently, so I just normal sentence-capitalization rules…
BALAZS: OK

- Neither "Run-time information” nor "Implementation-time information”
constitute terms. Seriously, no one is going to care this document for
those terms. Also not that "Run-time information” is only used once, as
"Implementation-time information” would be as well if the text in the two
back-to-back paragraphs were rearranged slightly.
BENOIT: in the context of this draft, we have 2 separate scenarios 
(run-time information and implementation-time information). Instead of 
re-explaining them in different sections and risking that the terms are 
misunderstood, we believe it's best to define them. We seem to recall 
that we also received that feedback from the WG (during LC, possibly)

- "On-change notification capability” is not a term and it’s not used 
anywhere
in the document
BALAZS: OK, removed


Section 2:

- The first sentence uses the word “capabilities” without explanation (and
it’s not a term). Given that this document defines the “capabilities”, I’m
expecting a term to be defined for it. S3 provides examples, but it seems
too far away…thoughts?

Maybe S1 and S2 could be collapsed:

OLD:
Systems implementing a server and/or a publisher often have
capabilities that are not defined by the YANG model itself. There is
a need to publish this capability information as it is part of the
contract between the server and client.

NEW:
Servers and/or a publishers often have capabilities, values describing
operational behavior, that need to be conveyed to clients, which is 
enabled by the YANG modules described in this document. <blank line>

BENOIT: DONE

- the bullet list should be indented (i.e., put inside another list with 
symbols="empty”)
BENOIT: please advice, the following doesn't work
         <list style="empty">
             <list style="symbols">
                   <t>vendor independent</t>
                   <t>machine readable</t>
                   <t>available in identical format both at 
implementation-time and run-time</t>
             </list>
         </list>

Section 2.1:
- remove all instances of “we” throughout the document. s/we/this document/?
BENOIT: done

- why is there a “2.1” if there isn’t at least a “2.2”? (Is this meant 
to actually be a section 3?)
BENOIT: corrected

- the bullet lists should be indented (i.e., put inside another list 
with symbols="empty”)
BENOIT: same remark;

- P4 (the paragraph beginning "Publishers have limitations”) doesn’t 
seem to say anything
concrete (i.e., relavent to the draft). Actually, the same can be said 
for P5.
BENOIT: OLD
    Publishers have limitations in how many update notifications and how
    many datastore node updates they can send out in a certain time-
    period.

    Publishers might not support periodic subscriptions to all
    datastores.

NEW: Publishers might have some capabilities (or limitations) to document.
    For example, how many update notifications and how many datastore 
node updates
    they can send out in a certain time-period. Other publishers might
    not support periodic subscriptions to all datastores.

- P6 and P7 appear to be (unnecessary) elaborations of bullet-point #2 
above.
BENOIT: combined the paragraphs to make a longer series of 
capabilities/limitations to document. The entire section is:
           Publishers might have some capabilities (or limitations) to 
document.
           For example, how many update notifications and how many 
datastore node
           updates they can send out in a certain time-period. Other 
publishers might
           not support periodic subscriptions to all datastores. In some 
cases, a publisher supporting
           on-change notifications will not
           be able to push updates for some object types on-change. 
Reasons for
           this might be that the value of the datastore node changes 
frequently
           (e.g., in-octets counter), that small object changes are
           frequent and irrelevant to the receiver (e.g., a temperature 
gauge changing 0.1
           degrees within a predetermined and acceptable range), or that 
the
           notification for a particular object. In all those cases,
           it will be important for subscriber applications to have
           a way to identify which objects on-change notifications are
           supported and for which ones not.


- s/Clients of a server, subscribers to a publisher/Clients of a server/ 
(note: that subscribers are clients)
BENOIT: NEW

    Clients of a server (and subscribers to a publisher, as subscribers
    are also clients) need a method to gather capability information.

- the last 2-3 paragraphs seem to go into the weeds wrt the focus of 
this draft…and the whole
Implementation-time vs Runtime tradeoff is repeated in Section 3. 
Honestly, I don’t think
this draft needs to discuss this at all but, if you must, factor it out 
into a separate section
and keep it short, e.g., as in Section 3.
BENOIT: Actually, we believe that we must address the 
implementation-time vs runtime topics, as the two provides the 
background for two distinct use cases, both solved with the solution in 
this draft.
However, we agreed with you that the section order was not right. We 
introduced the more generic text in the introduction and kept the 
specifiations for subsequent sections.


Section 3:
- why doesn’t this section come before Section 2.1 (which maybe should 
actually be a Section 3)?

BENOIT: corrected
- System-level capabilities is the “base” model, in hence should be 
introduced first…
BENOIT: corrected

- actually, after reading the entirely of Section 3, I no understand the 
purpose of Section 2.1
BENOIT: yes, there is some history behind the draft. First it was about 
the YANG push capabilities only. Then, as we foresaw that other type of 
capabilities would be needed, we added the generic system capabilities 
information. Reviewing the draft, the history still transpired through 
the section order. Hopefully, this is solved now.

- recommend 1) extracting the middle part out into a new “providing 
data” section where the
Implementation-time vs Runtime options are discussed and 2) moving the 
remainder to End of the Introduction section…

- s/The module ietf-system-capabilities/The module 
"ietf-system-capabilities"/
BALAZS: OK .
- s/is defined to provide a structure/provides a structure/
- s/any YANG related system capability/YANG-defined capabilities/
BENOIT: It might be best to keep it aligned with the abstract.
The last two points become:

    The module "ietf-system-capabilities" provides a placeholder
    structure to be used to specify any YANG related system capability.


- s/The module ietf-notification-capabilities/The module 
"ietf-notification-capabilities”/
BALAZS: OK
- s/"Subscription to YANG Datastores" (YANG-Push)/Subscription to YANG 
Notifications for Datastore Updates [RFC8641]/
BENOIT: done
- Re: "on system/publisher level” - why is “publisher” mentioned in 
Section 4 at all?
BENOIT: removed


Section 4:
- s/The module ietf-system-capabilities/The module 
"ietf-system-capabilities"/
BENOIT: DONE
- s/is defined to provide a structure/provides a structure/
BENOIT: DONE
- s/that can be used to specify/that can be used to discover (as 
read-only operational state)/
BENOIT: DONE
- s/any YANG related system capability/YANG-defined capabilities/
BENOIT: see above
- P3 should be P2 (like they are in the YANG module)
BENOIT: right
- the presence of P2 is confusing, if “this module itself does not 
contain any capabilities”
BENOIT: See below.

- I think you want to say something like the “structure” provides 
augmentation points
for capabilities to be defined at different levels: a) system, when foo, 
b) datastore,
when bar, and c) specific data nodes, the baz. (where foo/bar/baz 
describe the
augmentation points used) - right?
BENOIT: right.
     This module itself does not contain any capabilities; it provides 
augmentation points
     for capabilities to be defined in subsequent YANG modules.
- s/config=false/“config false”/
BENOIT: DONE


Section 4.2: ([e-mail])

- should the “description” statement for "import ietf-yang-library”
1) s/a revision/a backwards-compatable revision/ and 2) state that
the module must be implemented? (Better, see my other comment
regarding the YANG Doctor;s comment)
BENOIT: Done

- s/RFC RFC8525/RFC 8525/
BENOIT: Done

- OLD:
This module specifies a module intended to contain system
capabilities. System capabilities may include capabilities of a
NETCONF or RESTCONF server or a notification publisher.
NEW:
This module specifies a structure to specify system
capabilities for s server or a publisher.
BENOIT: OK

- s/capabilities it only/capabilities, it only/
BENOIT: OK

- s/should be augmented in/are augmented in/
BENOIT: OK
- be sure there is a blank line between paragraphs and numbered
list items, and/or list items are indented (this comment applies to
*all* “description” statements)
BENOIT: OK

- list item (4) is missing punctuation and the first “sentence” doesn’t
form a sentence.
BENOIT: OK

- "RFC XXX: YANG-Push Notification Capabilities” has both an incorrect 
number of X’s as well as incorrect document title.
BENOIT: OK

- rather than have the “description” statement "// augmentation
point for system level capabilities”, why not have an empty
“choice” statement with the same description statement?
BALAZS: choice would not be a good solution because it would only allow 
one set of capabilities, while our goal is to allow different capability 
set side by side.

- re: "Only individual datastores”, is “individual” the right word? 
Would “specific” or “leaf” be better?
BALAZS: we used "specific"

- rather than have the “description” statement "// augmentation
augmentation point for datastore or data node level capabilities”,
why not have an empty “choice” statement with the same
description statement?
BALAZS: see above.


- is the comment "The special value '/' denotes all data nodes in the
datastore.” should state if it’s a CLR or simply consistent with XPath 
1.0 expressions.
BALAZS: we are consistent witjh the NACM RFC, which says:
                     The special value '/' refers to all possible
                     datastore contents.";

- this command detected an issue: pyang --ietf 
ietf-system-capabilities\@2020-03-23.yang.
(I’m unsure why, exactly, as I see the keywords block in the 
module…suggest copying
text directly from `pyang --ietf-help`.


Section 5
- s/The YANG module ietf-notification-capabilities is defined to 
provide/The YANG module "ietf-notification-capabilities” provides/
BENOIT: done


Section 5.2: ([e-mail])
- P1 doesn’t specify all the normative imports
BENOIT: done

- be sure there is a blank line between paragraphs and numbered
list items, and/or list items are indented (this comment applies to
*all* “description” statements)
BENOIT: done

- s/If, different/If different/
BENOIT: done

- "RFC XXX: YANG-Push Notification Capabilities” has both an incorrect 
number of X’s as well as incorrect document title.
BENOIT: done

- s/config=true/‘config true’/
BENOIT: done

- s/config=false/‘config false’/
BENOIT: done

- "supported for none” - nonsensical, rephrase.
BENOIT:
               "Type for defining whether 'on-change' or
            'periodic' notifications are supported 'config false'
            data nodes, 'config true' date nodes, no data nodes,
            or all data nodes.

- s/ or all data nodes/, or all data nodes/ (also try to fix the 
sentence so it makes sense)
- s/ or a set of/, or a set of/
- s/update-too-big/'update-too-big’/
BENOIT: done

- many of the “reference” statements are being misused. Move content
information into the “description” and only set the reference per 
https://tools.ietf.org/html/rfc7950#section-7.21.4
BENOIT: all corrected

- s/and the subtree below them/including any subtrees that may exist 
below them/
BENOIT: done

- s/on-change notifications/'on-change' notifications/
BENOIT: done

- this command detected a number of issues: pyang -f yang 
--keep-comments --yang-line-length 69 
ietf-notification-capabilities\@2020-03-23.yang > tmp; diff 
ietf-notification-capabilities\@2020-03-23.yang tmp. (whitespace at end 
of lines)



Section 7.2 (really 5.2, but I didn’t catch it until here)

- the “inc” prefix seems odd, perhaps wrong, how about “notc” (to match 
“sysc”)?:
BENOIT: You're right, changed

Appendix A

- the example is wrapped in the <CODE BEGINS>/<CODE ENDS>, but now that 
`rfcstrip` has the “-a” parameter, these are no longer needed - agreed? 
Please move the filename augment to the “name” attribute in the 
<artwork> element for both examples.
BALAZS: OK

- ideally there are subsections (i.e., A.1 and A.2) delineating each example
BENOIT: done

- s/config=false/“config false”/
BENOIT: done
- please reorder the examples so that the system-capability examples 
precedes the notification-capability example
BENOIT: done

- in both examples:
- s/following instance-data/following instance data/ (or "instance data 
set”?)
BENOIT: done

^-- also add a reference to I-D.ietf-netmod-yang-instance-file-format
- please remove the "<!-- revision date, contact, etc. —>” comment
BENOIT: done
- s/config=false/“config false”/
BALAZS: OK
- s/config=true/"config true”/
BALAZS: OK
- consider putting quotes around words like “on-change” and “periodic”
BENOIT: Done
- s/only 2/only two/
BENOIT: Done


Other:

- nowhere in the document is there a request to the RFC Editor to set 
the “XXXX”, “YYYY”, and “YYY" strings. Perhaps other strings need to be 
converted as well...
BENOIT: done

- through draft (in both body and YANG modules>, bracket datastore names 
with
angle brackets (e.g., s/running/<running>/) wherever the datastore name 
is NOT
followed by word “datastore”. When the datastore name is NOT followed by the
word “datastore", the datastore name should be in quotes, where it makes 
sense
to do so. BTW, I note the none of the datastore names are imported as 
terms or
otherwise introduced where first used...
Benoit: Done

- the YANG Doctor review suggested adding comments regarding
if imports needed to be implemented. I see such comments were
added in -06 but they’ve subsequently been removed. I strongly
second Lada’s suggestion to include a minimal YANG Library
instance somewhere, not only to document which modules need to be 
implemented, but also to document to totality of them, as I
was unpleasantly surprised by how many there were (12 in total!)
BALAZS: OK, Included for the 3 imports that need implemented. They were 
removed due to comments, but I like the sentences.


Kent // shepherd




_______________________________________________
netconf mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/netconf
>
>
> _______________________________________________
> netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf