Document: draft-ietf-netmod-schedule-yang
Title: A Common YANG Data Model for Scheduling
Reviewer: Per Andersson
Review result: Has Nits
Reviewer: Per Andersson
Review Result: Has Nits
I have reviewed this document as part of the Operational directorate's
ongoing effort to review all IETF documents being processed by the IESG.
These comments were written with the intent of improving the operational
aspects of the IETF drafts. Comments that are not addressed in last call
may be included in AD reviews during the IESG review. Document editors
and WG chairs should treat these comments just like any other last call
comments.
Summary:
The document defines common types and groupings for scheduling purposes.
A YANG module is defined which includes a set of recurrence related
groupings with basic to advanced levels of representation. Groupings for
validating schedules and reporting schedule status are also defined.
I find the document easy to read and to understand, with a good
presentation of the proposed solution.
Major issues:
There are no major issues.
Minor issues:
* Section 3.3.6
For readers unaware of the yang:timeticks type definition a reference
to RFC 6991 for the period-timeticks using the yang:timeticks type
could be good.
* Section 3.3.8
Why is the iCalendar BYWEEKNO, BYMONTH, and WKST defined as
"byyearweek", "byyearmonth", and "workweek-start" respectively?
Suggest to follow the naming convention from RFC 5545, e.g.
"byweekno", "bymonth", and "week-start".
* Section 3.3.9
Why have a leaf with current local time instead of the pattern that
was used before with yang:date-and-time leafs and a
timezone-identifier if the times are in local time? This incurs
different calculations to be made to infer the time offset from UTC
for the schedule-status groupings and the other groupings. In one the
timezone is supplied and can be feed to a time generating function to
calculate the localtime, in the other the current UTC timestamp is
diffed from the supplied localtime to calculate what timezone (or
offset from UTC) is used.
Suggest to use the pattern with timezone-identifier even for the
schedule-status groupings.
* Section 6
RFC 6991 is listed as a normative reference. However the numerical
values in the must statement in period-start are magical constants in
the YANG module without any further explanation. Suggest to add a
reference and elaborate what the leaf values represent, instead of
just stating "start/stop time".
Nits:
The following commands yield a diff
$ pyang -f yang --keep-comments --yang-line-length=69 \
[email protected] > new.yang
$ diff [email protected] new.yang
11d10
<
283a283
>
439,440c439
< error-message
< "Frequency must be provided.";
---
> error-message "Frequency must be provided.";
594,611c593,609
< must
< "(not(derived-from(../../frequency,"
< +"'schedule:secondly')) or (current() < 100)) and "
< +"(not(derived-from(../../frequency,"
< +"'schedule:minutely')) or (current() < 6000)) and "
< +"(not(derived-from(../../frequency,'schedule:hourly'))"
< +" or (current() < 360000)) and "
< +"(not(derived-from(../../frequency,'schedule:daily'))"
< +" or (current() < 8640000)) and "
< +"(not(derived-from(../../frequency,'schedule:weekly'))"
< +" or (current() < 60480000)) and "
< +"(not(derived-from(../../frequency,"
< +"'schedule:monthly')) or (current() < 267840000)) and "
< +"(not(derived-from(../../frequency,'schedule:yearly'))"
< +" or (current() < 3162240000))" {
< error-message
< "The period-start must not exceed the frequency
< interval.";
---
> must "(not(derived-from(../../frequency,"
> + "'schedule:secondly')) or (current() < 100)) and "
> + "(not(derived-from(../../frequency,"
> + "'schedule:minutely')) or (current() < 6000)) and "
> + "(not(derived-from(../../frequency,'schedule:hourly'))"
> + " or (current() < 360000)) and "
> + "(not(derived-from(../../frequency,'schedule:daily'))"
> + " or (current() < 8640000)) and "
> + "(not(derived-from(../../frequency,'schedule:weekly'))"
> + " or (current() < 60480000)) and "
> + "(not(derived-from(../../frequency,"
> + "'schedule:monthly')) or (current() < 267840000)) and "
> + "(not(derived-from(../../frequency,'schedule:yearly'))"
> + " or (current() < 3162240000))" {
> error-message
> "The period-start must not exceed the frequency
> interval.";
674,675c672,673
< + "(derived-from(../../frequency, 'schedule:yearly') "
< + " and not(../../byyearweek))";
---
> + "(derived-from(../../frequency, 'schedule:yearly') "
> + " and not(../../byyearweek))";
Spelling:
iclander -> iCalender
occurences -> occurrences
avaliable -> available
paramter -> parameter
prioritised -> prioritized
specifc -> specific
maxinum -> maximum
difficient -> difficult (?)
feaure -> feature
YANGDOCTORS -> YANG Doctors
Thanks for your contribution!
--
Per
_______________________________________________
netmod mailing list -- [email protected]
To unsubscribe send an email to [email protected]
