Re: [netmod] [yang-doctors] Yangdoctors last call review of draft-ietf-netmod-nmda-diff-04

[Alexander L Clemm <[e-mail]>]

Hi Reshad,

re: question 1: As you indicate, there may be no distinction between
indicating a "remove" or a "delete" in the patch.  Right now it would be
acceptable to return either.  If we want to eliminate this freedom,
which one would you prefer be used?  Shall we remove the possibility for
"delete" and just cover it using "remove"? 

Note that the place where this is specified in the model is as part of a
condition that specifies when the source value should be included.   If
we want to rule out that diff can return either "remove" or "delete"
(indeed they are synonymous), we would need to add text to the container
description that when a data object is present in the target of the
comparison but not the source, that "remove" should be used to indicate
that.

The model would be changed follows.  Please confirm if this looks good
to you & we'll incorporate it. 

OLD

           container differences {
             description
               "The list of differences, encoded per RFC8072 <https://tools.ietf.org/html/rfc8072> with an
                augmentation to include source values where
                applicable.";
             uses ypatch:yang-patch {
               augment "yang-patch/edit" {
                 description
                   "Provide the value of the source of the patch,
                    respectively of the comparison, in addition to
                    the target value, where applicable.";
                 anydata source-value {
                   when "../operation = 'delete'"
                     + "or ../operation = 'merge'"
                     + "or ../operation = 'move'"
                     + "or ../operation = 'replace'"
                     + "or ../operation = 'remove'";
                   description
                     "The anydata 'value' is only used for 'delete',
                      'move', 'merge', 'replace', and 'remove'
                      operations.";
                 }
                 reference "RFC 8072 <https://tools.ietf.org/html/rfc8072>: YANG Patch Media Type";
               }
             }
           }


NEW:

           container differences {
             description
               "The list of differences, encoded per RFC8072 <https://tools.ietf.org/html/rfc8072> with an
                augmentation to include source values where
                applicable.  Where a difference include a data object
                in the target that is not present in the source, 
                this shall be indicated as a 'remove' operation 
                in the patch, not as a 'delete' operation.";
             uses ypatch:yang-patch {
               augment "yang-patch/edit" {
                 description
                   "Provide the value of the source of the patch,
                    respectively of the comparison, in addition to
                    the target value, where applicable.";
                 anydata source-value {
                   when "../operation = 'merge'"
                     + "or ../operation = 'move'"
                     + "or ../operation = 'replace'"
                     + "or ../operation = 'remove'";
                   description
                     "The anydata 'value' is only used for 'merge',
                      'move','replace', and 'remove' operations.";
                 }
                 reference "RFC 8072 <https://tools.ietf.org/html/rfc8072>: YANG Patch Media Type";
               }
             }
           }


Thanks
--- Alex

On 9/15/2020 4:04 PM, Reshad Rahman (rrahman) wrote:
> Hi Alex,
>
> I will review the latest version.
>
> See below for questions/responses.
>
> On 2020-09-15, 5:19 PM, "yang-doctors on behalf of Alexander L Clemm" <[e-mail] on behalf of [e-mail]> wrote:
>
>     Hello Reshad, hello YANG Doctors,
>
>     thank you for your review!  Please find my replies inline, <ALEX>.  We
>     have also just posted -05 (thanks, Yingzhen, for doublechecking my
>     updates).   
>
>     --- Alex on behalf of coauthors
>
>     On 9/7/2020 7:06 AM, Reshad Rahman (rrahman) wrote:
>     > <Here's the same message with hopefully more readable formatting>
>     >
>     > Review of rev -04 by Reshad Rahman
>     >
>     > The document is clear and well-written. While some issues have been identified, they can be resolved quickly.
>     >
> <snip>
>
>     > Questions
>     > 	1.	YANG model: does the operation “delete” make sense for a diff operation? If it is kept, it’d be good to have some text explaining that for a diff operation, “delete” and “replace” are the same? If they’re not the same, please also add some text….
> <RR> I actually meant "delete" and "remove".
>     <ALEX> Here we are simply referring to the basic YANG-patch edit
>     operations per https://tools.ietf.org/html/rfc8072#page-11.  Those are
>     in turn derived from <edit-config> operations per
>     https://tools.ietf.org/html/rfc6241#page-37.  I am not sure we need add
>     to explain those, as we are directly referring to YANG-patch. 
>
>     </ALEX>
> <RR> The operations are indeed well defined in RFC8072 (copied below), but they are defined from the perspective of YANG-Patch. So for YANG-Patch "delete" and "remove" are different operations, but from a diff comparison I believe they are the same (the resource must exist since it's being returned in a diff)
>
>    +-----------+-----------------------------------------------------------------+
>    | delete    | delete a data resource if it already exists; if it    |
>    |                | does not exist, return an error                               |
>    |                |                                                                                      |
>    | remove | remove a data resource if it already exists           |
>    +-----------+-----------------------------------------------------------------+
>
>     > 	3.	YANG model P9, for the “uses path:yang-patch”, why not have a  reference to RFC8072 (is it because the description above mentions RFC8072)?
>     <ALEX> We are clearly referencing RFC 8072; are you suggesting to put a
>     reference substatement below the uses statement?   It looks a little
>     strange to me but sure, we will add it.   
> <RR> Not needed. 
>
>     > 	4.	Section 7 mentions rate limiting requests per client. Should there be a “global” rate-limiting too, i.e not client-specific?
>
>     <ALEX> I am not sure this is really needed as I think the number of
>     management clients will in general be fairly limited to begin with, but
>     we can certainly add it.  How about the following text:
>
>     OLD:
>
>     One possibility for an implementation to mitigate against such a
>     possibility is to limit the number of requests that is served to a
>     client in any one time interval, rejecting requests made at a higher
>     frequency than the implementation can reasonably sustain.
>
>     NEW:
>
>     One possibility for an implementation to mitigate against such a
>     possibility is to limit the number of requests that is served to a
>     client, or to any number of clients, in any one time interval, rejecting
>     requests made at a higher frequency than the implementation can
>     reasonably sustain.
> <RR> Good with me.
>
>     </ALEX>
>
>     > 	5.	Wondering if section 8 should be in an Appendix (or even removed)? Also, the method suggested doesn’t seem to guarantee that the difference persisted for the “dampening” time.
>
>     <ALEX> Personally, I do think it makes sense to include a brief
>     discussion of possible further extensions.  I suggest to keep the
>     section if it's okay with you, or perhaps leave it to the chair whether
>     they have a preference to remove it.  
>
>     </ALEX>
> <RR>Whatever the WG/chairs decide is fine with me.
>
> Regards,
> Reshad.
>
>