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

Alexander Clemm <alexander.clemm@huawei.com> Tue, 28 August 2018 01:16 UTC

Return-Path: <alexander.clemm@huawei.com>
X-Original-To: netconf@ietfa.amsl.com
Delivered-To: netconf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id C833A130EB5 for <netconf@ietfa.amsl.com>; Mon, 27 Aug 2018 18:16:58 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id F35HWFqDl0YD for <netconf@ietfa.amsl.com>; Mon, 27 Aug 2018 18:16:55 -0700 (PDT)
Received: from huawei.com (lhrrgout.huawei.com [185.176.76.210]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id B5988130DEB for <netconf@ietf.org>; Mon, 27 Aug 2018 18:16:54 -0700 (PDT)
Received: from lhreml704-cah.china.huawei.com (unknown [172.18.7.107]) by Forcepoint Email with ESMTP id 0FD7F223691C7 for <netconf@ietf.org>; Tue, 28 Aug 2018 02:16:50 +0100 (IST)
Received: from SJCEML702-CHM.china.huawei.com (10.208.112.38) by lhreml704-cah.china.huawei.com (10.201.108.45) with Microsoft SMTP Server (TLS) id 14.3.399.0; Tue, 28 Aug 2018 02:16:51 +0100
Received: from SJCEML521-MBS.china.huawei.com ([169.254.2.188]) by SJCEML702-CHM.china.huawei.com ([169.254.4.168]) with mapi id 14.03.0415.000; Mon, 27 Aug 2018 18:16:48 -0700
From: Alexander Clemm <alexander.clemm@huawei.com>
To: Martin Bjorklund <mbj@tail-f.com>, "netconf@ietf.org" <netconf@ietf.org>
Thread-Topic: [Netconf] mbj's WGLC review of yang-push-17
Thread-Index: AQHUNImzU9I4weZMB0iH1L0LVTTudKTT9lwA
Date: Tue, 28 Aug 2018 01:16:48 +0000
Message-ID: <644DA50AFA8C314EA9BDDAC83BD38A2E0EB59E44@sjceml521-mbs.china.huawei.com>
References: <20180815.131758.1464388348783195997.mbj@tail-f.com>
In-Reply-To: <20180815.131758.1464388348783195997.mbj@tail-f.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.193.35.68]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-CFilter-Loop: Reflected
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/cDyYNS9wNt3Lan6LzLm4oLUoFNY>
Subject: Re: [Netconf] mbj's WGLC review of yang-push-17
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.27
Precedence: list
List-Id: Network Configuration WG mailing list <netconf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/netconf>, <mailto:netconf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/netconf/>
List-Post: <mailto:netconf@ietf.org>
List-Help: <mailto:netconf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/netconf>, <mailto:netconf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 28 Aug 2018 01:16:59 -0000

Hi Martin,

thank you for your comments.  

Please see my responses inline, <ALEX>

(Apologies for the sluggish response; I have been travelling)

--- Alex

> -----Original Message-----
> From: Netconf [mailto:netconf-bounces@ietf.org] On Behalf Of Martin
> Bjorklund
> Sent: Wednesday, August 15, 2018 4:18 AM
> To: netconf@ietf.org
> Subject: [Netconf] mbj's WGLC review of yang-push-17
> 
> Hi,
> 
> Kent Watsen <kwatsen@juniper.net> wrote:
> > This message starts a Last Call on draft-ietf-netconf-yang-push-17:
> 
> I have reviewed this document, and I think it is almost ready.  Here are my
> comments:
> 
> 
> o  3.3
> 
>   The text says:
> 
>    Putting it all together, following is the conceptual process for
>    creating an push-change-update notification:
> 
>   Up until this point, the text has just talked about "update
>   record".  Here it says "an push-change-update notification".
> 
>   Maybe simply s/an push-change-update notification/an update record/?
> 

<ALEX> The process itself talk also about sending the record to the receiver.  How about refining it to say " following is the conceptual process for
   creating a update record as part of an on-change subscription:"?  
</ALEX>

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

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

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

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

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


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

> 
> 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?   Arguably, the server should always be able to reject a request it cannot fulfill, which may include underspecification.  

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?)  
</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>

> 
> 
> o  9.2
> 
>   The reference [bergstra2014] is not used and can be removed.
> 

<ALEX> Removed
</ALEX>

> 
> o  Comment from an earlier review:
> 
>   (the document also uses the term "data object" and "datastore
>   object", these should be fixed)
> 
>   These should both be changed to "datastore node" or "object".
> 

<ALEX> Changed the last remaining instances
</ALEX>


> 
> 
> /martin
> 
> _______________________________________________
> Netconf mailing list
> Netconf@ietf.org
> https://www.ietf.org/mailman/listinfo/netconf