Re: [yang-doctors] [Netconf] mbj's YANG doctor's review and WGLC comments on yang-push-15

Martin Bjorklund <mbj@tail-f.com> Fri, 08 June 2018 14:18 UTC

Return-Path: <mbj@tail-f.com>
X-Original-To: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 89281130EC0; Fri, 8 Jun 2018 07:18:47 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.901
X-Spam-Level:
X-Spam-Status: No, score=-1.901 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_PASS=-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 2s4YkiqdCnI3; Fri, 8 Jun 2018 07:18:44 -0700 (PDT)
Received: from mail.tail-f.com (mail.tail-f.com [46.21.102.45]) by ietfa.amsl.com (Postfix) with ESMTP id EB1B8130EBF; Fri, 8 Jun 2018 07:18:43 -0700 (PDT)
Received: from localhost (h-80-27.A165.priv.bahnhof.se [212.85.80.27]) by mail.tail-f.com (Postfix) with ESMTPSA id B7BAF1AE027A; Fri, 8 Jun 2018 16:18:41 +0200 (CEST)
Date: Fri, 08 Jun 2018 16:18:41 +0200 (CEST)
Message-Id: <20180608.161841.1253958512370940227.mbj@tail-f.com>
To: ludwig@clemm.org
Cc: netconf@ietf.org, yang-doctors@ietf.org, evoit@cisco.com
From: Martin Bjorklund <mbj@tail-f.com>
In-Reply-To: <03a301d3f8ab$274e1100$75ea3300$@clemm.org>
References: <20180316.104706.2162517973525816941.mbj@tail-f.com> <03a301d3f8ab$274e1100$75ea3300$@clemm.org>
X-Mailer: Mew version 6.7 on Emacs 24.5 / Mule 6.0 (HANACHIRUSATO)
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/NU0o6N3-kG_54UlfBsWeV3Tc0jM>
Subject: Re: [yang-doctors] [Netconf] mbj's YANG doctor's review and WGLC comments on yang-push-15
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.26
Precedence: list
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 08 Jun 2018 14:18:48 -0000

Hi,

Thanks for addressing my comments.  See inline for further comments on
some of the issues.


"Alexander Clemm" <ludwig@clemm.org> wrote:
> Hi Martin, 
> 
> Apologies for my delay in response.
> 
> Thank you for your thorough review and comments!  Please find enclosed our
> responses ("<ALEX>").  We will be posting an updated revision (-16)
> momentarily.
> 
> --- Alex
> 
> -----Original Message-----
> From: Netconf <netconf-bounces@ietf.org> On Behalf Of Martin Bjorklund
> Sent: Friday, March 16, 2018 2:47 AM
> To: netconf@ietf.org; yang-doctors@ietf.org
> Subject: [Netconf] mbj's YANG doctor's review and WGLC comments on
> yang-push-15
> 
> Hi,
> 
> Here are my WGLC comments and YANG doctor's review on
> draft-ietf-netconf-yang-push-15.

[...]

> o  Section 2
> 
>   You have:
> 
>    Datastore node update: A data item containing the current value/
>    property of a datastore node at the time the datastore node update
>    was created.
> 
>   This definition is not clear to me.  The term is used twice in the
>   document so maybe it can be removed.  When it is used in section
>   3.1, it is not clear that you mean a "data item".
> 
>   My suggestion is to remove this term from the terminology list, but
>   use the words "datastore node update" as you already do.
> 
> <ALEX> We would prefer to keep it.  Perhaps remove the "/property" (i.e.
> just saying "containing the current value of a datastore node ...".  The
> reason to keep it is that at the end of the day, what we send are those
> updates.  
> </ALEX>

But the definition is recursive, and in 3.1 you have:

   allow subscribers to subscribe to datastore node
   updates

With your definition this reads:

   allow subscribers to subscribe to data items that contain the
   current value of a datastore node...

What is a "data item"?

Also, you have:

   o  Update record: A representation of one or more datastore node
      updates.

This reads:


   o  Update record: A representation of one or more data items that
      contain ... 

IMO the doc would be more clear of you simply removed the term from
the terminology section.  You already define the term "datastore
node", and the word "update" is quite clear as it is.

> o  Section 2
> 
>   You have:
> 
>    Update record: A representation datastore node update(s) resulting
>    from the application of a selection filter for a subscription.  An
>    update record will include the value/property of one or more
>    datastore nodes at a point in time.  It may contain the update type
>    for each datastore node (e.g., add, change, delete).  Also included
>    may be metadata/headers such as a subscription identifier.
> 
>   s/A representation datastore node/A representation of datastore node/
> 
>   What is "value/property"; first, does "/" mean "and" or "or"?
>   second, what is a "property" of a datastore node?
> 
>   Ditto for "metadata/headers".
> 
> <ALEX> We updated this as follows, does this work?
> "Update record: A representation of one or more datastore node updates.  In
> addition, an update record may contain which type of update led to the
> datastore node update (e.g., whether the datastore node was added, changed,
> deleted).  Also included in the update record may be other metadata, such as
> a subscription identifier of the subscription as part of which the update
> record was generated."
> </ALEX>

Ok.

> o  Section 2
> 
>   The term "Update trigger" isn't used in the document.  I suggest you
>   remove it.
> 
> <ALEX> Yes, indeed we only use the word "trigger" throughout the document.
> However, using "update trigger" is perhaps more precise.  Keeping the term,
> but replacing instances of "trigger" with "update trigger" throughout the
> document as appropriate.  
> </ALEX>

Ok.

> o  Section 3
> 
>   You write:
> 
>    This document specifies a solution for a push update subscription
>    service.
> 
>   I think this should be reworded; what exactly is a "push update
>   subscription service"?
> 
> <ALEX> Rephrased to 
> "This document specifies a solution that provides subscription service for
> updates from a datastore. "
> </ALEX>

Ok.

>   The you have:
> 
>    This solution supports dynamic as well as configured
>    subscriptions to information updates from datastores.
> 
>   Maybe s/information updates from datastores/updates to datastore
>   nodes/
> 
> <ALEX> Rephrased to
> "This solution supports dynamic as well as configured subscriptions to
> updates of datastore nodes."
> </ALEX>

Ok.

> o  Section 3.1
> 
>       There are two
>       types of triggers for subscriptions: periodic and on-change.
> 
>   I think you mean that there are two types of subscriptions:
> 
>       There are two
>       types of subscriptions: periodic and on-change.
> 
>   B/c later in the document you use the terms "on-change subscription"
>   and "periodic subscription".  I never see "on-change trigger".
> 
>   These terms should be defined in the terminology section, b/c they
>   are central.
> 
> <ALEX> Rephrased the sentence in question to:
> "There are two types of subscriptions, distinguished by how updates are
> triggered: periodic and on-change. "
> Added the terms "periodic subscription", "on-change subscription",
> "datastore subscription" to the terminology section. 
> </ALEX>

Ok.

> o  Section 3.4
> 
>   The title is "Promise-Theory Considerations".
> 
>   I have pointed this out before; either change the title, or have a
>   reference to the promise-theory you relate to, and explain in more
>   details how you relate to this theory.
> 
> <ALEX> I removed the reference to this and changed the title to "Reliability
> Considerations".  Giving an intro to promise theory would probably be
> distracting.
> </ALEX>

Ok.

>   Also, this section says:
> 
>    If for
>    some reason the publisher of a subscription is not able to keep its
>    promise, receivers MUST be notified immediately and reliably.
> 
>   Is this intended as a requirement on the server, or on the solution?
> 
>   If it is the former, it shouldn't be defined here, but rather be
>   explicit when the operations are defined (which I think it is).
> 
>   If it is the latter, you should make that clear, and remove the 2119
>   language.
> 
>   <ALEX> Really, it is both.  It is a requirement on the solution, that our
> solution addresses (and hence by extension servers that implement the
> solution).  Applied some slight rewording to make clear that it is the
> former:
>   "For this reason, 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."
>   </ALEX>

Ok.

> o  Section 3.5.2
> 
>   It is not clear how the server is supposed to construct the YANG
>   patch record.  You have some text about what a client should do, but
>   no instructions for the server.  When would a server use the
>   "remove" edit-operation, rather than "delete"?
> 
>   Instead of having special rules for the client, wouldn't it be
>   better to say that the server MUST NOT use "create" and "delete"
>   (instead use "repalce" and "remove").  Then the YANG patch semantics
>   is left intact.
> 
>   <ALEX> I am not sure that it is a good idea to prohibit create and delete.
> 
>   "Remove" would still be okay, I guess (although I am not sure what
> avoiding delete buys us, though), but for "create", I don't think "replace"
> expresses the same.  In short, my preference would be to keep this as is.  
>   
>   As to more server-side instructions, at the risk of making the text a bit
> redundant, we are adding the following snippet:
>   "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.

Ok.

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

Why are list elements and containers treated differently?

I still think that it would be better to limit the number of
operations so that YANG PATCH semantics are left intact.  It is ok
with me to view the patch from the server's point of view, i.e.,
always use "create" and "delete", and never use "remove" or "replace".

It would be useful to hear other people's opinion on this one.


> o  Section 3.8
> 
>   OLD:
> 
>    The RPCs defined within
>    [I-D.draft-ietf-netconf-subscribed-notifications] have been enhanced
>    to support datastore subscription negotiation.  Included in these
>    enhancements are error codes which can indicate why a datastore
>    subscription attempt has failed.
> 
>   NEW:
> 
>    The RPCs defined within
>    [I-D.draft-ietf-netconf-subscribed-notifications] have been
>    augmented to support datastore subscription negotiation.  Also, new
>    error codes that indicates why a datastore subscription attempt has
>    failed have been added.
> 
> 
>    <ALEX> Changed the second sentence to 
>    "Also, new error codes have been added that are able to indicate why a
> datastore subscription attempt has failed"
>    </ALEX>

Second sentence is ok.

But I think you should also change the first sentence as I proposed
above ("augment" rather than "enhance").


> o  Section 3.8
> 
>   You write:
> 
>    o  "error-app-tag" with the value being a string that corresponds to
>       an identity with a base of "establish-subscription-error" (for
>       error responses to an establish-subscription request), "modify-
>       subscription-error" (for error responses to a modify-subscription
>       request), "delete-subscription-error" (for error responses to a
>       delete-subscription request), "resynch-subscription-error" (for
>       error responses to resynch-subscription request), or "kill-
>       subscription-error" (for error responses to a kill-subscription
>       request), respectively.
> 
>   This is not how errors are handled in
>   draft-ietf-netconf-subscribed-notifications.  Why do you have two
>   different mechanisms?  And why is this identity needed; you already
>   have the identity in the "establish-subscription-error-datastore".
> 
>   I think you should decide on one mechanism, and use it in both
>   drafts (specifically, the error-app-tag handling.  BTW, *if* you
>   decide to keep it, you need to clarify what "a string that
>   corresponds to" means.  Maybe use the JSON encoding of identities in
>   this case (<modname>:<identityname>)).
> 
>   <ALEX> I agree we should have one mechanism and be consistent.  Updating
> this to be in line with subscribed-notifications.  
>   I am also renaming the yang data to establish- (respectively modify-)
> subscription-datatore-error-info to be consistent with the naming
> conventions in subscribed-notifications.  </ALEX>

Ok.  But in -16, the "error-app-tag" text is still present in section
4.4.1 and 4.4.2.

>   Also, in the example in Figure 4, the "reason" leaf is missing.
> 
>   <ALEX> Struck the example and updated the text with the yang tree for the
> yang-data instead, so we are consistent with subscribed-notifications now.
> </ALEX>

Ok.

> o  Section 3.9
> 
>   Because of the NACM rules you have here, should this document
>   formally "Update" 6536bis?
> 
>   <ALEX> We don't know what the guidelines here are.  You tell us.  If we
> don't have to introduce additional dependencies, that will be preferred by
> us. 
>   </ALEX>

First, the text references [RFC8342], this should be [RFC8341].  Also,
I think it should reference 3.4.5 which describes general rules for
accessing nodes in a datastore, which this function does.

Second, the figure text has [rfc6536bis], this should be [RFC8341].

Third, I think you actaully just follow section 3.4.5, so maybe no
formal "updates 8341" is needed.  Maybe the chairs that chime in here.


> o  Section 3.10
> 
>   (clarification)
> 
>   OLD:
> 
>    In some cases, a publisher supporting on-change notifications may not
>    be able to push updates for some datastore node types on-change.
> 
>   NEW:
> 
>    In some cases, a publisher supporting on-change notifications may not
>    be able to detect changes in all datastore nodes.
> 
>    <ALEX> We prefer to keep this as is.  This is not just about detection,
> but also about the fact that changes would simply be too frequent (e.g.
> generating too much load).  
>    </ALEX>

Ok.

> o  Section 4.2
> 
>   You write:
> 
>    o  For periodic subscriptions, triggered updates will occur at the
>       boundaries of a specified time interval.  These boundaries many be
>       calculated from the periodic parameters:
> 
>    s/many/may/  or s/many be/are/
> 
>    <ALEX> substituted it with "can be" </ALEX>

Ok, but IMO this is not precise... Do you mean that they can be, but
not have to be calculated from the periodic parameters?  If so, how
can they otherwise be calculated?

> o  Section 4.3.1
> 
>   You write:
> 
>    Subscription state notifications and mechanism are reused from
>    [I-D.draft-ietf-netconf-subscribed-notifications].  Some have been
>    augmented to include the datastore specific objects.
> 
>   Instead of writing "some have been augmented", be explicit and list
>   the ones that have been updated (I think it is just two).
> 
>   <ALEX> OK, changed to "Notifications "subscription-started" and
> "subscription-modified" have been augmented to include the datastore
> specific objects." </ALEX>


Ok.   (probably "The notifications ...")


> o  Section 4.4.1
> 
>   The XML example is not correct.  Please ensure that all examples are
>   validated by some tool.   Also I suggest you ensure the examples are
>   consistently indented, and that they use "xmlns" consistently.  As
>   it looks a bit messy.
> 
>   In this case, there's an element <yp:source> in the XML that doesn't
>   exist in the datamodel; the element <xpath-filter> is placed within
>   a leaf, and incorrectly named (it should be
>   <yp:datastore-xpath-filter>); and it uses an incorrect "select"
>   attribute.
> 
>   
>   <ALEX> We will update this. </ALEX>

Ok.  I note that they are not updated in -16.

> o rpc resynch-subscription
> 
>     description
>       "This RPC allows a subscriber of an active on-change
>        subscription to request a full push of objects in their current
>        state. A successful result would invoke a push-update of all
>        datastore objects that the subscriber is permitted to access.
>        This request may only come from the same subscriber using the
>        establish-subscription RPC.";
> 
>   Consider removinging the words "in their current state"; a
>   'push-update' always send nodes in their current state.
> 
>   <ALEX> OK </ALEX>
>   
>   Rephrase "invoke a push-update"; maybe :result in the publisher sending a
>   'push-update' notification".
> 
>   <ALEX> rephrased to "A successful invocation results in a push-update of
> all 
>        datastore objects that the subscriber is permitted to access. "
> </ALEX>
>   
>   Rephrase the last sentence; you probably mean that it must be sent
>   on the same session where the subscription was established.
> 
>   <ALEX> rephrased to "This RPC can only be invoked on the same session on
> which the 
>        subscription was established (using an establish-subscription 
>        RPC)."
>    </ALEX>
>        
>   Also, include text in the rpc description that explains that the
>   resynch-subscription-error is sent on error.
> 
> <ALEX> added "In case of an error, a resynch-subscription-error is 
>        sent as part of an error response." </ALEX>
> 

Ok.

> o  error identities
> 
>   When reading the YANG module, it is not really clear how these
>   identities are supposed to be used.  For example, one of the first
>   identities is:
> 
>     identity cant-exclude {
>       base sn:establish-subscription-error;
>       description
>         "Unable to remove the set of 'excluded-changes'.  This means the
>          publisher is unable to restrict 'push-change-update's to just the
>          change types requested for this subscription.";
>     }
> 
>   Since this is derived from sn:establish-subscription-error, it seems
>   that the idea is to use it anywhere an
>   sn:establish-subscription-error is expected, e.g., as the "reason"
>   in "establish-subscription-error-stream".  But this is probably not
>   true?
> 
>   I think that since you define new yang-data structure for errors
>   related to push-subscriptions, you shouldn't use the
>   sn:establish-subscription-error at all; instead you should define
>   a new set of base identities in this module.
> 
> <ALEX> The identities are used by the server in a response.  A correct
> server implementation will need to pick the correct reason, regardless of
> where the identities are defined.  Clients are only going to to read those.
> Since we are still using the same RPC and augmenting it, we prefer to have
> the relationship also reflected by the identities.  Of course, we could have
> an entirely different set of RPCs, one to manage stream subscriptions, the
> other to manage datastore subscriptions.  In that case, having separate
> identities would be appropriate.  However, that runs counter to the
> decisions we made earlier as a working group, that one should be a
> generalization of the other.   For those reasons, we prefer to leave this
> item unchanged. </ALEX>

Well, you already define a separate error-info structure than what is
used in subscribed-notifications; you have
"establish-subscription-datastore-error-info" and
subscribed-notifications has
"establish-subscription-stream-error-info".

Since you have a separate structure, with its own "reason" leaf, there
is no point in using the base sn:establish-subscription-error.  In
fact I think it is wrong, since it gives the impression that for
example "cant-exclude" can be used within
"sn:establish-subscription-stream-error-info".

I do not suggest that you should have a new set of RPCs.



> o  typedef change-type
> 
>   The description in this type talks about "data resource" (see my
>   comment on terminology above).  The description doesn't quite match
>   how this type is used.  For example:
> 
>       enum "create" {
>         description
>           "Create a new data resource if it does not already exist.  If
>           it already exists, replace it.";
>       }
> 
>   This type is used only in "excluded-change", and as such it is
>   used to inform the server about which changes the client doesn't
>   want.  So shouldn't the description text describe this?  Something
>   like this for "create":
> 
>     "Instructs the server that if a datastore node is created, it
>      should not trigger an update."
> 
>      <ALEX> Updating the descriptions:
>      create: "A change that refers to the creation of a new data node."
>      delete: "A change that refers to the deletion of a data node."
>      insert: "A change that refers to the insertion of a new user-ordered
> data node."
>      merge: "A change that refers to a merging of a new value with a target
> data node."
>      move: "A change that refers to a reordering of the target data node"
>      replace: "A change that refers to a replacement of the target data
> node's value."
>      remove: "A change that refers to the removal of a data node."
>      </ALEX>
>      
>   Maybe also change the name of the typedef to "excluded-change-type".
> 
>   See also my comment on 3.5.2 above.
> 
>   <ALEX> On changing the name of the typed: The type is currently only used
> in excluded-change, that is correct.  However, when you look at the enum,
> this really just defines the change types.  Perhaps the same typedef might
> be used in a different capacity elsewhere.  I don't feel strongly about
> this, however, I think the name of the typedef is in fact appropriate.
> </ALEX>

Ok, and the new descriptions are better.  But compare the description
of "delete" and "remove" - it is not clear at all what the difference
is, see my comment above as well.

> o  list selection-filter
> 
>   The description says:
> 
>         "A list of pre-positioned filters that can be applied
>          to datastore subscriptions.";
> 
>          
>   What is a "pre-positioned" filter?  I suggest:
> 
>         "A list of filters that can be applied
>          to datastore subscriptions.";
> 
>   (I just noticed that the same words exists in the
>   subscribed-notifications document; I suggest the same change is
>   applied there as well.)
> 
>   <ALEX> Rephrased "pre-positioned" to "pre-configured" (I think we wanted
> to write "pre-provisioned" at first, but mistyped...)
>   </ALEX>

I still think you should simply write "A list of filters".  Otherwise,
why does this list have "pre-configured" elements, but e.g. the
"subscription" list does not have "pre-configured" elements?

IMO, "pre-configuration" is when you provide configuration for
something that doesn't yet exist, e.g. configure an interface for
which the hardware isn't present.


> o  leaf selection-filter/identifier
> 
>   This has:
> 
>       leaf identifier {
>         type sn:filter-id;
> 
>   I think this is a bit over-engineered.  I suggest to simply use
>   "type string" here.   And the same in ietf-subscribed-notifications;
>   the type filter-id should be removed.
> 
>   Also, I think all existing modules have followed some kind of
>   unspoken convention that arbitrary named list entries have a key
>   called "name" of type string.  When there's some kind of restricted
>   identifier used, the key leaf is called "id" or "<foo>-id"
>   (e.g. "router-id", "session-id" etc).
> 
> <ALEX> I liked the leafref as it makes it harder to violate referential
> integrity than with a string, but ok.  Renamed it to "filter-id".  </ALEX>

But sn:filter-id is not a leafref, it is just a string.  Which I think
is overkill; simply do:

  leaf name {
    type string;
  }

... and I still prefer "name" over "identifier" and "filter-id";
"name" is also more in line with other published IETF models.

"filter-id" goes against the 6087bis recommendation in 4.3.1:

   Child nodes within a container or list SHOULD NOT
   replicate the parent identifier. 

> o  notifications push-update and push-change-update
> 
>   Both these have this text:
> 
>        This notification shall only be sent to receivers of a
>        subscription; it does not constitute a general-purpose
>        notification.
> 
>   Why do you have this text?  It seems to imply that it would be
>   illegal for a server to ever send these notifs on some "stream".
>   Why is this necessary?  If I have a clever use case for this, why is
>   it made illegal?
> 
> <ALEX>   
>         rephrased to "It does not constitute a general-purpose
>        notification that would be subscribable as part of the NETCONF
>        event stream by any receiver."
> </ALEX>

Ok.

> o  anydata datastore-contents in notification push-update
> 
>       description
>         "This contains the updated data.  It constitutes a snapshot
>          at the time-of-update of the set of data that has been
>          subscribed to.  The format and syntax of the data
>          corresponds to the format and syntax of data that would be
>          returned in a corresponding get operation with the same
>          selection filter parameters applied.";
> 
>   This description is not precise enough.  What is "a corresponding
>   get operation"?  Does this text mean that I will get different
>   contents if I use NETCONF than if I use RESTCONF?
> 
> <ALEX> rephrased as follows: 
> "      description
>         "This contains the updated data.  It constitutes a snapshot 
>          at the time-of-update of the set of data that has been 
>          subscribed to.  The snapshot corresponds to the same 
>          snapshot that would be returned in a corresponding get 
>          operation with the same selection filter parameters 
>          applied.";"
> </ALEX>

This text still uses "corresponding get operation".  What is that?
Corresponding to what?  Do you mean RESTCONF GET?  NETCONF <get>?


> o  anydata datastore-changes in notification push-change-update
> 
>   You write:
> 
>         "This contains the set of datastore changes needed
>          to update a remote datastore starting at the time of the
>          previous update, per the terms of the subscription.
> 
>   Isn't this backwards?  Shouldn't it be:
> 
>         "This contains the set of datastore changes of the
>          target datastore starting at the time of the
>          previous update, per the terms of the subscription.
> 
>   With the current text, it seems the server must have knowledge about
>   the remote datastore (if it even exists).
> 
>   <ALEX>
>   Thank you, updated
>   </ALEX>

Ok.

>   Further, it says:
> 
>          Changes
>          are encoded analogous to the syntax of a corresponding yang-
>          patch operation, i.e. a yang-patch operation applied to the
>          datastore implied by the previous update to result in the
>          current state.
> 
>   I don't understand what this text says.  Looking at the example, I
>   think that what you really should do is remove that sentence, and
>   change this "anydata" node to a container:
> 
>      container datastore-changes {
>        uses ypatch:yang-patch;
>        ...
>      }
> 
>   With this approach, you can even "refine" the description statement
>   of some nodes (e.g. the "operation" leaf) and explain the
>   create/delete semantics.
> 
> 
>   This node also has this reference:
> 
>       reference
>         "RFC 8072 section 2.5, with a delta that it is ok to receive
>          ability create on an existing node, or receive a delete on a
>          missing node.";
> 
>    The reference statement is just supposed to contain the formal
>    reference.  If descriptive text is needed it should go into the
>    "description".  But in this case I think you should simply remove
>    the text in the reference.
> 
>    <ALEX> Since we are using this only in the push-change-update, we prefer
>     to leave it as is.

I don't understand this comment.  I suggest you use the grouping that
is defined in ietf-yang-patch for the purpose of being reused.

It would be useful to hear other people's opinion on this.

If the WG decides to keep anydata, I think it needs to be clear from
the description that an instantiation of the rc:yang-data "yang-patch"
is expected.

> We removed the reference statement and updated the
> desciption
>     to now read as follows:
>     anydata datastore-changes {
>       description
>         "This contains the set of datastore changes of the 
>          target datastore starting at the time of the        
>          previous update, per the terms of the subscription.
>          The datastore changes are encoded per RFC 8027 
>          (YANG Patch).";
>     }

That should be RFC 8072.  But I think you should keep the reference
statement:

  reference
     "RFC 8072: YANG Patch Media Type";


Note also that the reference statement for the typedef "change-type"
has the same problem; the text should be moved to the description, and
the reference statement just list the document and section.

> o  augment "/sn:subscription-modified"
>     description
>       "This augmentation adds many datastore specific objects to
>        the notification that a subscription has been modified.";
> 
> 
>   Please be more specific than "adds many nodes".  How many is many?
> 
>   <ALEX> removed "many" </ALEX>.  Which nodes is provided through the "uses"
> statement; there is no need to expand it in the description. </ALEX>

Ok.

> o  augment "/sn:subscription-modified/sn:target"
> 
>     case datastore {
>        uses datastore-criteria {
>           refine "selection-filter/within-subscription" {
>           description
>             "Specifies where the selection filter, and where it came
>             from within the subscription and then populated within this
>             notification.
> 
>    This sentence doesn't parse.
> 
> <ALEX> Rephrased as follows:
> "Specifies the selection filter and where it originated from."
> </ALEX>

Ok.



/martin