Re: [yang-doctors] Yangdoctors last call review of draft-ietf-ccamp-layer0-types-01

Young Lee <younglee.tx@gmail.com> Fri, 26 July 2019 20:57 UTC

Return-Path: <younglee.tx@gmail.com>
X-Original-To: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 4320C12006E; Fri, 26 Jul 2019 13:57:35 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.997
X-Spam-Level:
X-Spam-Status: No, score=-1.997 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id n4KlJYxhIyEk; Fri, 26 Jul 2019 13:57:32 -0700 (PDT)
Received: from mail-io1-xd31.google.com (mail-io1-xd31.google.com [IPv6:2607:f8b0:4864:20::d31]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 28F201200B7; Fri, 26 Jul 2019 13:57:29 -0700 (PDT)
Received: by mail-io1-xd31.google.com with SMTP id k8so107598812iot.1; Fri, 26 Jul 2019 13:57:29 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=i+kU1VVGct6XFn6nnw6BfbO0NumiG0TAUgUDxMYHxa8=; b=H7D5sDENLyMpo5vcBwbYlHtl8+Fu87oKQ7Id1dbK89fZcXzdHDO4WnZH0E5TTaqLOf XIHfoi+FgnrVoqICbUn2s4kID6EQvLg+sbR7b5cJkb6orW/RU3hQ1riNtaM+AeezQh4C 0pXYxgYOZ2Wgm2q+ILdYH5fh+fA2dD9DDybiPOmglUd0xrkLZYtaTO/4HJygtTi3Qpay GSDSLpkEH1DC71dNs6qm/EkNsO9FVwYOXHgFebPjiWkIzpsJQKr5i6qx0ri6XXMPSgH3 1w1XfvXaElKhFQncwXI30UiZMfGSDpj88470wF3EDQNrSTrUjD3zOifWNF5WcwH9Qi89 x+ow==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=i+kU1VVGct6XFn6nnw6BfbO0NumiG0TAUgUDxMYHxa8=; b=JnwMg2uxxIcS0H4kOMFHbGmj3mrwW79at0aWL1ORRH8g468jI/YEYJEzL9ZSq26Goa FK+Rk84HYeqPo9lTtd8kowYDP04ExL+3Cyuk7F8rWFumURUg3ipdwRuhsdgwctlLJpGs 4KvMOGRBCLiYhrBCZfuSG9jqN9rInvLB6K25VuWsrT8oRbzuB7HTRe9tQ0S3EzedM+EH ykjMmcv12FlBLcCjBE8i+irw4B+/LCwF5ME343cgPOFnDRlTzqxEzF2YnB3HmpVCcrN1 IVLEC3CvMNuoiyIXwcjIyHSGnzl0w65Wo0gV1GtWzegubCo5RICIio05BlhUub2ey0v+ GheQ==
X-Gm-Message-State: APjAAAVL6DybCbwGLNgtL30YtA8El03fVGDrB+50VGxEzxRLH8EGo82D glK383oBlE6JbVaHE+ZtiYNfG5fiksDl40rQJak=
X-Google-Smtp-Source: APXvYqy01q5G3WJN0uzkfWRzddBVCwI9EGIy0GAFv4BiKKVnLWgm/+T9VC8Ee3mpJzAoqCwoqjlZfqB0ezmMnrf+TmY=
X-Received: by 2002:a02:c646:: with SMTP id k6mr26443086jan.134.1564174648269; Fri, 26 Jul 2019 13:57:28 -0700 (PDT)
MIME-Version: 1.0
References: <156410979633.17774.2714564310181191660@ietfa.amsl.com>
In-Reply-To: <156410979633.17774.2714564310181191660@ietfa.amsl.com>
From: Young Lee <younglee.tx@gmail.com>
Date: Fri, 26 Jul 2019 15:57:18 -0500
Message-ID: <CAGHSPWNA6k6EMa3DfeG_JSGecRkP5gjw7pU=rKOCtardihhk0Q@mail.gmail.com>
To: Robert Wilton <rwilton@cisco.com>
Cc: yang-doctors@ietf.org, draft-ietf-ccamp-layer0-types.all@ietf.org, "CCAMP (ccamp@ietf.org)" <ccamp@ietf.org>, ietf@ietf.org
Content-Type: multipart/alternative; boundary="00000000000046dece058e9bcab1"
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/Rbe79-_vO1L-nriiGoqLojbmpVg>
Subject: Re: [yang-doctors] Yangdoctors last call review of draft-ietf-ccamp-layer0-types-01
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 26 Jul 2019 20:57:36 -0000

Hi Robert

Thanks for your YANG doctors review. Please see the response to your
comment in line. Once you give us a green signal, we will upload the draft
as we have agreed on.

Best regards,
Young

On Thu, Jul 25, 2019 at 9:56 PM Robert Wilton via Datatracker <
noreply@ietf.org> wrote:

> Reviewer: Robert Wilton
> Review result: Ready with Nits
>
> I have reviewed this document as part of the YANG doctors 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.
>
> Nits:
>
> 1) Doc title: Perhaps change to "A YANG Data Model for TE Layer 0 Types",
> also
> changing the short name from "Layer0 Types" to "TE Layer 0 Types".
>
> YL>> I think we should stay as is. The reason for this is that layer 0
types are applicable mainly for TE but not limited to. We can envision the
groupings defined in this document can be applicable for configuring
physical network components. I would add in the abstract. "The
applicability of this document is mainly for TE, but not limited to."

2) Abstract:
>  "in YANG" => "in the YANG"
>

YL>> Agree.


>  Add the comment to the abstract:
>
>    The YANG data model in this document conforms to the Network
>    Management Datastore Architecture defined in RFC 8342.
>

YL>> Agree.

>
> 3) Section 1.1 is probably not required, or at least the document doesn't
> appear to use RFC2119 language.
>

YL>> Agree.

>
> 4) Section 1.3. Prefix and Module naming:
> The module description states that the types are for TE.
> Hence, would a module name of "ietf-te-layer0-types", with a module prefix
> of
> "te-l0-types" be better?
>

YL>> As mentioned above, we;d like to keep the module name as is. In
Introduction, we can change some wording to make sure this document's
applicability is larger than TE.

Old:

The derived types and groupings are designed to be the common types
applicable for modeling Traffic Engineering (TE) for Layer 0 optical
networks in model(s) defined outside of this document.

New:

The derived types and groupings are designed to be the common types
applicable for modeling Traffic Engineering (TE) features as well as non-TE
features (e.g., physical network configuration aspect) for Layer 0 optical
networks in model(s) defined outside of this document.


5) Section 3.1 A lot of the types have "flexi-grid" in the type name by
> "flex-grid" in the description.  Perhaps it would be appropriate to change
> "flex-grid" to "flexi-grid" or "flexible-grid"?
>

YL>> Will change globally to "flexi-grid".

>
> YANG Module:
>
> (1) The main comment is that you should define typedefs in a couple of
> places:
>
> (1-i) For the dwdm-n frequency definition (used by leafs dwdn-n in
> groupings
> wson-link-label and wson-path-label, and leaf flexi-n in grouping
> flexi-grid-link-label):
>
> The description for this typedef (which can be removed from the
> description in
> the leaves) should probably be:
>  "The given value 'N' is used to determine the nominal
>   central frequency.
>
>   The nominal central frequency, 'f' is defined by,
>     f = 193.1 THz + N x 0.00625 THz,
>   where 193.1 THz is the ITU-T 'anchor frequency' for
>   transmission over the C band";
>
> (1-ii) For the cwdn-n frequency definition (used by leafs cwdm-n in
> groupings
> wsol-link-label and wson-path-label):
>
> The description for this typedef (which can be removed from the
> description in
> the leaves) should probably be:
>  "The given value 'N' is used to compute the channel
>   wavelength as per the formula:
>     Wavelength (nm) = 1471 + N x 20";
>
> The typedef's reference statement should be "ITU-T G.694.2" (perhaps also
> with
> the date it is published as MM/YYYY)
>
> (1-iii) It might also be worth defining a typedef for the subcarrier-dwdn-n
> leaf, even though the type is only used is one place.
>
>
YL>> We can define typedef fo the first two cases. For (1-iii), we may keep
it as is.


> Minor comments:
>
> (2) I note that your sample values in frequency-thz are not in the
> canonical
> value (e.g. the canonical value would be "193.125" rather than
> "193.12500").  I
> think that this is OK, but wanted to point it out.
>

YL>> This is not too big a deal. We can change to 193.125 (which is
typical); also change
fraction-digits to 3 to match with the example.

>
> (3) Your fequency-Ghz is to 5 fractional digits, e.g. allowing
> "193125.12345"
> Ghz.  I.e. this is 1000 times more precise than the Thz value.  Is this
> intentional?  If you keep it as it is, then I would consider change the
> example
> value to "193125.0".
>

YL>>  I agree. This may be an overkill. I will change the fraction-digits
to "1" and show the
example as you suggested.


> (4) "flexi Grid node" => "Flexi-grid node"
>

YL>>Agree.

>
> (5) "identity random-wavelength-assignment" => Check line length of
> description, probably could be expanded.
>

YL>> I think so. I will change this as well.

>
> (6) "Layer0 grid type." => "Layer 0 grid type"
>

YL>> Agree.

>
> (7) "Termination type." => "Termination type"
>

YL>> Agree.

>
> (8) In several places "Termination" at end of description => "termination"
>

YL>> Agree.

>
> (9) "identity term-phys" => description to "Physical layer termination",
> indent
> all description and reference strings to 2 spaces after the begining of the
> word 'description'/'reference' word.  Also need to fix in "identity
> fec-type"
> reference statement,
>

YL>>  Agree.

>
> (10) "Section Layer Termination" => "Section layer termination"
>

YL>> Agree.

>
> (11) Some of the reference statements should be expanded from just the RFC
> number to also the RFC name.
>

YL>> Agree.

>
> (12) The description of subcarrier-dwdn-n leaf-list should probably be
> something like:
>
> description
>   "List of subcarrier channels for the super channel.
>
>    The given value 'N' is used to determine the nominal
>    central frequency.
>
>    The nominal central frequency, 'f', is defined by,
>     f = 193.1 THz + N x 'channel spacing (THz)',
>   where 193.1 THz is the ITU-T 'anchor frequency' for
>   transmission over the C band";
>
> reference "ITU-T G.694.1";
>

YL>> Thanks for making the description clearer. I think we need to modify
as follows:

description
  "List of subcarrier channels for the super channel.

   The given value 'N' for each subcarrier channel
   is used to determine the nominal
   central frequency.

   The nominal central frequency, 'f', is defined by,
    f = 193.1 THz + N x 'channel spacing (THz)',
    where 193.1 THz is the ITU-T 'anchor frequency' for
    transmission over the C band,
  N is a positive or negative integer including 0";

   reference "ITU-T Recommendation G.694.1: Spectral girds for WDM
applications:
                     DWDM frequency grid";

>
> (13) leaf "priority" description "priority" => "Priority"
>

YL>> Agree.

>
> (14) grouping flexi-grid-channel description, "where M is an integer
> greater or
> equal to 1" => "where M is a positive integer"
>

YL>> OK.

>
> (15) "onsidered" => "considered"
>

YL>> Agree.

>
> (16) Remove extra space in hte URI in the IANA considerations.
>

YL>> Agree.


> Thanks,
> Rob
>
>