Re: [Netconf] mbj's YANG doctor's review and WGLC comments on yang-push-15
"Alexander Clemm" <ludwig@clemm.org> Thu, 31 May 2018 06:47 UTC
Return-Path: <ludwig@clemm.org>
X-Original-To: netconf@ietfa.amsl.com
Delivered-To: netconf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id A897112E036; Wed, 30 May 2018 23:47:15 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.899
X-Spam-Level:
X-Spam-Status: No, score=-1.899 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_NONE=-0.0001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id CNxkJvBhO6li; Wed, 30 May 2018 23:47:11 -0700 (PDT)
Received: from mout.perfora.net (mout.perfora.net [74.208.4.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 6E3F2126C19; Wed, 30 May 2018 23:47:11 -0700 (PDT)
Received: from LAPTOPR7T053C2 ([24.6.143.61]) by mrelay.perfora.net (mreueus001 [74.208.5.2]) with ESMTPSA (Nemesis) id 0Mgdfd-1fiPIz2VC3-00O0AE; Thu, 31 May 2018 08:46:50 +0200
From: Alexander Clemm <ludwig@clemm.org>
To: 'Martin Bjorklund' <mbj@tail-f.com>, netconf@ietf.org, yang-doctors@ietf.org
References: <20180316.104706.2162517973525816941.mbj@tail-f.com>
In-Reply-To: <20180316.104706.2162517973525816941.mbj@tail-f.com>
Date: Wed, 30 May 2018 23:46:47 -0700
Message-ID: <03a301d3f8ab$274e1100$75ea3300$@clemm.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
X-Mailer: Microsoft Outlook 16.0
Thread-Index: AQG7ecnXtDJvskNvSQqX9INyHyK5S6R6YhSQ
Content-Language: en-us
X-Provags-ID: V03:K1:NHGNi0mg2vHleG9EKRUXO56gAYMEHhZr21vDI9VS4IOq8sVHJG5 WLVYGHr8RVg2R7/kDiFMv1AtKzLVQ55nc5nXw/uhrkiwEaqyTkugJw97LtdWXzm36irGl/b tNH1eDRqQsxbUmnkJ7nUzC6wI96vau4IqbPgq521aTF/VS8ryw/WPQUw4LTdaS/QEU7ry2m zocgQ2qXqT29zWuNlp5Og==
X-UI-Out-Filterresults: notjunk:1;V01:K0:XZ8v1WvuvZw=:jz0UchdjzYFle8ozb6Eoom 7WT/GVFA0RHn7Ra2PlKyG+rgRaKdf1DlNvw8zI3dxjdFD8rZf64TciAEkmXFl173sTKvTpDzb H+OpPbvtnvlQre9kJP+6+TE3yItHMB/kJRI7742F30ZNxU1DoF+zVa9RVJXUUEqUtt3Ta27uM iH7r0eihJL+zkoka/mqM2mfvmb4Zj0TYyAFmvNJmjn8YsAuxEY+8CXWY+hYQ/0tfMsqi5KsJV 4bZwGukNk07+EL1Xp/hcwcwGc/5Vhg6t7IdWB3N2D5/OddKmPred8+1v8AH7eQtNmvIaeTUhR L4LJ/4jCsgAzhJ3Q+PYydmElilQ8/pTtwOyLXJPJr2siO4zR+0eNG0LVGQzT+YPE6JP/7Ekt6 bAeEUJnDA3zQEgIjQOy6H5yFk+RA/qKVsLUCbimdmwNxy98EfIQnD0GRDxycOSUtdLfEgSVnK LXpGgizXcigmG0+sq/cQZORG6F9xXJC4NEN8qC/yKmDG/kET5vB77VFAhCZV5VBZ4egbQwaRG 86C8TMUuGxt8lIglQsyWs6oJGxme6p2GdPoTciLdV1iwwvszHper/cnRs2JnC8wXdACfr0B4g iHcFu16V1tCD07lcgYktr8C2FH/uJxiIsz/CveWimpf1Oa1h5kEFPZVK3zo56Eq2x1V7t6mQm HL7al+yCXYSF6DxiAM7lgBaEyUgMFclnN12fiY8jNeCLl8UwOKbWtW/rzl6i1jEKuEYsu7FQB MmX4A+FmC1A1uGWp
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/cUNaS3TbZrmG-b3iYl6wyHX0-mo>
Subject: Re: [Netconf] mbj's YANG doctor's review and WGLC comments on yang-push-15
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: Network Configuration WG mailing list <netconf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netconf>, <mailto:netconf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netconf/>
List-Post: <mailto:netconf@ietf.org>
List-Help: <mailto:netconf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netconf>, <mailto:netconf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 31 May 2018 06:47:16 -0000
Hi Martin, Apologies for my delay in response. Thank you for your thorough review and comments! Please find enclosed our responses ("<ALEX>"). We will be posting an updated revision (-16) momentarily. --- Alex -----Original Message----- From: Netconf <netconf-bounces@ietf.org> On Behalf Of Martin Bjorklund Sent: Friday, March 16, 2018 2:47 AM To: netconf@ietf.org; yang-doctors@ietf.org Subject: [Netconf] mbj's YANG doctor's review and WGLC comments on yang-push-15 Hi, Here are my WGLC comments and YANG doctor's review on draft-ietf-netconf-yang-push-15. o On p. 13, theres a missing " (which messes up the colors in the emacs mode I use) OLD: responses to an establish-subscription request) or "modify- subscription-error-datastore (for error responses to a modify- NEW: responses to an establish-subscription request) or "modify- subscription-error-datastore" (for error responses to a modify- <ALEX> fixed </ALEX> o Section 2 I suggest you write that this document uses the terms defined in other docs: OLD: The terms below supplement those defined in [I-D.draft-ietf-netconf-subscribed-notifications]. In addition, the term "datastore" is defined in [I-D.draft-ietf-netmod-revised-datastores]. NEW: This document uses the terminology defined in [RFC7950], [I-D.draft-ietf-netmod-revised-datastores], and [I-D.draft-ietf-netconf-subscribed-notifications]. For clarity, I also suggest you introduce the new terms in a bullet list: OLD: Datastore node: An instance of management information in a datastore. Also known as "datastore node". NEW: o datastore node: An instance of management information in a datastore. Also known as "datastore node". Also, in one place, you use the term "data resource". Either import that term from the proper document, or change to "datastore node". <ALEX> done </ALEX> o Section 2 I think you definition of "datastore node" can be made more clear: OLD: Datastore node: An instance of management information in a datastore. Also known as "datastore node". NEW: o datastore node: An instance of a data node in a datastore. NOTE: I think you should stick to the term "datastore node", and not use the term "datastore node". I know that "datastore node" is used quite a lot in this document, but I still think we should change this. (the document also uses the term "data datastore node" and "datastore datastore node", these should be fixed) <ALEX> Updated definition as follows: Datastore node: An instance of a data node in a datastore. In the context of this document sometimes also referred to as "datastore node". I also replaced most occurrences of "object" with "datastore node". (Some instances of "object" remain, mostly in descriptions and names and where replacing "object" with "datastore node" would for some reason or another be awkward.) </ALEX> o Section 2 You have: Datastore node update: A data item containing the current value/ property of a datastore node at the time the datastore node update was created. This definition is not clear to me. The term is used twice in the document so maybe it can be removed. When it is used in section 3.1, it is not clear that you mean a "data item". My suggestion is to remove this term from the terminology list, but use the words "datastore node update" as you already do. <ALEX> We would prefer to keep it. Perhaps remove the "/property" (i.e. just saying "containing the current value of a datastore node ...". The reason to keep it is that at the end of the day, what we send are those updates. </ALEX> o Section 2 You have: Update record: A representation datastore node update(s) resulting from the application of a selection filter for a subscription. An update record will include the value/property of one or more datastore nodes at a point in time. It may contain the update type for each datastore node (e.g., add, change, delete). Also included may be metadata/headers such as a subscription identifier. s/A representation datastore node/A representation of datastore node/ What is "value/property"; first, does "/" mean "and" or "or"? second, what is a "property" of a datastore node? Ditto for "metadata/headers". <ALEX> We updated this as follows, does this work? "Update record: A representation of one or more datastore node updates. In addition, an update record may contain which type of update led to the datastore node update (e.g., whether the datastore node was added, changed, deleted). Also included in the update record may be other metadata, such as a subscription identifier of the subscription as part of which the update record was generated." </ALEX> o Section 2 The term "Update trigger" isn't used in the document. I suggest you remove it. <ALEX> Yes, indeed we only use the word "trigger" throughout the document. However, using "update trigger" is perhaps more precise. Keeping the term, but replacing instances of "trigger" with "update trigger" throughout the document as appropriate. </ALEX> o Section 3 You write: This document specifies a solution for a push update subscription service. I think this should be reworded; what exactly is a "push update subscription service"? <ALEX> Rephrased to "This document specifies a solution that provides subscription service for updates from a datastore. " </ALEX> The you have: This solution supports dynamic as well as configured subscriptions to information updates from datastores. Maybe s/information updates from datastores/updates to datastore nodes/ <ALEX> Rephrased to "This solution supports dynamic as well as configured subscriptions to updates of datastore nodes." </ALEX> o Section 3.1 There are two types of triggers for subscriptions: periodic and on-change. I think you mean that there are two types of subscriptions: There are two types of subscriptions: periodic and on-change. B/c later in the document you use the terms "on-change subscription" and "periodic subscription". I never see "on-change trigger". These terms should be defined in the terminology section, b/c they are central. <ALEX> Rephrased the sentence in question to: "There are two types of subscriptions, distinguished by how updates are triggered: periodic and on-change. " Added the terms "periodic subscription", "on-change subscription", "datastore subscription" to the terminology section. </ALEX> o Section 3.4 The title is "Promise-Theory Considerations". I have pointed this out before; either change the title, or have a reference to the promise-theory you relate to, and explain in more details how you relate to this theory. <ALEX> I removed the reference to this and changed the title to "Reliability Considerations". Giving an intro to promise theory would probably be distracting. </ALEX> Also, this section says: If for some reason the publisher of a subscription is not able to keep its promise, receivers MUST be notified immediately and reliably. Is this intended as a requirement on the server, or on the solution? If it is the former, it shouldn't be defined here, but rather be explicit when the operations are defined (which I think it is). If it is the latter, you should make that clear, and remove the 2119 language. <ALEX> Really, it is both. It is a requirement on the solution, that our solution addresses (and hence by extension servers that implement the solution). Applied some slight rewording to make clear that it is the former: "For this reason, the solution that is defined in this document mandates that a publisher notifies receivers immediately and reliably whenever it encounters a situation in which it is unable to keep the terms of the subscription, and provides the publisher with the option to suspend the subscription in such a case." </ALEX> o Section 3.5.2 It is not clear how the server is supposed to construct the YANG patch record. You have some text about what a client should do, but no instructions for the server. When would a server use the "remove" edit-operation, rather than "delete"? Instead of having special rules for the client, wouldn't it be better to say that the server MUST NOT use "create" and "delete" (instead use "repalce" and "remove"). Then the YANG patch semantics is left intact. <ALEX> I am not sure that it is a good idea to prohibit create and delete. "Remove" would still be okay, I guess (although I am not sure what avoiding delete buys us, though), but for "create", I don't think "replace" expresses the same. In short, my preference would be to keep this as is. As to more server-side instructions, at the risk of making the text a bit redundant, we are adding the following snippet: "A publisher will indicate a change to the effect that a value of a datstore node has been updated by indicating a "replace" operation (applied to the datastore node) in the patch. When a new datastore node was created (other than an element in a list), a publisher will indicate a "create" operation in the patch. When a datastore node was deleted (other than an element in a list), the publisher indicates this by a "delete". When a new list element was created or removed, the publisher indicates it by an "insert" or "remove", respectively. " </ALEX> o Section 3.6 (terminology) OLD: o xpath: An xpath selection filter is an XPath expression that returns a node set. When specified, updates will only come from the selected data nodes. NEW: o xpath: An xpath selection filter is an XPath expression that returns a node set. When specified, updates will only come from the selected datastore nodes. Also, when the word "xpath" is used in general text, it should be "XPath". When it refers to a YANG node/type etc, it should be quoted: "xpath". <ALEX> Done </ALEX> o Section 3.7 In the examples, the XML namespace for RFC 7223 is wrong: s/http://foo.com/ietf-interfaces/ urn:ietf:params:xml:ns:yang:ietf-interfaces/ <ALEX> Done </ALEX> o Section 3.7 You write: Also if used as a counter, the counter MUST be reset to '1' the after passing a maximum value of '99999'. Isn't a max of 65535 or 4294967295 more reasonable? (uint16, or uint32) <ALEX> Ok, let's go with with 4294967295 (uint32) </ALEX> o Section 3.8 OLD: The RPCs defined within [I-D.draft-ietf-netconf-subscribed-notifications] have been enhanced to support datastore subscription negotiation. Included in these enhancements are error codes which can indicate why a datastore subscription attempt has failed. NEW: The RPCs defined within [I-D.draft-ietf-netconf-subscribed-notifications] have been augmented to support datastore subscription negotiation. Also, new error codes that indicates why a datastore subscription attempt has failed have been added. <ALEX> Changed the second sentence to "Also, new error codes have been added that are able to indicate why a datastore subscription attempt has failed" </ALEX> o Section 3.8 You write: o "error-app-tag" with the value being a string that corresponds to an identity with a base of "establish-subscription-error" (for error responses to an establish-subscription request), "modify- subscription-error" (for error responses to a modify-subscription request), "delete-subscription-error" (for error responses to a delete-subscription request), "resynch-subscription-error" (for error responses to resynch-subscription request), or "kill- subscription-error" (for error responses to a kill-subscription request), respectively. This is not how errors are handled in draft-ietf-netconf-subscribed-notifications. Why do you have two different mechanisms? And why is this identity needed; you already have the identity in the "establish-subscription-error-datastore". I think you should decide on one mechanism, and use it in both drafts (specifically, the error-app-tag handling. BTW, *if* you decide to keep it, you need to clarify what "a string that corresponds to" means. Maybe use the JSON encoding of identities in this case (<modname>:<identityname>)). <ALEX> I agree we should have one mechanism and be consistent. Updating this to be in line with subscribed-notifications. I am also renaming the yang data to establish- (respectively modify-) subscription-datatore-error-info to be consistent with the naming conventions in subscribed-notifications. </ALEX> Also, in the example in Figure 4, the "reason" leaf is missing. <ALEX> Struck the example and updated the text with the yang tree for the yang-data instead, so we are consistent with subscribed-notifications now. </ALEX> o Section 3.9 Because of the NACM rules you have here, should this document formally "Update" 6536bis? <ALEX> We don't know what the guidelines here are. You tell us. If we don't have to introduce additional dependencies, that will be preferred by us. </ALEX> o Section 3.10 (clarification) OLD: In some cases, a publisher supporting on-change notifications may not be able to push updates for some datastore node types on-change. NEW: In some cases, a publisher supporting on-change notifications may not be able to detect changes in all datastore nodes. <ALEX> We prefer to keep this as is. This is not just about detection, but also about the fact that changes would simply be too frequent (e.g. generating too much load). </ALEX> o Section 3.11.1 You're using the term "MAY NOT", but this is not a 2119 term (BTW, you need to include the usual 2119 boilerplate) I think you mean "MUST NOT". <ALEX> changed </ALEX> o Section 4 You should remove the text describing the tree diagrams, and instead reference: Tree diagrams used in this document follow the notation defined in [I-D.ietf-netmod-yang-tree-diagrams]. <ALEX> changed </ALEX> o Section 4.1 The tree diagram is quite hard to read. I suggest to prune the tree a bit by removing unnecessary nodes defined in ietf-subscribed-notifications: module: ietf-subscribed-notifications ... +--rw filters | ... | +--rw yp:selection-filter* [identifier] | +--rw yp:identifier sn:filter-id | +--rw (yp:filter-spec)? | +--:(yp:datastore-subtree-filter) | | +--rw yp:datastore-subtree-filter? <anydata> | | {sn:subtree}? | +--:(yp:datastore-xpath-filter) | +--rw yp:datastore-xpath-filter? yang:xpath1.0 | {sn:xpath}? +--rw subscriptions +--rw subscription* [identifier] | ... +--rw (target) | +--:(stream) | | ... | +--:(yp:datastore) | +--rw yp:datastore | | identityref | +--rw (yp:selection-filter)? | +--:(yp:by-reference) | | +--rw yp:selection-filter-ref | | selection-filter-ref | +--:(yp:within-subscription) | +--rw (yp:filter-spec)? | +--:(yp:datastore-subtree-filter) | | +--rw yp:datastore-subtree-filter? | | <anydata> {sn:subtree}? | +--:(yp:datastore-xpath-filter) | +--rw yp:datastore-xpath-filter? | yang:xpath1.0 {sn:xpath}? | ... +--rw (yp:update-trigger)? +--:(yp:periodic) | +--rw yp:periodic! | +--rw yp:period yang:timeticks | +--rw yp:anchor-time? yang:date-and-time +--:(yp:on-change) {on-change}? +--rw yp:on-change! +--rw yp:dampening-period? yang:timeticks +--rw yp:no-synch-on-start? empty +--rw yp:excluded-change* change-type (This was generated with the --tree-line-length 69 option to pyang, then manually edited to add the "...") But I also think that the tree diagram is difficult to read b/c you include everything in one single diagram. I suggest you put the config part of the diagram (shown above) in section 4.2, then a tree diagram snippet per notification in sections 4.3.1 and 4.3.2, and rpc tree snippets in 4.4.*. <ALEX> Snipped the tree substantially, in essence pruning all items from subscribed-notifications that were not required for structure. (Hopefully we don't need to regenerate it at some later point...) </ALEX> o Section 4.2 (editorial) OLD: Both configured and dynamic subscriptions are represented within the list subscription. New and enhanced parameters extending the basic subscription data model in [I-D.draft-ietf-netconf-subscribed-notifications] include: NEW: Both configured and dynamic subscriptions are represented within the list "subscription". New parameters extending the basic subscription data model in [I-D.draft-ietf-netconf-subscribed-notifications] include: <ALEX> OK </ALEX> o Section 4.2 You write: o For periodic subscriptions, triggered updates will occur at the boundaries of a specified time interval. These boundaries many be calculated from the periodic parameters: s/many/may/ or s/many be/are/ <ALEX> substituted it with "can be" </ALEX> o Section 4.2 You write: * a "period" which defines duration between period push updates. s/period push updates/push updates/ <ALEX> OK </ALEX> o Section 4.3.1 You write: Subscription state notifications and mechanism are reused from [I-D.draft-ietf-netconf-subscribed-notifications]. Some have been augmented to include the datastore specific objects. Instead of writing "some have been augmented", be explicit and list the ones that have been updated (I think it is just two). <ALEX> OK, changed to "Notifications "subscription-started" and "subscription-modified" have been augmented to include the datastore specific objects." </ALEX> o Section 4.3.2 The first paragraph: OLD: Along with the subscribed content, there are other objects which might be part of a "push-update" or "push-change-update" NEW: Along with the subscribed content, there are other objects which might be part of a "push-update" or "push-change-update" notifications. <ALEX> OK (added "notification") </ALEX> o Section 4.3.2 (terminology) OLD: An "incomplete-update" datastore node. This datastore node indicates that not all NEW: An "incomplete-update" leaf. This leaf indicates that not all <ALEX> OK </ALEX> o Section 4.4.1 The XML example is not correct. Please ensure that all examples are validated by some tool. Also I suggest you ensure the examples are consistently indented, and that they use "xmlns" consistently. As it looks a bit messy. In this case, there's an element <yp:source> in the XML that doesn't exist in the datamodel; the element <xpath-filter> is placed within a leaf, and incorrectly named (it should be <yp:datastore-xpath-filter>); and it uses an incorrect "select" attribute. <ALEX> We will update this. </ALEX> o Section 4.4.1 You write: The publisher MUST respond explicitly positively (i.e., subscription accepted) or negatively (i.e., subscription rejected) to the request. (similar text in a couple of other places as well) What exactly does this tell me? What does "explicitly" mean? Is this telling me that the server MUST NOT contain any bugs? I suggest you remove this sentence (and the other similar ones). <ALEX> Removed extra text </ALEX> o Section 5 (fix syntax so that extraction tools work properly) OLD: <CODE BEGINS>; file "ietf-yang-push@2018-02-23.yang" NEW: <CODE BEGINS> file "ietf-yang-push@2018-02-23.yang" <ALEX> removed the ";" </ALEX> In general, the module is quite hard to understand, due to the usage of all groupings. <ALEX> Well, we want to reuse definitions. </ALEX> o rpc resynch-subscription description "This RPC allows a subscriber of an active on-change subscription to request a full push of objects in their current state. A successful result would invoke a push-update of all datastore objects that the subscriber is permitted to access. This request may only come from the same subscriber using the establish-subscription RPC."; Consider removinging the words "in their current state"; a 'push-update' always send nodes in their current state. <ALEX> OK </ALEX> Rephrase "invoke a push-update"; maybe :result in the publisher sending a 'push-update' notification". <ALEX> rephrased to "A successful invocation results in a push-update of all datastore objects that the subscriber is permitted to access. " </ALEX> Rephrase the last sentence; you probably mean that it must be sent on the same session where the subscription was established. <ALEX> rephrased to "This RPC can only be invoked on the same session on which the subscription was established (using an establish-subscription RPC)." </ALEX> Also, include text in the rpc description that explains that the resynch-subscription-error is sent on error. <ALEX> added "In case of an error, a resynch-subscription-error is sent as part of an error response." </ALEX> o error identities When reading the YANG module, it is not really clear how these identities are supposed to be used. For example, one of the first identities is: identity cant-exclude { base sn:establish-subscription-error; description "Unable to remove the set of 'excluded-changes'. This means the publisher is unable to restrict 'push-change-update's to just the change types requested for this subscription."; } Since this is derived from sn:establish-subscription-error, it seems that the idea is to use it anywhere an sn:establish-subscription-error is expected, e.g., as the "reason" in "establish-subscription-error-stream". But this is probably not true? I think that since you define new yang-data structure for errors related to push-subscriptions, you shouldn't use the sn:establish-subscription-error at all; instead you should define a new set of base identities in this module. <ALEX> The identities are used by the server in a response. A correct server implementation will need to pick the correct reason, regardless of where the identities are defined. Clients are only going to to read those. Since we are still using the same RPC and augmenting it, we prefer to have the relationship also reflected by the identities. Of course, we could have an entirely different set of RPCs, one to manage stream subscriptions, the other to manage datastore subscriptions. In that case, having separate identities would be appropriate. However, that runs counter to the decisions we made earlier as a working group, that one should be a generalization of the other. For those reasons, we prefer to leave this item unchanged. </ALEX> o typedef change-type The description in this type talks about "data resource" (see my comment on terminology above). The description doesn't quite match how this type is used. For example: enum "create" { description "Create a new data resource if it does not already exist. If it already exists, replace it."; } This type is used only in "excluded-change", and as such it is used to inform the server about which changes the client doesn't want. So shouldn't the description text describe this? Something like this for "create": "Instructs the server that if a datastore node is created, it should not trigger an update." <ALEX> Updating the descriptions: create: "A change that refers to the creation of a new data node." delete: "A change that refers to the deletion of a data node." insert: "A change that refers to the insertion of a new user-ordered data node." merge: "A change that refers to a merging of a new value with a target data node." move: "A change that refers to a reordering of the target data node" replace: "A change that refers to a replacement of the target data node's value." remove: "A change that refers to the removal of a data node." </ALEX> Maybe also change the name of the typedef to "excluded-change-type". See also my comment on 3.5.2 above. <ALEX> On changing the name of the typed: The type is currently only used in excluded-change, that is correct. However, when you look at the enum, this really just defines the change types. Perhaps the same typedef might be used in a different capacity elsewhere. I don't feel strongly about this, however, I think the name of the typedef is in fact appropriate. </ALEX> o leaf datastore-xpath-filter In the description, do: s/'xpath-filter' leaf/'datastore-xpath-filter' leaf/ <ALEX> OK </ALEX> o list selection-filter The description says: "A list of pre-positioned filters that can be applied to datastore subscriptions."; What is a "pre-positioned" filter? I suggest: "A list of filters that can be applied to datastore subscriptions."; (I just noticed that the same words exists in the subscribed-notifications document; I suggest the same change is applied there as well.) <ALEX> Rephrased "pre-positioned" to "pre-configured" (I think we wanted to write "pre-provisioned" at first, but mistyped...) </ALEX> o leaf selection-filter/identifier This has: leaf identifier { type sn:filter-id; I think this is a bit over-engineered. I suggest to simply use "type string" here. And the same in ietf-subscribed-notifications; the type filter-id should be removed. Also, I think all existing modules have followed some kind of unspoken convention that arbitrary named list entries have a key called "name" of type string. When there's some kind of restricted identifier used, the key leaf is called "id" or "<foo>-id" (e.g. "router-id", "session-id" etc). <ALEX> I liked the leafref as it makes it harder to violate referential integrity than with a string, but ok. Renamed it to "filter-id". </ALEX> o notifications push-update and push-change-update Both these have this text: This notification shall only be sent to receivers of a subscription; it does not constitute a general-purpose notification. Why do you have this text? It seems to imply that it would be illegal for a server to ever send these notifs on some "stream". Why is this necessary? If I have a clever use case for this, why is it made illegal? <ALEX> rephrased to "It does not constitute a general-purpose notification that would be subscribable as part of the NETCONF event stream by any receiver." </ALEX> o anydata datastore-contents in notification push-update description "This contains the updated data. It constitutes a snapshot at the time-of-update of the set of data that has been subscribed to. The format and syntax of the data corresponds to the format and syntax of data that would be returned in a corresponding get operation with the same selection filter parameters applied."; This description is not precise enough. What is "a corresponding get operation"? Does this text mean that I will get different contents if I use NETCONF than if I use RESTCONF? <ALEX> rephrased as follows: " description "This contains the updated data. It constitutes a snapshot at the time-of-update of the set of data that has been subscribed to. The snapshot corresponds to the same snapshot that would be returned in a corresponding get operation with the same selection filter parameters applied.";" </ALEX> o anydata datastore-changes in notification push-change-update You write: "This contains the set of datastore changes needed to update a remote datastore starting at the time of the previous update, per the terms of the subscription. Isn't this backwards? Shouldn't it be: "This contains the set of datastore changes of the target datastore starting at the time of the previous update, per the terms of the subscription. With the current text, it seems the server must have knowledge about the remote datastore (if it even exists). <ALEX> Thank you, updated </ALEX> Further, it says: Changes are encoded analogous to the syntax of a corresponding yang- patch operation, i.e. a yang-patch operation applied to the datastore implied by the previous update to result in the current state. I don't understand what this text says. Looking at the example, I think that what you really should do is remove that sentence, and change this "anydata" node to a container: container datastore-changes { uses ypatch:yang-patch; ... } With this approach, you can even "refine" the description statement of some nodes (e.g. the "operation" leaf) and explain the create/delete semantics. This node also has this reference: reference "RFC 8072 section 2.5, with a delta that it is ok to receive ability create on an existing node, or receive a delete on a missing node."; The reference statement is just supposed to contain the formal reference. If descriptive text is needed it should go into the "description". But in this case I think you should simply remove the text in the reference. <ALEX> Since we are using this only in the push-change-update, we prefer to leave it as is. We removed the reference statement and updated the desciption to now read as follows: anydata datastore-changes { description "This contains the set of datastore changes of the target datastore starting at the time of the previous update, per the terms of the subscription. The datastore changes are encoded per RFC 8027 (YANG Patch)."; } </ALEX> o augment "/sn:subscription-modified" description "This augmentation adds many datastore specific objects to the notification that a subscription has been modified."; Please be more specific than "adds many nodes". How many is many? <ALEX> removed "many" </ALEX>. Which nodes is provided through the "uses" statement; there is no need to expand it in the description. </ALEX> o augment "/sn:subscription-modified/sn:target" case datastore { uses datastore-criteria { refine "selection-filter/within-subscription" { description "Specifies where the selection filter, and where it came from within the subscription and then populated within this notification. This sentence doesn't parse. <ALEX> Rephrased as follows: "Specifies the selection filter and where it originated from." </ALEX> o The YANG module is inconsistently indented. To get some help, you can use the latest version of pyang on github and run: $ pyang -f yang --keep-comments ietf-yang-push.yang > x $ diff ietf-yang-push.yang x <ALEX> Thanks. Updated. </ALEX> /martin _______________________________________________ Netconf mailing list Netconf@ietf.org https://www.ietf.org/mailman/listinfo/netconf
- Re: [Netconf] mbj's YANG doctor's review and WGLC… Martin Bjorklund
- Re: [Netconf] mbj's YANG doctor's review and WGLC… Kent Watsen
- [Netconf] mbj's YANG doctor's review and WGLC com… Martin Bjorklund
- Re: [Netconf] mbj's YANG doctor's review and WGLC… Alexander Clemm