Re: [Netconf] mbj's WGLC review of yang-push-17

[Martin Bjorklund <[e-mail]>]

Alexander Clemm <[e-mail]> wrote:
> Hi Martin,
> 
> thank you for your replies!  One inline reply, <ALEX2>

Ok.  What about my other comments?  I see that some of them still
applies to -18 (the ones you didn't reply to below).

Also, looking at the diff for 17-18, I found a new issue:


o  Section 3

   Subscriptions specify when notification messages (also referred to as
   "push updates") should be sent and what data to include in update
   records.


  I accept that you use "update record" and "update" for the same
  thing (namely "A representation of one or more datastore node
  updates.").

  But what is a "push update"?  From the quoted text, it seems to be
  the *notifican message* rather than the "update record".  But when
  the term "push update" is used, it seems to be a third name for
  "update record".  (For example in 3.9:  "A publisher MUST ensure
  that no non-authorized data is included in push updates.")

  There is also the specific "push-update" notification (one form of
  "update record", the other being "push-change-update", I assume).

  In order to avoid confusion, I suggest that you change the
  occurances of "push update" to simply "update" or "update record"
  (or "push-update" notification, when/if that is referred to).



/martin



> 
> Thanks
> --- Alex
> 
> > -----Original Message-----
> > From: Martin Bjorklund [mailto:[e-mail]]
> > Sent: Tuesday, August 28, 2018 1:09 AM
> > To: Alexander Clemm <[e-mail]>
> > Cc: [e-mail]; [e-mail]
> > Subject: Re: [Netconf] mbj's WGLC review of yang-push-17
> > 
> > Hi,
> > 
> > Thanks for addressing my comments.  Some replies inline.
> > 
> > 
> > Alexander Clemm <[e-mail]> 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:[e-mail]] On Behalf Of Martin
> > > > Bjorklund
> > > > Sent: Wednesday, August 15, 2018 4:18 AM
> > > > To: [e-mail]
> > > > Subject: [Netconf] mbj's WGLC review of yang-push-17
> > > >
> > > > Hi,
> > > >
> > > > Kent Watsen <[e-mail]> 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".
> > 
> 
> <ALEX2> ok </ALEX2>
> 
> > > > 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
>