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

[Mahesh Jethanandani <[e-mail]>]

Hi Juergen,

Reviving this thread to identify next steps for the document. Where are we with publishing a -16 version of the draft?

I do not see objections to most of your suggestions on the changes you recommend. For milli vs micro part of the discussion, could we have both?

Separately, on the other thread on for changes to IPv6 zone definition, which I will respond to, can you summarize what the consensus is, so we can try to close on the thread.

Thanks


> On Mar 23, 2023, at 11:29 AM, Andy Bierman <[e-mail]> wrote:
> 
> 
> 
> On Thu, Mar 23, 2023 at 5:11 AM tom petch <[e-mail] <mailto:[e-mail]>> wrote:
> From: netmod <[e-mail] <mailto:[e-mail]>> on behalf of Jürgen Schönwälder <[e-mail]>
> Sent: 23 March 2023 11:13
> 
> On Wed, Mar 22, 2023 at 01:31:43PM +0000, Rob Wilton (rwilton) wrote:
> > 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.
> 
> Thanks for your review. See responses inline.
> 
> > 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'.
> 
> Earlier versions had just 'date' and 'time' and both included a zone
> offset. We then also got 'date-no-zone' and 'time-no-zone' and all was
> kind of nice and consistent. Then the IP address debate kicked in and
> finally some people made the point that we should be always explicit
> (but then you can't encode all semantics in a name anyway). So this is
> how we got to the names we have now. For me personally, 'date' and
> 'date-no-zone' and 'time' and 'time-no-zone' was just fine. The
> '-offset' came in as a way to future proof definitions since in some
> contexts you may want to indicate the timezone not with a fixed offset
> but with a timezone name like 'Europe/Berlin' that is then resolved
> using more complex rules to a specific offset.
> 
> I personally would be happy to change date-with-zone-offset back to
> date and time-with-zone-offset back to time and to deal with the
> future in the future...
> 
> <tp>
> 
> Me too.  I think that the fashion for incorporating ever more semantics into an identifier is a misunderstanding of what an identifier is for ie to identify.
> 
> 
> Me three.
> IMO simple names like 'date' and 'time' and 'date-and-time' are easier to understand.
> Optional fields are different than mandatory fields.
> If a data type has some mandatory field, then it may need to have its own typedef and name.
> 
> 
> 
> Tom Petch
> 
> Andy
>  
> 
> > (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.
> 
> I am happy to add. "It includes and optional time zone offset." so
> that it says:
> 
>      "The date type represents a time-interval of the length
>       of a day, i.e., 24 hours. It includes and optional time
>       zone offset.
> 
> With the current naming scheme, we would have to
> s/date/date-with-zone-offset/ everywhere in the description.
> That will look pretty ugly. [sarcasm on] Perhaps we should
> call it 'date-with-optional-zone-offset' and then the additional
> sentence is not needed anymore. [sarcasm off]
> 
> > This same comment also applies to 'time-with-zone-offset'.
> 
> Yes.
> 
> > (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'.
> 
> This would require to copy quite some text and then this cloned text
> needs to be kept consistent in the future. I do not think this is a
> good idea. Implementations can take shortcuts if this is found to be
> time critical (but that might also be very implementation specific).
> My preference is to not change this.
> 
> > (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.
> 
> For time periods with lower resolution, the details start to matter,
> (see microseconds32 on the extreme end) and so I ended up using the
> same notation and precision for all types. I think this is generally
> the right thing to do, being always precise is better than arbitrarily
> dropping precision. If someone has ideas for a better notation, I am
> open for that. Perhaps adding ", where hh:mm:ss stands for hours,
> minutes and seconds" would already do it?:
> 
>          The maximum time period that can be expressed is in the
>          range [-89478485 days 08:00:00 to 89478485 days 07:00:00],
>          where hh:mm:ss stands for hours, minutes and seconds."
> 
> > (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.
> 
> I ones had unsigned versions of these types. If we also add unsigned
> types, we end up with nine additional types, we would get something
> like:
> 
> hours-int32
> hours-uint32
> minutes-int32
> minutes-uint32
> seconds-int32
> seconds-uint32
> ...
> nanoseconds-int64
> nanoseconds-uint64
> 
> Well, perhaps this is the right thing to do, people can then pick what
> is most appropriate for their use case.
> 
> > (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.
> >
> 
> So far nobody asked for it. On POSIX systems (I think POSIX.1-2001 and
> later), you usually have system APIs that can go into microsecond
> resolution:
> 
>            struct timeval {
>                time_t      tv_sec;     /* seconds */
>                suseconds_t tv_usec;    /* microseconds */
>            };
> 
> But if there is a use case for milliseconds64, I can easily add it.
> well milliseconds-int64 and milliseconds-uint64, depending on the
> resolution of your previous point.
> 
> > 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'
> 
> Changed.
> 
> /js
> 
> --
> Jürgen Schönwälder              Constructor University Bremen gGmbH
> Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
> Fax:   [phone]         <https://constructor.university/ <https://constructor.university/>>
> 
> _______________________________________________
> netmod mailing list
> [e-mail] <mailto:[e-mail]>
> https://www.ietf.org/mailman/listinfo/netmod <https://www.ietf.org/mailman/listinfo/netmod>
> 
> _______________________________________________
> netmod mailing list
> [e-mail] <mailto:[e-mail]>
> https://www.ietf.org/mailman/listinfo/netmod <https://www.ietf.org/mailman/listinfo/netmod>
> _______________________________________________
> netmod mailing list
> [e-mail] <mailto:[e-mail]>
> https://www.ietf.org/mailman/listinfo/netmod <https://www.ietf.org/mailman/listinfo/netmod>

Mahesh Jethanandani
[e-mail]