Re: [Netconf] mbj's WGLC review of yang-push-17
Martin Bjorklund <mbj@tail-f.com> Tue, 28 August 2018 08:09 UTC
Return-Path: <mbj@tail-f.com>
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 8553A130E54 for <netconf@ietfa.amsl.com>; Tue, 28 Aug 2018 01:09:26 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001, 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 TPT4L0sDc8mk for <netconf@ietfa.amsl.com>; Tue, 28 Aug 2018 01:09:21 -0700 (PDT)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id 76141130E45 for <netconf@ietf.org>; Tue, 28 Aug 2018 01:09:21 -0700 (PDT)
Received: from localhost (unknown [173.38.220.61]) by mail.tail-f.com (Postfix) with ESMTPSA id 58F6A1AE0449; Tue, 28 Aug 2018 10:09:18 +0200 (CEST)
Date: Tue, 28 Aug 2018 10:09:17 +0200
Message-Id: <20180828.100917.925597025431754922.mbj@tail-f.com>
To: alexander.clemm@huawei.com
Cc: netconf@ietf.org, evoit@cisco.com
From: Martin Bjorklund <mbj@tail-f.com>
In-Reply-To: <644DA50AFA8C314EA9BDDAC83BD38A2E0EB59E44@sjceml521-mbs.china.huawei.com>
References: <20180815.131758.1464388348783195997.mbj@tail-f.com> <644DA50AFA8C314EA9BDDAC83BD38A2E0EB59E44@sjceml521-mbs.china.huawei.com>
X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Text/Plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/6x_oc77vOSm2Qp05Pcz1kZoEplk>
Subject: Re: [Netconf] mbj's WGLC review of yang-push-17
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.27
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: Tue, 28 Aug 2018 08:09:27 -0000
Hi, Thanks for addressing my comments. Some replies inline. Alexander Clemm <alexander.clemm@huawei.com> wrote: > Hi Martin, > > thank you for your comments. > > Please see my responses inline, <ALEX> > > (Apologies for the sluggish response; I have been travelling) > > --- Alex > > > -----Original Message----- > > From: Netconf [mailto:netconf-bounces@ietf.org] On Behalf Of Martin > > Bjorklund > > Sent: Wednesday, August 15, 2018 4:18 AM > > To: netconf@ietf.org > > Subject: [Netconf] mbj's WGLC review of yang-push-17 > > > > Hi, > > > > Kent Watsen <kwatsen@juniper.net> wrote: > > > This message starts a Last Call on draft-ietf-netconf-yang-push-17: > > > > I have reviewed this document, and I think it is almost ready. Here > > are my > > comments: > > > > > > o 3.3 > > > > The text says: > > > > Putting it all together, following is the conceptual process for > > creating an push-change-update notification: > > > > Up until this point, the text has just talked about "update > > record". Here it says "an push-change-update notification". > > > > Maybe simply s/an push-change-update notification/an update record/? > > > > <ALEX> The process itself talk also about sending the record to the > receiver. How about refining it to say " following is the conceptual > process for > creating a update record as part of an on-change subscription:"? > </ALEX> Ok. (s/a update/an update/) > > o 3.4 > > > > The text says: > > > > 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. > > > > I think it would help if you could put a forward reference to the > > mechanism that exists to do this immediate notification. > > > > <ALEX> Adding the following forward reference: > "This is described further in Section 3.11.1." Ok. > > > > > o 3.5.2 > > > > The text describes incorrect usage of the "insert" operation; it is > > only applicable to user ordered lists. Also, there is no reason for > > special handling of deletion of list entries. (also use the term > > "list entry" rather than "element"). Hence, I suggest: > > > > OLD: > > > > 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. > > > > NEW: > > > > 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 entry in a user ordered list), a > > publisher will indicate a "create" operation in the patch. When a > > datastore node was deleted, the publisher indicates this by a > > "delete". When a new entry in a user ordered list was created, the > > publisher indicates this by an "insert" operation. > > > > <ALEX> OK, changed. </ALEX> > > > > > o 3.5.2 > > > > In the last paragraph, s/"merge"/"replace"/ > > since the paragraph before just describes "replace", not "merge". > > > > <ALEX> done </ALEX> > > > Shouldn't the text also mention the operation "move" for user > > ordered lists? > > > <ALEX> Thank you. Added the following text: " When an entry in a > user-ordered list was moved, the publisher indicates this by a "move" > operation. " Ok. > > o 3.6 > > > > s/Xpath/XPath/ > > <ALEX> done </ALEX> > > > > > > > o 3.7 > > > > The examples are not quite correct, and I suggest they are modified > > to not include deprecated nodes: > > > > OLD: > > > > <notification xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0"> > > <eventTime>2017-10-25T08:00:11.22Z</eventTime> > > <push-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > <subscription-id>1011</subscription-id> > > <datastore-contents> > > <interfaces-state xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"> > > <interface> > > <name>eth0</name> > > <oper-status>up</oper-status> > > </interface> > > </interfaces-state> > > </datastore-contents> > > </push-update> > > </notification> > > > > NEW: > > > > <notification xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0"> > > <eventTime>2017-10-25T08:00:11.22Z</eventTime> > > <push-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > <subscription-id>1011</subscription-id> > > <datastore-contents> > > <interfaces xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"> > > <interface> > > <name>eth0</name> > > <oper-status>up</oper-status> > > </interface> > > </interfaces> > > </datastore-contents> > > </push-update> > > </notification> > > > > > > OLD: > > > > <notification xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0"> > > <eventTime>2017-10-25T08:22:33.44Z</eventTime> > > <push-change-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang- > > push"> > > <subscription-id>89</subscription-id> > > <datastore-changes> > > <yang-patch xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-patch"> > > <patch-id>1</patch-id> > > <edit> > > <edit-id>edit1</edit-id> > > <operation>merge</operation> > > <target>/ietf-interfaces:interfaces-state</target> > > <value> > > <interfaces-state xmlns="http://foo.com/ietf-interfaces"> > > <interface> > > <name>eth0</name> > > <oper-status>down</oper-status> > > </interface> > > </interfaces-state> > > </value> > > </edit> > > </yang-patch> > > </datastore-changes> > > </push-change-update> > > </notification> > > > > NEW: > > > > <notification xmlns="urn:ietf:params:xml:ns:netconf:notification:1.0"> > > <eventTime>2017-10-25T08:22:33.44Z</eventTime> > > <push-change-update xmlns="urn:ietf:params:xml:ns:yang:ietf-yang- > > push"> > > <subscription-id>89</subscription-id> > > <datastore-changes> > > <yang-patch xmlns="urn:ietf:params:xml:ns:yang:ietf-yang-patch"> > > <patch-id>1</patch-id> > > <edit> > > <edit-id>edit1</edit-id> > > <operation>replace</operation> > > <target>/ietf-interfaces:interfaces</target> > > <value> > > <interfaces xmlns="urn:ietf:params:xml:ns:yang:ietf-interfaces"> > > <interface> > > <name>eth0</name> > > <oper-status>down</oper-status> > > </interface> > > </interfaces> > > </value> > > </edit> > > </yang-patch> > > </datastore-changes> > > </push-change-update> > > </notification> > > > > (uses the "replace" operation, fixed xml namespace, don't use > > interfaces-state) > > > > <ALEX> Thank you. Updated. </ALEX> > > > > > o 3.8 > > > > s/establish-subscription-datasore-error-info/ > > establish-subscription-datastore-error-info/ > > > > <ALEX> Fixed, thank you </ALEX> > > > > > o 3.8 > > > > The text says: > > > > In the case of a rejected request for an establishment of a datastore > > subscription, the hints MUST be transported within a yang-data > > "establish-subscription-datastore-error-info" container > > > > Do you mean that *if* there are hints, they MUST be sent within this > > container, or you mean that on failure, this container MUST be sent? > > > > (ditto for modify-subscription-datastore-error-info) > > > > (it seems 4.4.1 says that hints SHOULD be included, but Appendix A > > that they MUST be included) > > > > <ALEX> Changing "MUST" to "SHOULD". And yes, *if* there are hints. I > don't think a change to Appendix A, is needed; it does not actually > state that they must be included. (It says the yang-data with the > hint MUST be sent if hints are included.) So then it seems that this text (3.8) says that if there are hints, they SHOULD be transported witin the "establish-subscription-datastore-error-info" container, but the appendix says that if there are hints they MUST be sent within the "establish-subscription-datastore-error-info" container. I would prefer to have the normative language in just one place (probably not in the appendix). But I also think that the current text in the appendix is more clear, since it says that *if* there are hints, they MUST be transported... > </ALEX> > > > > > > o 3.9 > > > > s/RFC8342/RFC8341/ > > s/rfc6536bis/RFC8341/ > > > > <ALEX> Thank you, changed </ALEX> > > > > > o 3.9 > > > > It is difficult to relate Figure 5 to the text. Should it be moved > > to right after the first paragraph? I think it would be useful to > > add a reference from the text that examplains the "updated access > > control rules" to the figure. > > > > <ALEX> I moved the paragraph preceding Figure 5, as well as Figure 5 > itself, further to the top (behind the first paragraph). </ALEX> > > > Also, s/update message/update record/ in the figure. > > > > <ALEX> Done </ALEX> > > > > > o 3.9 > > > > The text says: > > > > A publisher MAY choose reject an establish-subscription request which > > selects non-existent or access-protected data. In addition, a > > publisher MAY choose to terminate a dynamic subscription or suspend a > > configured receiver when the authorization privileges of a receiver > > change, or the access controls for subscribed objects change. Such a > > capability enables the publisher to avoid having to support a > > continuous, and total filtering of an entire subscription's content. > > > > In these cases above, the error identity "unchanging-selection" > > SHOULD be returned. > > > > "the cases above" refers to (i) terminating a dynamic subscription, > > or (ii) suspend a configured receiver. What does it mean to > > "return" an error identity when a subscription is terminated, or > > suspended? > > > > Maybe you meant that the error identity "unchanging-selection" > > SHOULD be sent in an "subscription-terminated" notification or > > "subscription-suspended" notification, respectively. > > > > If so, the "unchanging-selection" identity should probably also > > derive from "sn:subscription-suspended-reason". > > > > <ALEX> Changed this section as follows: > "A publisher MAY choose to reject an establish-subscription request > which selects non-existent or access-protected data. In addition, a > publisher MAY choose to terminate a dynamic subscription or suspend a > configured receiver when the authorization privileges of a receiver > change, or the access controls for subscribed objects change. As > reason, the error identity "unchanging-selection" SHOULD be > returned. Such a capability enables the publisher to avoid having to > support continuous and total filtering of a subscription's content for > every update record. It also reduces the possibility of leakage of > access-controlled objects." > </ALEX> This new text doesn't address my concern, which is the usage of the term "return". How can a server "return" anything when a subscription is terminated? > <ALEX> On a separate note, the next paragraph states: "If read access > into previously accessible nodes has been lost due to a receiver > permissions change, this SHOULD be reported as a patch "delete" > operation for on-change subscriptions. If not capable of handling such > receiver permission changes with such a "delete", publisher > implementations MUST force dynamic subscription re-establishment or > configured subscription re-initialization so that appropriate > filtering is installed." > > I am wondering if one should actually report the "delete"-operation > here. This is somewhat inaccurate, as the object may not have been > actually deleted, only its access has been revoked. I am leaving this > as is for now, but am wondering if we should change this instead as > follows: > " If read access into previously accessible nodes has been lost due to > a receiver permissions change, publisher implementations MUST force > dynamic subscription re-establishment or configured subscription > re-initialization so that appropriate filtering is installed." > > Thoughts? Ok with me. > </ALEX> > > > > > o 3.11.1 > > > > The text says: > > > > It is not > > required to merge pending update messages. > > > > This can be read as indicating that a server MAY merge pending > > update messages. I assume that it should say that pending update > > messages MUST NOT be merged. > > <ALEX> Hmm. I am not sure I agree. The server is not required to > merge pending update messages - i.e. can send multiple messages each > with a separate update record. However, there is no reason to > preclude that they could be combined. So, I don't think an update is > needed here. > </ALEX> So you say that the text means that the server MAY merge pending update messages in this case? If so, I think you should update the text so that this is clear. > > Also, s/update message/update record/ > > > > <ALEX> Sure </ALEX> > > > > > o 4.2 > > > > s/an "excluded-change" flag/an "excluded-change" parameter/ > > > > (it is more than just a flag...) > > > > <ALEX> Updated </ALEX> > > > > > o 4.3.2 > > > > The second paragraph is a bit confusing. I suggest to simplify: > > > > OLD: > > > > A "subscription-id" MUST be transported along with the subscribed > > contents. An [RFC5277] Section 4 one-way notification MAY be used > > for encoding updates. Where it is, the relevant "subscription-id" > > MUST be encoded as the first element within each "push-update" or > > "push-change-update". This allows a receiver to differentiate which > > subscription resulted in a particular push. > > > > NEW: > > > > A "subscription-id" is transported along with the subscribed > > contents. This allows a receiver to differentiate which > > subscription resulted in a particular push. > > > > <ALEX> Updated </ALEX> > > > > > o 4.4.1 > > > > The examples are (still) wrong. > > > > OLD: > > > > <establish-subscription > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications" > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > <yp:datastore> > > <yp:source xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores"> > > ds:operational > > </yp:source> > > <xpath-filter > > xmlns:ex="http://example.com/sample-data/1.0" > > select="/ex:foo"/> > > </yp:datastore> > > <yp:periodic> > > <yp:period>500</yp:period> > > </yp:periodic> > > </establish-subscription> > > > > NEW: > > > > <establish-subscription > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications" > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > <yp:datastore xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores"> > > ds:operational > > </yp:datastore> > > <yp:datastore-xpath-filter xmlns:ex="http://example.com/sample- > > data/1.0"> > > /ex:foo > > </yp:datastore-xpath-filter> > > <yp:periodic> > > <yp:period>500</yp:period> > > </yp:periodic> > > </establish-subscription> > > > > > > > > OLD: > > > > <rpc-reply message-id="101" > > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> > > <subscription-result > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"> > > ok > > </subscription-result> > > <identifier > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"> > > 52 > > </identifier> > > </rpc-reply> > > > > NEW: > > > > <rpc-reply message-id="101" > > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> > > <identifier > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"> > > 52 > > </identifier> > > </rpc-reply> > > > > > > OLD: > > > > <netconf:rpc message-id="101" > > xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0"> > > <establish-subscription > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications" > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > <yp:datastore > > xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores"> > > ds:operational > > </yp:datastore> > > <yp:datastore-xpath-filter netconf:type="xpath" > > xmlns:ex="http://example.com/sample-data/1.0"> > > /ex:foo > > </yp:datastore-xpath-filter> > > <yp:on-change> > > <yp:dampening-period>100</yp:dampening-period> > > </yp:on-change> > > </establish-subscription> > > </netconf:rpc> > > > > NEW: > > > > <netconf:rpc message-id="101" > > xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0"> > > <establish-subscription > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications" > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > <yp:datastore > > xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores"> > > ds:operational > > </yp:datastore> > > <yp:datastore-xpath-filter > > xmlns:ex="http://example.com/sample-data/1.0"> > > /ex:foo > > </yp:datastore-xpath-filter> > > <yp:on-change> > > <yp:dampening-period>100</yp:dampening-period> > > </yp:on-change> > > <establish-subscription> > > </netconf:rpc> > > > > > > <ALEX> Updated </ALEX> > > > > > > > o 4.4.1 > > > > REMOVE: > > > > o "error-app-tag" with the value being a string that corresponds to > > an identity with a base of "establish-subscription-error". > > > > (this app-tag thing was removed from subscribed-notifications) > > > > > > And modify the example accordingly: > > > > OLD: > > > > <rpc-reply message-id="101" > > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> > > <rpc-error> > > <error-type>application</error-type> > > <error-tag>operation-failed</error-tag> > > <error-severity>error</error-severity> > > <error-app-tag> > > on-change-unsupported > > </error-message> > > <error-path > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"> > > /yp:periodic/yp:period > > </error-path> > > </rpc-error> > > </rpc-reply> > > > > NEW: > > > > <rpc-reply message-id="101" > > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0"> > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"> > > <rpc-error> > > <error-type>application</error-type> > > <error-tag>operation-failed</error-tag> > > <error-severity>error</error-severity> > > <error-path>/yp:periodic/yp:period</error-path> > > <error-info> > > <yp:establish-subscription-error-datastore> > > <yp:reason>yp:on-change-unsupported</yp:reason> > > </yp:establish-subscription-error-datastore> > > </rpc-error> > > </rpc-reply> > > > > > > <ALEX> I think there are some mismatches in the NEW as indicated above > (no error-info closing bracket, xmln:yp without container) - updated > as follows (NEW2): > > <rpc-reply message-id="101" > xmlns="urn:ietf:params:xml:ns:netconf:base:1.0" > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications"> > <rpc-error> > <error-type>application</error-type> > <error-tag>operation-failed</error-tag> > <error-severity>error</error-severity> > <error-path>/yp:periodic/yp:period</error-path> > <error-info> > <yp:establish-subscription-error-datastore> > <yp:reason>yp:on-change-unsupported</yp:reason> > </yp:establish-subscription-error-datastore> > </error-info> > </rpc-error> > </rpc-reply> > > </ALEX> > > > > o 4.4.2 > > > > The example is wrong: > > > > OLD: > > > > <netconf:rpc message-id="102" > > xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0"> > > <modify-subscription > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications" > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > <identifier>1011</identifier> > > <yp:datastore > > xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores"> > > ds:operational > > </yp:datastore> > > <yp:datastore-xpath-filter > > netconf:type="xpath" xmlns:ex="http://example.com/sample-data/1.0"> > > /ex:bar > > </yp:datastore-xpath-filter> > > <yp:periodic> > > <yp:period>250</yp:period> > > </yp:periodic> > > </modify-subscription> > > </netconf:rpc> > > > > NEW: > > > > <netconf:rpc message-id="102" > > xmlns:netconf="urn:ietf:params:xml:ns:netconf:base:1.0"> > > <modify-subscription > > xmlns="urn:ietf:params:xml:ns:yang:ietf-subscribed-notifications" > > xmlns:yp="urn:ietf:params:xml:ns:yang:ietf-yang-push"> > > <identifier>1011</identifier> > > <yp:datastore > > xmlns:ds="urn:ietf:params:xml:ns:yang:ietf-datastores"> > > ds:operational > > </yp:datastore> > > <yp:datastore-xpath-filter > > xmlns:ex="http://example.com/sample-data/1.0"> > > /ex:bar > > </yp:datastore-xpath-filter> > > <yp:periodic> > > <yp:period>250</yp:period> > > </yp:periodic> > > </modify-subscription> > > </netconf:rpc> > > > > <ALEX> Done </ALEX> > > > > o 4.4.2 > > > > REMOVE: > > > > o "error-app-tag" with the value being a string that corresponds to > > an identity with a base of "modify-subscription-error". > > > > <ALEX> Done </ALEX> > > > > > o 4.4.5 > > > > s/YANG 1.0/YANG 1/ > > > > (the version is "1", not "1.0") > > > > > <ALEX> Done </ALEX> > > > > > o 4.4.5 > > > > I suggest you remove the sentence: > > > > The > > "/modules-state/module-set-id" leaf in the "ietf-yang-library" module > > can be used to cache the YANG library information. > > > > This is chnaged with yang-library-bis, and the sentence is not > > really needed in this draft. > > > > <ALEX> Agreed, removed </ALEX> > > > > > o 4.4.5 > > > > I don't understand what the third paragraph is supposed to tell me. > > Can it be removed? > > > > > > <ALEX> Greatly simplified and rephrased it as follows: > " The set of modules, revisions, features, and deviations can change > at run-time (if supported by the publisher implementation). For this > purpose, the YANG library provides a simple "yang-library-change" > notification that informs the subscriber that the library has changed. > In this case, a subscription may need to be updated to take the > updates into account. The receiver may also need to be informed of > module changes in order to process updates regarding datastore nodes > from changed modules correctly." > </ALEX> Ok. > > o 5 > > > > In subscribed-notifications, the subscription identifier leaf is > > called "identifier", in this model it is called "subscription-id" > > and "identifier". > > > > I think the two models should use the same term. Either change this > > model, or subscribed notifications. Remember to update the > > examples. > > > > <ALEX> I think what you refer to is the way that subscriptions are > identified in push update notifications. In all other places, we > consistently use subscription-id as the type, and "identifier" to > identify a subscription (YANG-Push does not introduce a new leaf, but > imports yp:identifier). > > Personally I find the term "identifier" a bit generic. That said, if > you want to make it consistent, we should stick with what we have in > subscribed notifications (to keep the impact low). I am not really > convinced it is a change for the better, but I am updating the model > to use "identifier" in the push update notifications (push-update and > push-change-update) to identify subscriptions. Examples have been > updated accordingly. > > </ALEX> Ok. (I agree that "subscription-id" is more descriptive...) > > o 5 > > > > Is it ok to do: > > > > <establish-subscription> > > <datastore>operational</datastore> > > </establish-subscription> > > > > Probably not, so I suggest making this illegal in the model: > > > > augment "/sn:establish-subscription/sn:input" { > > when "sn:target/yp:datastore"; // NEW statement > > > > description > > "This augmentation adds additional subscription parameters that > > apply specifically to datastore updates to RPC input."; > > uses update-policy; > > } > > > > and ditto for all of: > > > > augment "/sn:modify-subscription/sn:input" { > > augment "/sn:subscription-started" { > > augment "/sn:subscription-modified" { > > augment "/sn:subscriptions/sn:subscription" { > > > > > > and then modify update-policy-modifiable to make the update-trigger > > choice mandatory: > > > > grouping update-policy-modifiable { > > description > > "This grouping describes the datastore specific subscription > > conditions that can be changed during the lifetime of the > > subscription."; > > choice update-trigger { > > mandatory true; // NEW > > > > > > <ALEX> I am not sure I understand this comment fully. Can you please > clarify? You should be able to subscribe to updates to <operational>. > I don't think this should be precluded. > I guess you are referring to the fact that you would like to make > update-trigger mandatory? Yes. > Arguably, the server should always be able > to reject a request it cannot fulfill, which may include > underspecification. Sure, but if a parameter is mandatory it is better to mark it as such instead of letting implementors figuring this out on their own. > Let me add your suggested augmentation to the inputs for > establish-subscription and modify-subscription. I don't think this > would be needed for a notification, which is generated by the server > anyway. Let me also add the mandatory statement to the grouping; > however, I am not sure this is actually legal, as the grouping will be > used in an augmentation. (Can an augmentation contain a "mandatory" > item?) Yes, in YANG 1.1, and it was added specifically for use cases like this, where the mandatory node are "protected" by a "when" expression. > </ALEX> > > > > > o 5 > > > > I have made this comment before. The anydata node > > "datastore-changes" should be a container that uses the grouping > > "yang-patch". It is more precise than using anydata and in text > > explain that the opaque anydata must be yang patch. > > > > <ALEX> ok </ALEX> > > > > > > o 5 > > > > identity result-too-big { > > > > identity synchronization-size { > > > > > > Why do we need both these errors? Can't we just have a single one, > > maybe "update-too-big"? > > > > (I think that *result-too-big* is a misnomer. If the result of an > > rpc is too big, the standard error-tag "too-big" should be used.) > > <ALEX> Renamed "result-too-big" to "update-too-big". However, leaving > the synchronization-size identity. That one would be used for resynch > only. While strictly speaking it may be possible to eliminate, it > probably doesn't hurt either. > </ALEX> > > > > > o 5 > > > > Since 3.5.2 specifies that a subset of all operations from YANG > > patch to be used in push update records, shouldn't the typedef > > change-type only include this subset? > > > > Otherwise, why should a client be able to exclude "merge", when > > "merge" can never be included? > > > > <ALEX> Not sure what you would like me to change? remove "merge" from > the enum? > </ALEX> Yes, my suggestion is to remove "merge" and "remove". > > o 9.2 > > > > The reference [bergstra2014] is not used and can be removed. > > > > <ALEX> Removed > </ALEX> > > > > > o Comment from an earlier review: > > > > (the document also uses the term "data object" and "datastore > > object", these should be fixed) > > > > These should both be changed to "datastore node" or "object". > > > > <ALEX> Changed the last remaining instances > </ALEX> /martin
- [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Martin Bjorklund
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Andy Bierman
- Re: [Netconf] mbj's WGLC review of yang-push-17 Yutianpeng (Tim)
- Re: [Netconf] mbj's WGLC review of yang-push-17 tom petch
- Re: [Netconf] mbj's WGLC review of yang-push-17 Andy Bierman
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Eric Voit (evoit)
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Alexander Clemm
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Juergen Schoenwaelder
- Re: [Netconf] mbj's WGLC review of yang-push-17 Andy Bierman
- Re: [Netconf] mbj's WGLC review of yang-push-17 Kent Watsen
- Re: [Netconf] mbj's WGLC review of yang-push-17 Juergen Schoenwaelder
- Re: [Netconf] mbj's WGLC review of yang-push-17 Qin Wu
- Re: [Netconf] mbj's WGLC review of yang-push-17 Juergen Schoenwaelder
- Re: [Netconf] mbj's WGLC review of yang-push-17 Qin Wu
- Re: [Netconf] mbj's WGLC review of yang-push-17 Juergen Schoenwaelder
- Re: [Netconf] mbj's WGLC review of yang-push-17 Qin Wu
- Re: [Netconf] mbj's WGLC review of yang-push-17 Robert Wilton
- Re: [Netconf] mbj's WGLC review of yang-push-17 tom petch
- Re: [Netconf] mbj's WGLC review of yang-push-17 Reshad Rahman (rrahman)