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" <[email protected] on behalf of [email protected]> > 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 > [email protected] > https://www.ietf.org/mailman/listinfo/yang-doctors > > _______________________________________________ > netmod mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/netmod _______________________________________________ netmod mailing list [email protected] https://www.ietf.org/mailman/listinfo/netmod
