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- 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 "object". NEW: o datastore node: An instance of management information in a datastore. Also known as "object". Also, in one place, you use the term "data resource". Either import that term from the proper document, or change to "datastore node". 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 "object". 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 "object". I know that "object" is used quite a lot in this document, but I still think we should change this. (the document also uses the term "data object" and "datastore object", these should be fixed) 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. 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". o Section 2 The term "Update trigger" isn't used in the document. I suggest you remove it. 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"? 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/ 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. 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. 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. 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. 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". 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/ 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) 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. 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 (:)). Also, in the example in Figure 4, the "reason" leaf is missing. o Section 3.9 Because of the NACM rules you have here, should this document formally "Update" 6536bis? o Section 3.10 (clarification) OLD: In some cases, a publisher supporting on-change notifications may not be able to push updates for some object types on-change. NEW: In some cases, a publisher supporting on-change notifications may not be able to detect changes in all datastore nodes. 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". 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]. 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? | | {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? | | {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.*. 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: 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/ o Section 4.2 You write: * a "period" which defines duration between period push updates. s/period push updates/push updates/ 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). 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. o Section 4.3.2 (terminology) OLD: An "incomplete-update" object. This object indicates that not all NEW: An "incomplete-update" leaf. This leaf indicates that not all 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 in the XML that doesn't exist in the datamodel; the element is placed within a leaf, and incorrectly named (it should be ); and it uses an incorrect "select" attribute. 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). o Section 5 (fix syntax so that extraction tools work properly) OLD: ; file "ietf-yang-push@2018-02-23.yang" NEW: file "ietf-yang-push@2018-02-23.yang" In general, the module is quite hard to understand, due to the usage of all groupings. 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. Rephrase "invoke a push-update"; maybe :result in the publisher sending a 'push-update' notification". Rephrase the last sentence; you probably mean that it must be sent on the same session where the subscription was established. Also, include text in the rpc description that explains that the resynch-subscription-error is sent on error. 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. 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." Maybe also change the name of the typedef to "excluded-change-type". See also my comment on 3.5.2 above. o leaf datastore-xpath-filter In the description, do: s/'xpath-filter' leaf/'datastore-xpath-filter' leaf/ 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.) 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 "-id" (e.g. "router-id", "session-id" etc). 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? 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? 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). 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. 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? 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. 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 /martin