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

[Alexander Clemm <[e-mail]>]

Hi Martin,

thank you for your replies!  One inline reply, <ALEX2>

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