Re: [Last-Call] Secdir last call review of draft-ietf-netconf-notification-capabilities-17

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Barry,

Thanks for your insightful review.
All remarks improve (the reading of) the specifications.
See inline for some some specific remarks.

On 10/1/2021 5:15 PM, Barry Leiba via Datatracker wrote:
Reviewer: Barry Leiba
Review result: Has Nits

Well written and easy to read; thanks.  I only have some very minor editorial
suggestions that I ask you to consider:

— Section 1 —

    Many such capabilities are
    specific to either the complete system, individual YANG datastores
    [RFC8342], specific parts of the YANG schema, or even individual data
    nodes.

Nit: “either” is correctly used for two items (“either A or B”).  For the four
items here, you might just eliminate the word “either”, as it’s not really
needed.

    A NMS implementation that wants to
    support notifications, needs the information about a system's
    capability to send "on-change" notifications.

I often find that I have to read this sort of thing (“A needs B to do C”) twice
to determine whether you mean that A requires that B do C, or that A needs B so
that A can do C — it’s ambiguous, so it requires extra analysis by the reader.
I suggest the following (which also eliminates the personification of NMS):

NEW
    An NMS implementation that supports
    notifications needs the information about a system's
    capability so it can send "on-change" notifications.
END

— Section 2 —

    This allows a user to
    discover capabilities both at implementation-time and run-time.

Nit: The “at” is factored wrong with respect to “both”. Either “both at
implementation time and at run time” or “at both implementation time and run
time”.  In either case, no hyphens here, as they’re not compound modifiers.

       The file MUST be
       available already at implementation-time retrievable in a way that
       does not depend on a live network node.

Nit: No hyphen (again, not a modifier), and it needs a comma after it:
“implementation time,”

       For the run-time use-case

Nit: Here, “run-time” is a modifier and needs the hyphen, but “use case” is a
noun and does not.

       (implementing the publisher) during run-time.  Implementations
       that support changing these capabilities at run-time SHOULD

Nit: No hyphens in “run time” for these two (nouns, not modifiers).

— Section 3 —

    A specific case is the need to specify capabilities is the YANG-Push
    functionality.

I’m not sure of the right fix for this, but the two instances of “is” can’t
both be right.  Maybe the first should be “of”?

A specific case is the need to specify capabilities in the YANG-Push
   functionality.


    As defined in [RFC8641] a publisher may allow
    subscribers to subscribe to updates from a datastore and subsequently
    push such update notifications to the receiver.

It’s unclear who is pushing: it looks like it could be the subscribers.  Maybe
clarify this way?:

NEW
    As defined in [RFC8641] a publisher may allow
    subscribers to subscribe to updates from a datastore and will
    subsequently push such update notifications to the subscriber.
END
Yes to the above.

    unless the subscriber has some means to
    identify which objects "on-change" notifications are supported.

Missing word: “are supported for.”

— Section 4 —

    It SHOULD be used by other modules to augment-in specific
    capability information.

The term “augment-in” is not one I’m familiar with.  If it’s common in YANG,
that’s fine.  If not, maybe rephrase?

   It SHOULD be used by other modules to augment in specific
   capability information.



    data is considered as if it was part
    of the running datastore.

Ultra-nit: “as if it were part”: subjunctive mood is needed after “as if”.


.
Thanks again.

Regards, Benoit

--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call




[Index of Archives]     [IETF Annoucements]     [IETF]     [IP Storage]     [Yosemite News]     [Linux SCTP]     [Linux Newbies]     [Mhonarc]     [Fedora Users]

  Powered by Linux