[netmod] AD review of draft-ietf-netmod-rfc6991-bis-15

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Jürgen,

Thanks for the draft.  Please see my AD review comments below, except for a couple of comments related to the change to ipv6-address definition that I've spun into a separate thread so that I can include the interested parties of draft-ietf-6man-rfc6874bis into the discussion.


Moderate level comments:

(1) p 13, sec 3.  Core YANG Types

     typedef date-with-zone-offset {

Why don't we just call this 'date' rather than 'date-with-zone-offset', particularly because the zone information is optional?  Intuitively, from the name of this type, I would have expected that zone information as being required rather than being optional.

I also note that the current naming convention of this type seems somewhat inconsistent from "date-no-zone", since one of them includes "offset" and the other does not.

This same comment also applies to 'time-with-zone-offset'.


(2) p 27, sec 4.  Internet Protocol Suite Types

I've moved this comment to a separate thread.


(3) p 28, sec 4.  Internet Protocol Suite Types

I've moved this comment to a separate thread.


Minor level comments:

(4) p 13, sec 3.  Core YANG Types

       description
        "The date type represents a time-interval of the length
         of a day, i.e., 24 hours.

I think that it might be helpful if the first part of the description stated that the type optionally includes the zone offset, particularly to differentiate from the type that excludes it.

This same comment also applies to 'time-with-zone-offset'.


(5) p 14, sec 3.  Core YANG Types

       type date-with-zone-offset {
         pattern '[0-9]{4}-(1[0-2]|0[1-9])-(0[1-9]|[1-2][0-9]|3[0-1])';
       }

Although I can understand why it is modelled this way, i.e., to make the relationship between the types clear, there is likely to be a small performance overhead of modelling it this way, where this regex for this type is a strict subset of date-with-zone-offset.  I wonder whether it would be cleaner to just define this type as an equivalent top-level type to date-with-zone-offset, both in the definition and description rather than as a derived type?

This same comment also applies to 'time-no-zone'.


(6) p 15, sec 3.  Core YANG Types

         The maximum time period that can be expressed is in the
         range [-89478485 days 08:00:00 to 89478485 days 07:00:00].

I found this notation slightly confusing.  When I originally saw it, I assumed that it is talking about time zones, and it only really made sense when comparing it to the other periods.

I wasn't sure whether the specific details are that important, and whether defining it as -89478485 days to 89478485 days, might be both sufficient and easier to read.

E.g.,
         The maximum time period that can be expressed is in the
         range [-89478485to 89478485] days .

If changed, this same comment applies to the other period types as well.


(7) p 15, sec 3.  Core YANG Types

         This type should be range restricted in situations
         where only non-negative time periods are desirable,
         (i.e., range '0..max').";

Isn't this going to be the common mainline case for network configuration?  I.e., I presume that most cases where periods are intervals are going to be reported will be positive.  Hence, it might be helpful to have a separate set of types defined for the positive only cases.

This same comment applies to the other period types.


(8) p 16, sec 3.  Core YANG Types

     typedef milliseconds32 {

I was slightly surprised that we don't have a milliseconds64, e.g., the default timestamp in Java is given as an int64 in milliseconds.



Nit level comments:

(9) p 21, sec 3.  Core YANG Types

          7950. An earlier version of this definition did exclude

I suggest 'did exclude' -> 'excluded'

Regards,
Rob