On Thu, Mar 23, 2023 at 5:11 AM tom petch <ie...@btconnect.com> wrote:
> From: netmod <netmod-boun...@ietf.org> on behalf of Jürgen Schönwälder
> <jschoenwaelder@constructor.university>
> 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: +49 421 200 3587 Campus Ring 1 | 28759 Bremen | Germany
> Fax: +49 421 200 3103 <https://constructor.university/>
>
> _______________________________________________
> netmod mailing list
> netmod@ietf.org
> https://www.ietf.org/mailman/listinfo/netmod
>
> _______________________________________________
> netmod mailing list
> netmod@ietf.org
> https://www.ietf.org/mailman/listinfo/netmod
>
_______________________________________________
netmod mailing list
netmod@ietf.org
https://www.ietf.org/mailman/listinfo/netmod
