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. > > Issues > 1. YANG model P8, for “leaf xpath-filter”, add reference to RFC6021. There should also be a normative reference to RFC6021 (as per RFC8407) <ALEX> Thanks. Adding reference to 6991 (as 6021 is obsoleted). </ALEX> > 2. Example P10, </interfa should be </interfaces> <ALEX> Thanks. Of course! </ALEX> > 3. Example P10, last paragraph talks about preference and explicit-router-id. This seems to be leftover from when the example was based on OSPF model. <ALEX> Yes, this is a leftover. Updated the text accordingly. </ALEX> > 4. Example P12 and P13. The RPC operation has “operational” as source (enabled is true) and “intended” as target (enabled is false). The differences shown (in RPC output) have “value true” and “source-value false”. But I thought value came from target datastore and source-value from source datastore, so the values are reversed, i.e.. it should be “value false” and “source-value true” instead? Looking at the origin in the output I am wondering if the intent is to have “intended” as source and ”operational” as target. Or am I misunderstanding this? <ALEX> Yes, the example contains a few mistakes. The values were reversed, and in addition what you mentioned I noticed an issue with the treatment of origin metadata (only part of <operational>. It is fixed now. </ALEX> > 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…. <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> > 2. YANG model: prefix “cp” doesn’t seem to be a great choice since cp is associated with copying. I realize that there is some preference for 2-letter prefixes, but to me “cp” is not a great choice. What about “cmp”? WG/chairs should have a word to say about this. <ALEX> Changing it to cmp, pending comments by WG chairs </ALEX> > 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. > 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. </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> > > Nits: > 1. P11 replace <operational< with <operational> <ALEX> fixed </ALEX> > > On 2020-09-06, 4:42 PM, "yang-doctors on behalf of Reshad Rahman via Datatracker" <yang-doctors-bounces@xxxxxxxx on behalf of noreply@xxxxxxxx> wrote: > > Reviewer: Reshad Rahman > Review result: Ready with Issues > > 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. > > Issues > 1. YANG model P8, for “leaf xpath-filter”, add reference to > RFC6021. There should also be a normative reference to RFC6021 (as per > RFC8407) 2. Example P10, </interfa should be </interfaces> 3. > Example P10, last paragraph talks about preference and > explicit-router-id. This seems to be leftover from when the example was > based on OSPF model. 4. Example P12 and P13. The RPC operation has > “operational” as source (enabled is true) and “intended” as target > (enabled is false). The differences shown (in RPC output) have “value > true” and “source-value false”. But I thought value came from target > datastore and source-value from source datastore, so the values are > reversed, i.e.. it should be “value false” and “source-value true” > instead? Looking at the origin in the output I am wondering if the > intent is to have “intended” as source and ”operational” as target. Or > am I misunderstanding this? > > 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…. 2. YANG model: > prefix “cp” doesn’t seem to be a great choice since cp is associated > with copying. I realize that there is some preference for 2-letter > prefixes, but to me “cp” is not a great choice. What about “cmp”? > WG/chairs should have a word to say about this. 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)? 4. Section > 7 mentions rate limiting requests per client. Should there be a > “global” rate-limiting too, i.e not client-specific? 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. > > Nits: > 1. P11 replace <operational< with <operational> > > > > _______________________________________________ > yang-doctors mailing list > yang-doctors@xxxxxxxx > https://www.ietf.org/mailman/listinfo/yang-doctors > > _______________________________________________ > netmod mailing list > netmod@xxxxxxxx > https://www.ietf.org/mailman/listinfo/netmod -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
