Re: [netmod] Yangdoctors last call review of draft-ietf-netmod-node-tags-04

[Qin Wu <[e-mail]>]

Thanks Mahesh for valuable review. Please see reply inline below.

-Qin
>-----邮件原件-----
>发件人: Mahesh Jethanandani via Datatracker [mailto:[e-mail]]
>发送时间: 2022年2月1日 13:25
>收件人: [e-mail]
>抄送: [e-mail]; [e-mail]; [e-mail]
>主题: Yangdoctors last call review of draft-ietf-netmod-node-tags-04

>Reviewer: Mahesh Jethanandani
>Review result: On the Right Track

>Summary:

>This document defines a method to tag data objects associated with operation and management data in YANG Modules.  This YANG data object tagging method can be used to classify data objects from different YANG modules and identify characteristics 
>data.

>Nits

>/subobjects/sub-objects/g
>[Qin] Okay.
>Comments:

>If the document updates RFC 8407, it needs to mention that in the Abstract.
>Also the abstract can be shortened to what the document defines, and move everything else into the introduction.


[Qin] Good point, similar comment was brought up by Adrian, I will make Abstract short.

>The document says "This document defines an extension statement ...". Is only extension statement defined?
[Qin]:I am not sure I capture your comment. But this document define one YANG model, three extension statements and one IANA registry for IETF tags. Maybe I should tweak this sentence as follows:
"This document defines three extension statements..."

>Text like "data object tags may be registered as well as assigned during module definition" follow the pattern of RFC 8819 and should be referred to rather than duplicated. 
[Qin]:Agree to reference to RFC8819, but this document focuses on data object tags while RFC8819 focuses on model tag. I will see how to tweak the text to reflect your comment.

>If assigned during implementation, is there a possibility that the same tag is assigned by two different implementations? What is the scope of a given data object tag?
[Qin]: Note that the data object tags aim at data object classification. Therefore the same tag can be assigned even by one implementation to different data nodes. If the tag is the IETF tag defined in this document, we need to make sure different 
implementation or different device can assign the same tag to the same data node in the module. For IETF tag, we should make sure the tag is unique. Same rule is applied to other vendor tag or user tag, But we don't suggest to use IETF tag together with
either vendor tag or user tags. Hope this clarifies.

>Similarly, the draft says "objects can be one of container, leaf-list and list". Did you mean to say "objects can be one of type container, leaf-list and list"?
[Qin]: Correct, I can tweak the text as you suggested.

>The example in Figure 2 can be improved. For example, if all the data objects are for the module name "tunnel-pm", do you need the last column. 
[Qin]: I can take out the last column or keep the last column and remove duplicated text in each row by merging all the rows associated with last column into one row.

>More importantly, it is not clear why tunnel-src/max-latency (why a gap between / and max-latency), is not an object tag? Can a sub-object tag exist if the node is not an object tag?

[Qin] As shown in figure 1 and figure 2, you will see only root node will be tagged as object tag, in figure 2, only tunnel-svc can be seen as root node, tunnel-svc/max-latency is just a child node and therefore can not be tagged with *theobject tag *. Please also refer to section9.2 table for clear definition of object tag.
Secondly sub-object tag and object tag can not tag the same node, only root node will be tagged with object tag, Sub-object will be tagged with sub-object tags such as property tag, metric tag, metric-type tag, multi-source tag.

>In Section 4, Data Object Tag Values, it says tags can be any value except carriage-returns, newlines and tabs. Does it mean spaces are allowed? Can a data object have multiple tags? What does it mean "No further structure is imposed ..."?
[Qin]: I think tabs is similar to spaces, maybe 2 spaces or 4 spaces but with less disk space / memory / compiler resource.
Secondly, a data object can have multiple tags, see figure 1, a data object can have one metric tag, one metric-type tag or one multi-source tag.
Third, no further structure is imposed means we don't further define detailed format for the value following "ietf:" or "vendor:" or "user:", it can be any YANG type 'string', e.g., we will not require the value following "ietf:" to start from 'AAA' or 'BBB'.

>Section 4.2 introduces the concept of vendor prefix for tags. It says vendors include extra identification in the tag to avoid collision. But what is to say that two organizations may not use the same identification? And is this identifier part of the tag or is 
>separated from the tag with a :.
[Qin] Yes, Each vendor or organization can define its own extra identification, such identifier can be part of the vendor tag.

>Similarly, it says that user prefix is RECOMMENDED. If not using it can cause collision, why is use prefix RECOMMENDED and not a MUST?

 [Qin] Becos user prefix has two forms, one is prefixed with "user:", the other is without prefix "user:", that is why we choose RECOMMENDED rather than MUST.


>The draft has just one example. And it shows mostly ietf prefixed tags. More examples showing use of different types of tags are needed. It would be helpful to know how tags can be removed.


[Qin] In this document, only IETF prefixed tags are registered, user tags and vendor tags are not predefined or registered, in addition, the example for vendor tag and user tag has no big Difference with ietf prefixed tags. That is why we don't provide example for other type tags.

Secondly, similar to module tag defined in RFC8819, we defined a list of masked-tags in the module tag extension module in this document which allow user remove tag from operational datastores.


>Section 5 - YANG Module.

>The section does not reference the RFCs that it imports modules from, e.g.
>ietf-netcom-acm.
[Qin]: Good catch, will add.
>Inside the YANG model, import statements need to carry reference statement.
[Qin]:Okay.
>The WG link needs to refer to datatracker.ietf.org and not tools.ietf.org 
[Qin]:Good catch, will fix this.
>The Copyright statement has 2021 as the year.
[Qin]:Okay
>Line length should be limited to 72 columns.
[Qin]:Okay
>No need to repeat parent name in child node, e.g. object-name -> name.
[Qin]:Okay.
>Indentation is off in places, specially in the example.
[Qin]:Okay.
>A pyang compilation of the model with —ietf and —lint option was clean.

>A idnits run of the draft reveals a few issues. Please address them.
[Qin]:thanks, will fix the in v-05.
>draft-ietf-netmod-node-tags-04.txt:

>  Checking boilerplate required by RFC 5378 and the IETF Trust (see
>  https://trustee.ietf.org/license-info):
>  -------------------------------------------------------------------

>     No issues found here.

>  Checking nits according to
>  https://www.ietf.org/id-info/1id-guidelines.txt:
  -------------------------------------------------------------------

>     No issues found here.

>  Checking nits according to https://www.ietf.org/id-info/checklist :
  -------------------------------------------------------------------

>  ** There are 70 instances of too long lines in the document, the
>     longest one being 15 characters in excess of 72.

>  -- The draft header indicates that this document updates RFC8407,
     but the abstract doesn't seem to mention this, which it should.

>  Miscellaneous warnings:
  -------------------------------------------------------------------

>  == The copyright year in the IETF Trust and authors Copyright Line
     does not match the current year

>  == Line 404 has weird spacing: '...ct-name    nac...'

>  == Line 493 has weird spacing: '...dentify  multi...'

>  == Line 656 has weird spacing: '...resents  a pro...'

>  == Line 999 has weird spacing: '...dentify  multi...'

>  -- The document date (November 2021) is 77 days in the past.  Is
     this intentional?

>  Checking references for intended status: Proposed Standard
  -------------------------------------------------------------------

>     (See RFCs 3967 and 4897 for information about using normative
>     references to lower-maturity documents in RFCs)

>  == Missing Reference: 'I-D.netconf-notification-capabilities' is
>     mentioned on line 139, but not defined

>  == Missing Reference: 'I-D.ietf-netmod-yang-instance-file-format'
>     is mentioned on line 1106, but not defined

>     Summary: 1 error (**), 0 flaws (~~), 7 warnings (==), 2
>     comments (--).

>     Run idnits with the --verbose option for more detailed
>     information about the items above.