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