Re: [Netconf] comments on draft-ietf-netconf-subscribed-notifications-12

["Eric Voit (evoit)" <[e-mail]>]

Hi Martin,

Updated file at:
https://github.com/netconf-wg/rfc5277bis/blob/master/draft-ietf-netconf-subscribed-notifications-13.txt 

> From: Martin Bjorklund, June 7, 2018 11:32 AM
> 
> Hi,
> 
> I checked draft-ietf-netconf-subscribed-notifications-12 (and to some extent
> draft-ietf-netconf-subscribed-notifications-13 in github) wrt my previous LC
> comments and new text.  Here are my additional
> comments:
> 
> 
> 
> o  Section 1.4
> 
>   This is a left-over from my previous review; you accepted the
>   proposed change, but it seems you forgot to change the text.
> 
>   OLD:
> 
>      o  the one way operation of [RFC5277], Section 4 is used.
> 
>   NEW:
> 
>      o  the <notification> message defined in [RFC5277] is still used.

In there now.  

> o  Section 1.4
> 
>   Also a left-over.
> 
>   OLD:
> 
>    o  a publisher MAY implement both the Notification Management Schema
>       and RPCs defined in [RFC5277] and this new document concurrently,
>       in order to support old clients.  However the use of both
>       alternatives on a single transport session is prohibited.
> 
>   NEW:
> 
>    o  a publisher MAY implement both the Notification Management Schema
>       and RPCs defined in [RFC5277] and this new document concurrently,
>       in order to support old clients.

Done

> o  Section 2.3
> 
>   s/"QoS" feature/"qos" feature/  (twice)
> 
> 
> o  Section 2.4.5
> 
>   OLD:
> 
>    "subscriptions/subscription/receivers/receiver/address".
> 
>   NEW:
> 
>    "/subscriptions/subscription/receivers/receiver/address".

Done

> o  Section 2.7.6
> 
>   OLD:
> 
>    This notification indicates that a configured subscription, which
>    includes a "stop-time", has successfully finished passing event
>    records upon the reaching of that time.
> 
>   NEW:
> 
>    This notification indicates that a subscription that includes a
>    "stop-time" has successfully finished passing event records upon
>    the reaching of that time.

Done

> o  Section 4
> 
>   It seems you changed the state names from "active" to "ACTIVE",
>   etc, i.e., all caps.  The 6087bis guidelines say that lower case
>   should be used.
>
>   My suggestion is to change to lower case in the model (which was in
>   -10 that was in LC), and change ACTIVE to "active" in the text
>   body.  (which I proposed in my LC review as well)

Done.  I changed *all* states in this and NETCONF-notif to lower case.  This includes the YANG file.

> o  Section 4
> 
>   You have:
> 
>           leaf pushed-notifications {
>             type yang:counter64;
>             config false;
>             description
>               "Operational data which provides the number of update
>                notification messages pushed to a receiver.";
>           }
> 
>   Is this a left-over from previous versions?  This document doesn't
>   define YANG push, and it doesn't define the term "update
>   notification message".
> 
>   Should it simply be notifications-sent, with description "A count of
>   the number of notifications sent to the receiver."?

Text wasn't left over.   But to make it as clean as possible, I changed to:

          leaf count-sent {
            type yang:counter64;
            config false;
            description
              "The number of event records sent to the receiver.  The 
              count is initialized when a dynamic subscription is 
              established, or when a configured subscription 
              transitions to the valid state.";
          } 
          leaf count-excluded {
            type yang:counter64;
            config false;
            description
              "The number of event records explicitly removed either 
              via an event stream filter or an access control filter so 
              that they are not passed to a receiver.  This count is 
              set to zero each time 'count-sent' is initialized.";
          }

> o  Section 4
> 
>     leaf encoding  should have "if-feature configured;"
> 
>     (it is dependent on ../transport, which has the if-feature)

The encoding can be set by RPC, so the issue is with the constraint.   (E.g., when HTTP is used, JSON and CBOR might be encodings.)

Maybe the way to address this (as can be seen in the draft) is to enhance the constraint to:

When  ' not(boolean(../transport))  or derived-from(../transport, "sn:configurable-encoding")'

And add an identity error to cover when an attempt is made establish a subscription with an unsupported encoding:

  identity encoding-not-supported {
    base establish-subscription-error;
    description
      "Unable to encode notification messages in the desired format.";
  }


>     Also, the description for this leaf is in -12:
> 
>         "The type of encoding for the subscribed data. If not
>         included as part of the RPC, the encoding MUST be set by the
>         publisher to be the encoding used by this RPC.";
> 
>    and in the not-yet-published -13 (from github):
> 
>         "The type of encoding for the subscribed data.   If not
>         included, the encoding used will be the default for one
>         encoding expected with a transport.";
> 
>    I can't parse from "the encoding used ...".

Made it:

"The type of encoding for notification messages.   For a dynamic subscription, if not included as part of an establish-subscription RPC, the encoding will be populated with the encoding used by that RPC.  For a configured subscription, if not explicitly configured the encoding with be the default encoding for an underlying transport.";

Thanks,
Eric

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