Subject: [netconf] Benjamin Kaduk's No Objection on draft-ietf-netconf-netconf-event-notifications-20: (with COMMENT)

From: Benjamin Kaduk via Datatracker <[email-masked]>
Date: Wed, 15 May 2019 11:04:16 -0700

Benjamin Kaduk has entered the following ballot position for
draft-ietf-netconf-netconf-event-notifications-20: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-netconf-netconf-event-notifications/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Section 6

   Notification messages transported over the NETCONF protocol MUST be
   encoded in a <notification> message as defined within [RFC5277],
   Section 4.  And per [RFC5277]'s "eventTime" object definition, the
   "eventTime" populated with the event occurrence time.

nit: I think the last sentence is actually a sentence fragment.

Section 7

This "Either it will correspond to [...] Or this 'error-tag' will
correspond to [...]" seems to preclude future extensions; do we want to
add some weakening language like "for the mechanisms specified in this
document"?

                                                  The specific identity
      to use depends on the RPC for which the error occurred.  Each
      error identity will be inserted as the "error-app-tag" following
      the form <modulename>:<identityname>.  An example of such as valid
      encoding would be "ietf-subscribed-notifications:no-such-
      subscription".  Viable errors for different RPCs are as follows:

            RPC                     use base identity
            ----------------------  ----------------------------
            establish-subscription  establish-subscription-error
            modify-subscription     modify-subscription-error
            delete-subscription     delete-subscription-error
            kill-subscription       delete-subscription-error
            resync-subscription     resync-subscription-error

This is probably just my lack of familiarity with the protocol, but the
text doesn't do much to indicate how the "base identity" concept in the
table corresponds to the "<modulename>:<identityname>" syntax or the
specific example given.  I think that this just means that the
<identityname> must be of the base type or derived from it, so maybe
"derive from" or "have" instead of "use" in the table heading would be
more clear.

      The yang-data included within "error-info" SHOULD NOT include the
      optional leaf "error-reason", as such a leaf would be redundant
      with information that is already placed within the
      "error-app-tag".

I'm not sure where this "error-reason" leaf is defined -- I don't  see
it in any of subscribed-notifications, yang-push, or RFC 6241.

Section 8

                                                                     The
   publisher MAY also suspend or terminate a subset of the active
   subscriptions on that NETCONF session.

I'd suggest saying/repeating why the publisher might do this, i.e., "MAY
also suspend or terminate [...], in order to reclaim resources and
preserve normal operation for the other subscriptions."

Appendix A.2

I'd suggest adding a note that the "id" values of 22, 23, and 39 are
just examples, and that actual values may not be small integers (akin to
my comment on the RESTCONF document).

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Wed, 15 May 2019 19:01:58 +0000

Thanks for the review Benjamin.  Some thoughts in-line.  I will post the corresponding update soon.

> From: Benjamin Kaduk, May 15, 2019 2:04 PM
> 
> When responding, please keep the subject line intact and reply to all email
> addresses included in the To and CC lines. (Feel free to cut this introductory
> paragraph, however.)
> 
> 

This reads fine with me.  Several on-line grammar checks give it a pass, but I am not sure how much I trust those tools.  So I will leave as is.

> Section 7
> 
> This "Either it will correspond to [...] Or this 'error-tag' will correspond to [...]"
> seems to preclude future extensions; do we want to add some weakening
> language like "for the mechanisms specified in this document"?

Works for me, now says...

For the mechanisms specified in this document, this "error-tag" will come from one of two places....
 
> This is probably just my lack of familiarity with the protocol, but the text doesn't
> do much to indicate how the "base identity" concept in the table corresponds to
> the "<modulename>:<identityname>" syntax or the specific example given.  I
> think that this just means that the <identityname> must be of the base type or
> derived from it, so maybe "derive from" or "have" instead of "use" in the table
> heading would be more clear.

Works for me.  Changed "use" to "have".
 
> I'm not sure where this "error-reason" leaf is defined -- I don't  see it in any of
> subscribed-notifications, yang-push, or RFC 6241.

Good catch.  It now reads "reason".  (We renamed to object "error-reason" to "reason" a few document iterations ago.)

> I'd suggest saying/repeating why the publisher might do this, i.e., "MAY also
> suspend or terminate [...], in order to reclaim resources and preserve normal
> operation for the other subscriptions."

Suggested text adopted.

> Appendix A.2
> 
> I'd suggest adding a note that the "id" values of 22, 23, and 39 are just examples,
> and that actual values may not be small integers (akin to my comment on the
> RESTCONF document).

Works for me.   I added the following at the top of the examples section:
"Additionally the subscription "id" values of 22, 23, and 39 used below are just examples. In production, the actual values of "id" may not be small integers."

Thanks again,
Eric

---

From: Randy Presuhn <[email-masked]>
Date: Wed, 15 May 2019 19:09:58 -0700

Hi -

On 5/15/2019 12:01 PM, Eric Voit (evoit) wrote:
...
>> From: Benjamin Kaduk, May 15, 2019 2:04 PM
...
> This reads fine with me.  Several on-line grammar checks give it a pass, but I am not sure how much I trust those tools.  So I will leave as is.

No, it really is a fragment.  Not only does it begin with a conjunction,
but it lacks a verb. If it were joined to the previous sentence, one
might infer a "MUST be" before "populated", but as a separate sentence
it no longer supports that interpretation.  If "MUST be populated" isn't
the intended interpretation here, then it's even more important to
turn that fragment into a real sentence.

Randy

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Thu, 16 May 2019 12:47:03 +0000

> From: Randy Presuhn, May 15, 2019 10:10 PM
> 
> Hi -
> 
> > This reads fine with me.  Several on-line grammar checks give it a pass, but I
> am not sure how much I trust those tools.  So I will leave as is.
> 
> No, it really is a fragment.  Not only does it begin with a conjunction, but it lacks
> a verb. If it were joined to the previous sentence, one might infer a "MUST be"
> before "populated", but as a separate sentence it no longer supports that
> interpretation.  If "MUST be populated" isn't the intended interpretation here,
> then it's even more important to turn that fragment into a real sentence.

Made it:  "And per [RFC5277]'s "eventTime" object definition, the "eventTime" is populated with the event occurrence time."

MUST is not required as previous references already enforce that requirement.

Eric

---

From: Randy Presuhn <[email-masked]>
Date: Thu, 16 May 2019 16:19:59 -0700

Hi -

> Made it:  "And per [RFC5277]'s "eventTime" object definition, the "eventTime" is populated with the event occurrence time."

Better, although I see no earthly reason for beginning this
sentence with "And".

Randy