Subject: [netconf] mbj review of draft-ietf-netconf-notification-capabilities-01

From: Martin Bjorklund <[email-masked]>
Date: Mon, 25 Mar 2019 21:56:37 +0100 (CET)

Hi,

Here are my (mostly minor) comments on
draft-ietf-netconf-notification-capabilities-01

o  Terminology

  1. The term that is used in th YP draft is "YANG-Push".  Please
     import that and use it consistently.  (I see at least "YangPush"
     and "Yang-Push").

  2. This document uses the terms "YANG server" and "Yang server".  I
     suggest you change this to "server", and import that from RFC
     8342.

o  Abstract

  The abstract mentions YANG Instance Data, but it doesn't mention
  run-time.  I think it should.

o  Section 1

  s/Rutime/Runtime/

o  Section 1

  s/protocol/management protocol/

  Should you really list HTTPS here?   + add references to NETCONF and
  RESTCONF.

o  Section 3

  Editorial: remove the text within the parentheses.

o  General

  We need to be able to specify on-change support per datastore.

  With support per datastore, it is not quite clear if
  notification-sent-for-config-default and
  notification-sent-for-state-default are needed, and/or how they
  should be interpreted.

o  General

  In YP, support for on-change is optional.   There is a feature
  called "on-change".  I think this document should state explicitly
  that this module is intended for servers that implement that
  feature.

o  Section 3.2

  Editorial:

    We no longer list the WG Chairs in the description.

    Since you use 2119 language in the model, please include the
    boilerplate in the module description.

    The module description doesn't contain the proper copyright text,

    The module should be formatted similar to the rest of the
    IETF-published modules.  I suggest you run

      pyang -f yang --keep-comments --yang-line-length 69 <FILE>

    and then ensure that any additions are consistent with that.

o  Section 3.2

  The top-level container is currently called
  "yangpush-notification-capabilities".

  This doesn't really match the naming in the YP document.

  I suggest it is called "datastore-subscription-capabilities", so
  that we use a word that can be related to the the nodes YP defines,
  such as in "establish-subscription" ... "datastore"
  "datastore-subtree-filter"

o  Section 3.2

  You have:

         units msec;

         units centiseconds;

  I think you probably should do s/msec/milliseconds/

o  Section 3.2

  minimum-dampening-period is optional.  It should be stated what it
  means if this leaf is not present.   (or should the default be 0?)

o  Section 3.2

  The choice update-period is not mandatory, and doesn't have a
  default.  What does it mean if this is not reported?

o  Section 3.2

  The leaf max-objects-per-update is not mandatory.  What does it mean
  if this is not reported?  It can have the value 0.  What does that
  mean?

  Perhaps make a union of uint32 0..max and the enum "infinite" and
  make that the default?  (of course "infinite" is not really
  correct...)

o  General

  I missed some examples, perhaps in an Appendix.

/martin

---

From: Balázs Lengyel <[email-masked]>
Date: Mon, 25 Mar 2019 23:40:56 +0000

Thanks for the comments, See below. Balazs

On 2019. 03. 25. 21:56, Martin
Bjorklund wrote:

Hi,

o Terminology

BALAZS: OK

BALAZS: OK

o Abstract

BALAZS: OK

o Section 1

s/Rutime/Runtime/

BALAZS: OK

o Section 1

BALAZS: OK

o Section 3

o General

BALAZS: The defaults will also be per
datastore. For datastores that do support them, they have no
meaning. E.g. state
data in intended. They could be removed formally with
when statements in the YANG, but IMHO its enough to state it in
text:

- default for state is anyway false

- For new datastores we would not know how to set up the when
condition

o General

BALAZS: OK, but some other capacity related
proerties are still valid.

o Section 3.2

Editorial:

BALAZS: OK

o Section 3.2

BALAZS: OK

o Section 3.2

You have:

units msec;

units centiseconds;

BALAZS: OK, probably change all to
centiseconds. Is there a general rule not to use the official
abbreviations?

o Section 3.2

BALAZS: If not present it means that the
system does not tell you what it is. No special meaning. I do not
see a reasonable minimum value (eventhough 0 would look strange).
IMHO it is the responsibility of the server to provide a
meaningful value. If it says zero, that's obviously crazy, just
ignore it. I don't thing we need to have rules, like: do not
provide stupid state data.

o Section 3.2

BALAZS: If not present it means that the
system does not tell you what it is.
No special meaning.

o Section 3.2

BALAZS: If not present it means that the
system does not tell you what it is. No special meaning. I do not
see a reasonable minimum value (eventhough 0 would look strange).
IMHO it is the responsibility of the server to provide a
meaningful value. If it says zero, that's obviously crazy, just
ignore it.

BALAZS: IMHO infinite is unreasonable. If the
server doesn't know the real limit, it should just not provide a
value.

o General

BALAZS: Will be provided.

_______________________________________________
netconf mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/netconf

--
Balazs Lengyel Ericsson Hungary Ltd.
Senior Specialist
Mobile: +36-70-330-7909 email: [email-masked]

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 26 Mar 2019 12:12:31 +0100 (CET)

Hi,

Pruning...

Balázs Lengyel <[email-masked]> wrote:
> On 2019. 03. 25. 21:56, Martin Bjorklund wrote:
> 
>     Hi,
> 

[...]

> BALAZS: OK, but some other capacity related proerties are still valid.

What other capacity related properties do you mean?

> BALAZS: OK, probably change all to centiseconds. Is there a general
> rule not to use the official abbreviations?

What is the official abbrevation?  According to wikipedia, the SI name
is "millisecond" and "centisecond", and the corresponding symbols are
"ms" and "cs".  I briefly scanned the SI spec, but didn't really come
to any conclusion...

My point was just that it is probably best to be consistent and not
use an abbreviation in one place and the full word in the other.

> BALAZS: If not present it means that the system does not tell you what
> it is. No special meaning. I do not see a reasonable minimum value
> (eventhough 0 would look strange). IMHO it is the responsibility of
> the server to provide a meaningful value. If it says zero, that's
> obviously crazy, just ignore it. I don't thing we need to have rules,
> like: do not provide stupid state data.

Note that 0 is the default in YP, so I don't know why you think it is
crazy.

I suggest you add default "0" to this.

> BALAZS: If not present it means that the system does not tell you what
> it is. No special meaning.

I think this should be clarified in the description.

> BALAZS: If not present it means that the system does not tell you what
> it is. No special meaning.
>
> I do not see a reasonable minimum value
> (eventhough 0 would look strange). IMHO it is the responsibility of
> the server to provide a meaningful value. If it says zero, that's
> obviously crazy, just ignore it.

I suggest you add a range "1..max" to the type.

> BALAZS: IMHO infinite is unreasonable. If the server doesn't know the
> real limit, it should just not provide a value.

Ok, makes sense.  I think this should be clarified in the description.

/martin

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Tue, 26 Mar 2019 12:44:23 +0000

> What other capacity related properties do you mean?

The latest draft allows minimum periodic reporting intervals to be set.

...
> Note that 0 is the default in YP, so I don't know why you think it is crazy.
> 

I agree.

> > I do not see a reasonable minimum value (eventhough 0 would look
> > strange). IMHO it is the responsibility of the server to provide a
> > meaningful value. If it says zero, that's obviously crazy, just ignore
> > it.
> 

Is max necessary?   I would assume that when this is not populated, it is  by default max (per below).

Eric

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 26 Mar 2019 14:01:02 +0100 (CET)

> The latest draft allows minimum periodic reporting intervals to be set.

Ok.  Then perhaps the nodes that are specific to on-change should have
a if-feature "yp:on-change".

> it is  by default max (per below).

Currently the type is just a uint32.  I think we should limit the
range so that 0 is not a valid value.  The way to do that in YANG is:

  type uint32 {
    range "1..max";
  }

/martin