[Teas] Yangdoctors early review of draft-ietf-teas-yang-te-21
Radek Krejčí via Datatracker <email@example.com> Thu, 13 June 2019 07:49 UTC
Received: from ietfa.amsl.com (localhost [IPv6:::1]) by ietfa.amsl.com (Postfix) with ESMTP id E16451200E6; Thu, 13 Jun 2019 00:49:28 -0700 (PDT)
Content-Type: text/plain; charset="utf-8"
From: =?utf-8?q?Radek_Krej=C4=8D=C3=AD_via_Datatracker?= <firstname.lastname@example.org>
Cc: email@example.com, firstname.lastname@example.org
Reply-To: =?utf-8?b?UmFkZWsgS3JlasSNw60=?= <email@example.com>
Date: Thu, 13 Jun 2019 00:49:28 -0700
Subject: [Teas] Yangdoctors early review of draft-ietf-teas-yang-te-21
List-Id: Traffic Engineering Architecture and Signaling working group discussion list <teas.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/teas>, <mailto:firstname.lastname@example.org?subject=unsubscribe>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/teas>, <mailto:email@example.com?subject=subscribe>
X-List-Received-Date: Thu, 13 Jun 2019 07:49:29 -0000
Reviewer: Radek Krejčí Review result: On the Right Track This is my YANG doctor review of draft-ietf-teas-yang-te-21 containing 2 YANG modules: - firstname.lastname@example.org - email@example.com The draft and both modules follows NMDA. Since I'm not expert in the area, the review focuses on YANG-specifics. Draft text ---------- 1. The second paragraph in 3.2 - items of the list are not properly formatted. Both YANG modules ----------------- 2. Some of the multiline strings (descriptions) are not correctly formatted, there is: "some text"; instead of "some text"; Also better consistency in writing description as a sentence (with capital first letter and full stop at the end) or not would be nice. 3. Remove WG Chairs from contact information (see https://tools.ietf.org/html/rfc8407#appendix-B) firstname.lastname@example.org ----------------------- 4. grouping path-properties: path-properties/path-route-objects/path-computed-route-objects - The list is defined as ordered-by user, but as a config false item (inherited from path-route-objects), the ordered-by statement is ignored (RFC 7950: 7.7.7.). 5. grouping p2p-path-properties-common :path-computation-server grouping p2p-path-properties-common :use-path-computation - Use derived-from-or-self() in when-stmt expression instead of comparing string values (RFC 8407: 4.6.2.). 6. There is a number of single-instantiated groupings: - p2p-reverse-path-properties - p2p-primary-reverse-path-properties - p2p-secondary-reverse-path-properties - p2p-primary-path-properties - p2p-secondary-path-properties - hierarchical-link-properties - protection-restoration-properties-state - ... (mybe the most of all that groupings) While it is a good mechanism to allow repeated instantiation of a group of statements, it can produce kind of "spaghetti code" - with such a number of groupings, it is quite challenging for human readers to follow the schema. And one of the YANG advantages is its readability. Please consider, if there is some real possibility to reuse groupings somewhere outside the schema itself. If yes, then ok - readability is the price for reuseability, but otherwise you actually don't get anything for poor readability. 7. grouping p2p-reverse-path-properties: named-path-constraint grouping p2p-path-properties: named-path-constraint - Use absolute leafref path, in this case you probably don't want to refer to something relative to the grouping (or something in the grouping), but specifically to /te/globals/... using relative path in this case really complicates ability to reuse the grouping somewhere else. Maybe in some cases this can be also an indicator of a grouping which is actually not supposed to be a separate grouping. - Similarly in the following nodes: grouping p2p-dependency-tunnels-properties: dependency-tunnels/dependency-tunnel/name grouping p2p-path-candidate-secondary-path-config: secondary-path ... 8. grouping protection-restoration-properties-state: lockout-of-normal grouping protection-restoration-properties-state: freeze - Weird formatting of the descriptions with the first and last empty lines. 9. grouping protection-restoration-properties: protection/hold-off-time grouping protection-restoration-properties: restoration/hold-of-time - Is there any compatibility reason to have 'milli-seconds' units? If not, change it to milliseconds. 10. grouping tunnel-p2p-associations-properties: association-objects(-extended) - As mentioned, I'm not an expert in the area, but what is the global-source (in contrast to source) and why is it a key of the lists? I didn't find it mentioned in RFC4872, so the reference is wrong. - Use lower case for the node identifiers (ID, extended-ID - other *-id identifiers in the document are lowercase) 11. RPCs and Notifications - From the descriptions, it is unclear what exactly the RPCs are supposed to do and about what the Notifications are supposed to notify. The description "TE tunnels RPC nodes" is completely useless. email@example.com ------------------------------ 12. grouping tunnel-device-config - It is not very clear why this grouping is actually defined, why other modules should import and instantiate this single-leaf grouping? At least the description must be improved. - Similarly, what are the use cases for interfaces-rpc and (empty) interfaces-notif (as 11)? 13. grouping te-admin-groups-config: admin-group-type/named-admin-groups/named-admin-groups/named-admin-group grouping te-srlgs-config: srlg-type/named-srlgs/named-srlgs/named-srlg - Same as 7. 14. grouping te-srlgs-config: srlg-type/value-srlgs/values/value - The range actually covers whole range of the type itself.
- [Teas] Yangdoctors early review of draft-ietf-t... Radek Krejčí via Datatracker