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

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

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

> From: Martin Bjorklund, June 8, 2018 5:02 AM
> 
> Hi,
> 
> 
> "Eric Voit (evoit)" <[e-mail]> wrote:
> > Hi Martin,
> >
> > Updated file at:
> > https://github.com/netconf-wg/rfc5277bis/blob/master/draft-ietf-netcon
> > f-subscribed-notifications-13.txt
> >
> > > From: Martin Bjorklund, June 7, 2018 11:32 AM
> 
> [...]
> 
> > > o  Section 2.3
> > >
> > >   s/"QoS" feature/"qos" feature/  (twice)
> 
> You forgot this one.  The feature in the YANG model is "qos", not "QoS".

Change made.

> > > 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;
> 
> You may want to use yang:zero-based-counter64.

Change made
 
> >             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;
> 
> From the description below, you definately want yang:zero-based-counter64.

Change made

> >             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.";
> >           }
> 
> 
> Ok with this, but I would prefer more descriptive leaf names.
> event-records-sent and event-records-excluded perhaps.

Left it as "count-sent" and "count-excluded".  Reason is that the "count-sent" can be reused for yang-push notifications without casual users needing to read through all the descriptions to understand that a "push-update" notification is actually a form of event record.

> > > 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")'
> 
> You don't need the boolean(...):
> 
>   when 'not(../transport) or derived-from(...)';

Change made

Eric

> /martin
> 
> 
> 
> > 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
> >