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

[Martin Bjorklund <[e-mail]>]

Hi,


"Eric Voit (evoit)" <[e-mail]> wrote:
> 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

[...]

> > o  Section 2.3
> > 
> >   s/"QoS" feature/"qos" feature/  (twice)

You forgot this one.  The feature in the YANG model is "qos", not
"QoS".


> > 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.

>             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.

>             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.


> > 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(...)';



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