[CCAMP] Yangdoctors last call review of draft-ietf-ccamp-layer0-types-03

Robert Wilton via Datatracker <noreply@ietf.org> Fri, 13 December 2019 16:50 UTC

Return-Path: <noreply@ietf.org>
X-Original-To: ccamp@ietf.org
Delivered-To: ccamp@ietfa.amsl.com
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id 35FDE120112; Fri, 13 Dec 2019 08:50:02 -0800 (PST)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: Robert Wilton via Datatracker <noreply@ietf.org>
To: yang-doctors@ietf.org
Cc: last-call@ietf.org, draft-ietf-ccamp-layer0-types.all@ietf.org, ccamp@ietf.org
X-Test-IDTracker: no
X-IETF-IDTracker: 6.113.0
Auto-Submitted: auto-generated
Precedence: bulk
Reply-To: Robert Wilton <rwilton@cisco.com>
Message-ID: <157625580216.12967.1363518560797059736@ietfa.amsl.com>
Date: Fri, 13 Dec 2019 08:50:02 -0800
Archived-At: <https://mailarchive.ietf.org/arch/msg/ccamp/vpIjwy2HokZI74pKkIBrr0fziIw>
Subject: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-layer0-types-03
X-BeenThere: ccamp@ietf.org
X-Mailman-Version: 2.1.29
List-Id: Discussion list for the CCAMP working group <ccamp.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/ccamp>, <mailto:ccamp-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/ccamp/>
List-Post: <mailto:ccamp@ietf.org>
List-Help: <mailto:ccamp-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/ccamp>, <mailto:ccamp-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 13 Dec 2019 16:50:02 -0000

Reviewer: Robert Wilton
Review result: Almost Ready

Comments on the document:

1) I would delete the "overview" paragraph at the top of section 2, and just
promote section 2.1 as section 2.

2) 2.1. Layer 0 Types Module Contents:

The descriptions are good, but I would suggest formatting these as a table, or
more tightly link the definition to it's description.

E.g.

"Operational-mode: A type that represents operational mode as defined in
[ITU-Tg6982]."

Instead of:

"Operational-mode:

A type that represents operational mode as defined in [ITU-Tg6982]."

3) I would define the module as YANG version "1.1" (because the language
behaviour is generally better specified) and then reference only RFC 7950 in
the introduction.

4) typo in the Security Considerations "layer0 => layer 0", and also in the
title of section 3.

5) I have suggested changing the module prefix to "l0-types" rather than
layer2-types.  If you make this change then the IANA considerations would need
to be updated, along with section 1.2.

Comments on the YANG module:

1) I would suggest changing the YANG prefix to "l0-types" to help keep it short
(particularly for the identities).

2) I would suggest making the module "yang-version 1.1;", because the behaviour
of YANG 1.1 is better specified.  In fact, I would recommend that all IETF YANG
modules are defined as YANG 1.1.

3) It is actually necessary to define frequency-thz at all?  I think that the
discussion in the WG suggested that it might be better if the frequencies are
always defined in Ghz and then converted in the client as necessary.

4) frequency-ghz: Is 3 fraction digits sufficient for future expansion?  E.g.
It would seem to support a flex grid 3.125Ghz channel spacing, but not 1.5625
...

5) We should aim for consistency of the identity names in the layer-1 types
module.  E.g. perhaps OTU, ODU and OPU should be capitalized.

6) The models uses identities for bandwidth, but I wonder whether defining a
numerical typedef might be a better choice (e.g. more efficient and perhaps
easier for programs to work with).  Here, I have constrained the values that
are allowed, but they could also be unconstrained:

E.g.
typedef channel-bandwidth {
  type decimal64 {
    fraction-digits 2;
    range
      "2.66|10.70|11.04|11.09|11.27|11.31|43.01|44.57|44.58|100.00 ... max"
    description
      "Bandwidth carried by a single wavelength channel"
  }
  units "Gb/s";
}

Or another alternative would be to use Mb/s, which would then allow them to be
specified as a union of specific values and an arbitrary bandwidth value.

7) Same comment as for bandwidth applies to the channel spacing identities. 
I.e. I wonder whether these wouldn't be better defined using a decimal64 type.

E.g.
typedef dwdm-channel-spacing {
  type decimal64 {
    fraction-digits 2;
    range
      "12.5|25|50|100"
    description
      "Bandwidth carried by a single wavelength channel"
  }
  units "Ghz";
}

8) Same comment as for bandwidth and dwdm-channel-spacing could also be applied
to flexi-grid-channel-spacing, flexi-slot-width-granularity,
cwdm-channel-spacing.

9) In grouping layer0-label-range-info
I would rename this grouping to l0-layer-range-info, change "layer0" => "layer
0" in the description.  Also "priority" could do with a more detailed
description as to what it means, etc.

10) In grouping flexi-grid-label-start-end, I think that the type should be
"flexi-n" rather than "int16".

Typos: "girds" => "grids", "attrtibutes" => "attributes"

Spacing/indenting needs to be fixed:
 - In "grouping wson-label-hop", just before case cwdm
 - In "grouping flexi-grip-label-hop", should have a blank line before, and
 "case super" block/fields indentation doesn't look right. - In some of the
 typedef definitions, the "{" should move from the start of the following line
 to the typedef line. In general, as a starting point, after all other markups
 have been made then it might be a good idea to use pyang to format the YANG
 file for you, e.g., "pyang -f yang --yang-line-length 69", but probably with
 some more blank lines, otherwise it is a bit dense.