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

"Alexander Clemm" <ludwig@clemm.org> Thu, 31 May 2018 06:47 UTC

Return-Path: <ludwig@clemm.org>
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 A897112E036; Wed, 30 May 2018 23:47:15 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.899
X-Spam-Level:
X-Spam-Status: No, score=-1.899 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_NONE=-0.0001, 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 CNxkJvBhO6li; Wed, 30 May 2018 23:47:11 -0700 (PDT)
Received: from mout.perfora.net (mout.perfora.net [74.208.4.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 6E3F2126C19; Wed, 30 May 2018 23:47:11 -0700 (PDT)
Received: from LAPTOPR7T053C2 ([24.6.143.61]) by mrelay.perfora.net (mreueus001 [74.208.5.2]) with ESMTPSA (Nemesis) id 0Mgdfd-1fiPIz2VC3-00O0AE; Thu, 31 May 2018 08:46:50 +0200
From: Alexander Clemm <ludwig@clemm.org>
To: 'Martin Bjorklund' <mbj@tail-f.com>, netconf@ietf.org, yang-doctors@ietf.org
References: <20180316.104706.2162517973525816941.mbj@tail-f.com>
In-Reply-To: <20180316.104706.2162517973525816941.mbj@tail-f.com>
Date: Wed, 30 May 2018 23:46:47 -0700
Message-ID: <03a301d3f8ab$274e1100$75ea3300$@clemm.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
X-Mailer: Microsoft Outlook 16.0
Thread-Index: AQG7ecnXtDJvskNvSQqX9INyHyK5S6R6YhSQ
Content-Language: en-us
X-Provags-ID: V03:K1:NHGNi0mg2vHleG9EKRUXO56gAYMEHhZr21vDI9VS4IOq8sVHJG5 WLVYGHr8RVg2R7/kDiFMv1AtKzLVQ55nc5nXw/uhrkiwEaqyTkugJw97LtdWXzm36irGl/b tNH1eDRqQsxbUmnkJ7nUzC6wI96vau4IqbPgq521aTF/VS8ryw/WPQUw4LTdaS/QEU7ry2m zocgQ2qXqT29zWuNlp5Og==
X-UI-Out-Filterresults: notjunk:1;V01:K0:XZ8v1WvuvZw=:jz0UchdjzYFle8ozb6Eoom 7WT/GVFA0RHn7Ra2PlKyG+rgRaKdf1DlNvw8zI3dxjdFD8rZf64TciAEkmXFl173sTKvTpDzb H+OpPbvtnvlQre9kJP+6+TE3yItHMB/kJRI7742F30ZNxU1DoF+zVa9RVJXUUEqUtt3Ta27uM iH7r0eihJL+zkoka/mqM2mfvmb4Zj0TYyAFmvNJmjn8YsAuxEY+8CXWY+hYQ/0tfMsqi5KsJV 4bZwGukNk07+EL1Xp/hcwcwGc/5Vhg6t7IdWB3N2D5/OddKmPred8+1v8AH7eQtNmvIaeTUhR L4LJ/4jCsgAzhJ3Q+PYydmElilQ8/pTtwOyLXJPJr2siO4zR+0eNG0LVGQzT+YPE6JP/7Ekt6 bAeEUJnDA3zQEgIjQOy6H5yFk+RA/qKVsLUCbimdmwNxy98EfIQnD0GRDxycOSUtdLfEgSVnK LXpGgizXcigmG0+sq/cQZORG6F9xXJC4NEN8qC/yKmDG/kET5vB77VFAhCZV5VBZ4egbQwaRG 86C8TMUuGxt8lIglQsyWs6oJGxme6p2GdPoTciLdV1iwwvszHper/cnRs2JnC8wXdACfr0B4g iHcFu16V1tCD07lcgYktr8C2FH/uJxiIsz/CveWimpf1Oa1h5kEFPZVK3zo56Eq2x1V7t6mQm HL7al+yCXYSF6DxiAM7lgBaEyUgMFclnN12fiY8jNeCLl8UwOKbWtW/rzl6i1jEKuEYsu7FQB MmX4A+FmC1A1uGWp
Archived-At: <https://mailarchive.ietf.org/arch/msg/netconf/cUNaS3TbZrmG-b3iYl6wyHX0-mo>
Subject: Re: [Netconf] mbj's YANG doctor's review and WGLC comments on yang-push-15
X-BeenThere: netconf@ietf.org
X-Mailman-Version: 2.1.22
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: Thu, 31 May 2018 06:47:16 -0000

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  On p. 13, theres a missing " (which messes up the colors in the
   emacs mode I use)

   OLD:

      responses to an establish-subscription request) or "modify-
      subscription-error-datastore (for error responses to a modify-

   NEW:

      responses to an establish-subscription request) or "modify-
      subscription-error-datastore" (for error responses to a modify-

<ALEX> fixed </ALEX>

o  Section 2

  I suggest you write that this document uses the terms defined in
  other docs:

  OLD:

   The terms below supplement those defined in
   [I-D.draft-ietf-netconf-subscribed-notifications].  In addition, the
   term "datastore" is defined in
   [I-D.draft-ietf-netmod-revised-datastores].

  NEW:

   This document uses the terminology defined in [RFC7950],
   [I-D.draft-ietf-netmod-revised-datastores], and
   [I-D.draft-ietf-netconf-subscribed-notifications].


  For clarity, I also suggest you introduce the new terms in a bullet
  list:

  OLD:

   Datastore node: An instance of management information in a datastore.
   Also known as "datastore node".

  NEW:

   o  datastore node: An instance of management information in a datastore.
      Also known as "datastore node".


  Also, in one place, you use the term "data resource".  Either import
  that term from the proper document, or change to "datastore node".

<ALEX> done </ALEX>

o  Section 2

  I think you definition of "datastore node" can be made more clear:


  OLD:

   Datastore node: An instance of management information in a datastore.
   Also known as "datastore node".

  NEW:

   o  datastore node: An instance of a data node in a datastore.

  NOTE: I think you should stick to the term "datastore node", and not
  use the term "datastore node".  I know that "datastore node" is used quite
a lot in
  this document, but I still think we should change this.

  (the document also uses the term "data datastore node" and "datastore
  datastore node", these should be fixed)

<ALEX> Updated definition as follows: 
Datastore node: An instance of a data node in a datastore.  In the context
of this document sometimes also referred to as "datastore node".  

I also replaced most occurrences of "object" with "datastore node".  (Some
instances of "object" remain, mostly in descriptions and names and where
replacing "object" with "datastore node" would for some reason or another be
awkward.)
 </ALEX>

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

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>

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

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>

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

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

o  Section 3.6

  (terminology)

  OLD:

   o  xpath: An xpath selection filter is an XPath expression that
      returns a node set.  When specified, updates will only come from
      the selected data nodes.

  NEW:

   o  xpath: An xpath selection filter is an XPath expression that
      returns a node set.  When specified, updates will only come from
      the selected datastore nodes.

  Also, when the word "xpath" is used in general text, it should be
  "XPath".  When it refers to a YANG node/type etc, it should be
  quoted:  "xpath".

  <ALEX> Done  </ALEX>

o  Section 3.7

  In the examples, the XML namespace for RFC 7223 is wrong:

  s/http://foo.com/ietf-interfaces/
    urn:ietf:params:xml:ns:yang:ietf-interfaces/

<ALEX> Done </ALEX>
    
o  Section 3.7

  You write:

   Also if used as a counter, the counter MUST be reset
   to '1' the after passing a maximum value of '99999'.

  Isn't a max of 65535 or 4294967295 more reasonable? (uint16, or uint32)

<ALEX> Ok, let's go with with 4294967295 (uint32) </ALEX>
  
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>
   
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>
  
  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>

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>

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>

o  Section 3.11.1

  You're using the term "MAY NOT", but this is not a 2119 term (BTW,
  you need to include the usual 2119 boilerplate)  I think you mean
  "MUST NOT".

  <ALEX> changed </ALEX>

o  Section 4

  You should remove the text describing the tree diagrams, and instead
  reference:

    Tree diagrams used in this document follow the notation defined in
    [I-D.ietf-netmod-yang-tree-diagrams].
    
    <ALEX> changed </ALEX>

o  Section 4.1

  The tree diagram is quite hard to read.  I suggest to prune the tree
  a bit by removing unnecessary nodes defined in
  ietf-subscribed-notifications:

module: ietf-subscribed-notifications
  ...
  +--rw filters
  |  ...
  |  +--rw yp:selection-filter* [identifier]
  |     +--rw yp:identifier                        sn:filter-id
  |     +--rw (yp:filter-spec)?
  |        +--:(yp:datastore-subtree-filter)
  |        |  +--rw yp:datastore-subtree-filter?   <anydata>
  |        |          {sn:subtree}?
  |        +--:(yp:datastore-xpath-filter)
  |           +--rw yp:datastore-xpath-filter?     yang:xpath1.0
  |                   {sn:xpath}?
  +--rw subscriptions
     +--rw subscription* [identifier]
        |  ...
        +--rw (target)
        |  +--:(stream)
        |  |  ...
        |  +--:(yp:datastore)
        |     +--rw yp:datastore
        |     |       identityref
        |     +--rw (yp:selection-filter)?
        |        +--:(yp:by-reference)
        |        |  +--rw yp:selection-filter-ref
        |        |          selection-filter-ref
        |        +--:(yp:within-subscription)
        |           +--rw (yp:filter-spec)?
        |              +--:(yp:datastore-subtree-filter)
        |              |  +--rw yp:datastore-subtree-filter?
        |              |          <anydata> {sn:subtree}?
        |              +--:(yp:datastore-xpath-filter)
        |                 +--rw yp:datastore-xpath-filter?
        |                         yang:xpath1.0 {sn:xpath}?
        |  ...
        +--rw (yp:update-trigger)?
           +--:(yp:periodic)
           |  +--rw yp:periodic!
           |     +--rw yp:period         yang:timeticks
           |     +--rw yp:anchor-time?   yang:date-and-time
           +--:(yp:on-change) {on-change}?
              +--rw yp:on-change!
                 +--rw yp:dampening-period?    yang:timeticks
                 +--rw yp:no-synch-on-start?   empty
                 +--rw yp:excluded-change*     change-type

  (This was generated with the --tree-line-length 69 option to pyang,
  then manually edited to add the "...")


  But I also think that the tree diagram is difficult to read b/c you
  include everything in one single diagram.  I suggest you put the
  config part of the diagram (shown above) in section 4.2, then a tree
  diagram snippet per notification in sections 4.3.1 and 4.3.2, and
  rpc tree snippets in 4.4.*.

  <ALEX> Snipped the tree substantially, in essence pruning all items from
subscribed-notifications that were not required for structure.  (Hopefully
we don't need to regenerate it at some later point...) </ALEX>

o  Section 4.2

  (editorial)

  OLD:

   Both configured and dynamic subscriptions are represented within the
   list subscription.  New and enhanced parameters extending the basic
   subscription data model in
   [I-D.draft-ietf-netconf-subscribed-notifications] include:

  NEW:

   Both configured and dynamic subscriptions are represented within the
   list "subscription".  New parameters extending the basic
   subscription data model in
   [I-D.draft-ietf-netconf-subscribed-notifications] include:

<ALEX> OK </ALEX>

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>

o  Section 4.2

  You write:

      *  a "period" which defines duration between period push updates.

  s/period push updates/push updates/

<ALEX> OK </ALEX>

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>

o  Section 4.3.2

  The first paragraph:

  OLD:

   Along with the subscribed content, there are other objects which
   might be part of a "push-update" or "push-change-update"

  NEW:

   Along with the subscribed content, there are other objects which
   might be part of a "push-update" or "push-change-update"
   notifications.

<ALEX> OK (added "notification") </ALEX>

o  Section 4.3.2

  (terminology)

  OLD:

   An "incomplete-update" datastore node.  This datastore node indicates
that not all

  NEW:

   An "incomplete-update" leaf.  This leaf indicates that not all

   <ALEX> OK </ALEX>

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>

o  Section 4.4.1

  You write:

   The publisher MUST respond explicitly positively (i.e., subscription
   accepted) or negatively (i.e., subscription rejected) to the request.

  (similar text in a couple of other places as well)

  What exactly does this tell me?  What does "explicitly" mean?  Is
  this telling me that the server MUST NOT contain any bugs?

  I suggest you remove this sentence (and the other similar ones).

<ALEX> Removed extra text </ALEX>
  
o  Section 5

  (fix syntax so that extraction tools work properly)

  OLD:

    <CODE BEGINS>; file "ietf-yang-push@2018-02-23.yang"

  NEW:

    <CODE BEGINS> file "ietf-yang-push@2018-02-23.yang"

    <ALEX> removed the ";" </ALEX>

  In general, the module is quite hard to understand, due to the usage
  of all groupings.

  <ALEX> Well, we want to reuse definitions. </ALEX>

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>

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>

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>

o  leaf datastore-xpath-filter

  In the description, do:

    s/'xpath-filter' leaf/'datastore-xpath-filter' leaf/

  <ALEX> OK </ALEX>

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>

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

  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.  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).";
    }

    </ALEX>

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>

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>

o  The YANG module is inconsistently indented.  To get some help, you
   can use the latest version of pyang on github and run:

     $ pyang -f yang --keep-comments ietf-yang-push.yang > x
     $ diff ietf-yang-push.yang x

<ALEX> Thanks.  Updated.  </ALEX>

/martin

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