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

[Alexander Clemm <[e-mail]>]

Hi Martin,

thank you for your new suggestion  - I will go through the occurrences of "push update" to clean up / clarify which ones refer to update record (respectively update) versus the notification.  

Regarding your previous comments (or replies to my responses to your comments), indeed I did not add responses to all replies of yours and in the course missed two things; my apologies; please see <ALEX3>

Thanks
--- Alex

> -----Original Message-----
> From: Martin Bjorklund [mailto:[e-mail]]
> Sent: Wednesday, August 29, 2018 6:08 AM
> To: Alexander Clemm <[e-mail]>
> Cc: [e-mail]; [e-mail]
> Subject: Re: [Netconf] mbj's WGLC review of yang-push-17
> 
> 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/)
> > >
<ALEX3> OK, closed </ALEX3>

> > > > > 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.
> > >
<ALEX3> OK, closed </ALEX3>

> > > >
> > > > >
> > > > > 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.

<ALEX3> OK, closed </ALEX3>
> > >
> > > > > 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?
> > >

<ALEX3> Ah, ok, I overlooked this one.   So, when rejecting, we can return the error identity.  When the subscription is terminated, a notification "subscription-terminated" is sent (per the Subscribed Notifications draft) that can include ungaging-selection as its reason. I will make this explicit in -19.  
  </ALEX3>

> > >
> > > > <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.
> > >
<ALEX3> OK, closed </ALEX3>

> > > > </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.
> > >

<ALEX3> OK, how about saying "The server MAY (but does not have to) merge pending update messages". </ALEX3>

> > >
> > > > >   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.
> > >
<ALEX3> OK, closed </ALEX3>
> > > > > 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...)
> > >
<ALEX3> OK, closed </ALEX3>
> > > > > 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.
> > >
> > >

<ALEX3> OK, incorporated your suggestions, closed </ALEX3>
> > > > </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
> >