Re: [Netconf] Last Call on yang-push-17

[Alexander Clemm <[e-mail]>]

Hello Henk,

thank you for your comments!  Replies inline, <ALEX>

Thanks
--- Alex


> -----Original Message-----
> From: Netconf [mailto:[e-mail]] On Behalf Of Henk
> Birkholz
> Sent: Tuesday, August 28, 2018 5:49 AM
> To: [e-mail]
> Subject: Re: [Netconf] Last Call on yang-push-17
> 
> Hi all,
> 
> after a thorough review of yang-push-17 I think it is in good shape and ready
> to progress.
> 
> A few comments below (more of the nit'esque kind):
> 
> 
> In 2. Update Record
> 
> It seems to me that "update record" and "update" are used synonymously in
> this document? If so, please add something like the following to the
> definition of update record- NEW:
> "In this document, update records are often simply referred to as
> "updates"."
> 
<ALEX> ok </ALEX>

> In 3.1. Subscription Model
> 
> OLD:
> "YANG-push subscriptions are defined using a data model that is itself
> defined in YANG."
> 
> I assume there is a good reason for not just phrasing it like - NEW:
> "YANG-push subscriptions are defined using a YANG data model."
> 

<ALEX> sure.  After too much writing, it is easy to get dizzy:-) </ALEX>

> In 3.1. Dampening period
> 
> OLD:
> "The dampening period goes into effect every time an update record
> completes assembly."
> 
> For clarity (and probably implementation guidance) maybe highlight - NEW:
> "The dampening period goes into effect every time an update record
> completes assembly. As long as a dampening period is in effect, the Update
> Trigger functions the same way as with periodic subscriptions, using the
> dampening period as the periodic interval."
> 

<ALEX> Would rather not say that they are the same - because then there is the issue of start time / anchor time etc.  I think it is clear as is.  Of course, one could say, if changes continue to occur during dampening periods, the on-change subscription behaves like a periodic description, with the dampening period as the de-facto periodic interval.  However, I don't think this is needed for clarification and would rather leave it as-is.  
</ALEX>  

> In 3.2.
> 
> OLD:
> "However, there are no guarantees that subsequent requests which
> consider these hints will be accepted."
> 
> NEW:
> "However, there are no guarantees that subsequent requests, which
> consider these hints, will be accepted."
> 
<ALEX>  I don't think the commas are needed (but in German they would be;-) </ALEX>

> In 3.3.
> 
> OLD:
> "In case of a configured subscription, the subscription MAY be suspended."
> 
> NEW:
> "In case of a configured subscription, the subscription MAY be suspended by
> the publisher."
> 

<ALEX> Changed to: "In case of a configured subscription, the publisher MAY suspend the subscription." 
</ALEX>

> In 3.3. conceptual process
> 
> "access control rules" are rather surprisingly introduced here and then never
> mentioned again.
> 

<ALEX> well, it _is_ mentioned and explained in section 3.9.  To make it clearer what is meant here, rephrased the bullet item as follows: 
"Just before a change, or at the start of a dampening period, evaluate any filtering and any access control rules to ensure receiver is authorized to view all subscribed datastore nodes."
</ALEX>

> In 3.3. conceptual process
> 
> Same thing with "patch record" the term is only used in the context of the
> conceptual process and never again. Why is this not an update record? The
> relationship to RFC8072 is elaborated on in 3.5.2., but is surprising to be found
> here already.
> 

<ALEX> Changed as follows:
"Construct an update record, which takes the form of YANG patch record for going from A to B."
</ALEX>

> In 3.3.
> 
> OLD:
> "A publisher SHOULD reject a request for a subscription if it is unlikely that
> the publisher will be able fulfill the terms of that subscription request."
> 
> NEW:
> "A publisher SHOULD reject a request for a subscription, if it is unlikely that
> the publisher will be able fulfill the terms of that subscription request."

<ALEX> I think the lack of a comma is actually correct. </ALEX>
> 
> In 3.5.1.
> 
> OLD:
> "In a periodic subscription, the data included as part of an update
> corresponds to data that could have been read using a retrieval operation."
> 
> See above. Is there a difference between update and update record? If so,
> highlight it, please - Optional NEW:
> "In a periodic subscription, the data included as part of an update record
> corresponds to data that could have been read using a retrieval operation."
> 
> (there are multiple occurrences)

<ALEX> Added the caveat in the terminology section, but changed it to update record here to make it more explicit that yes, indeed, this is what we are talking about.  </ALEX>

> 
> In 3.5.2.
> 
> OLD:
> "Therefore encoding rules for data in on-change updates will generally follow
> YANG-patch operation as specified in [RFC8072]."
> 
> NEW:
> "Therefore, encoding rules for data in on-change updates will generally
> follow YANG-patch operation as specified in [RFC8072]."
> 

<ALEX> ok </ALEX>

> In 3.5.2.
> 
> OLD:
> "However a patch must be able to do more than just describe the delta from
> the previous state to the current state. As per Section 3.3, it must also be
> able to identify if transient changes have occurred on an object during a
> dampening period."
> 
> NEW:
> "However, a patch must be able to do more than just describe the delta from
> the previous state to the current state. As per Section 3.3, it must also be
> able to identify, if transient changes have occurred on an object during a
> dampening period."

<ALEX> Added the comma after "However".  Left the non-comma before "if" but changed "if" to "whether"
</ALEX>

> 
> In 3.6.
> 
> xpath, Xpath and XPath are found in this section. More consistency cannot
> hurt.
> 

<ALEX>, well, xpath refers to the type, whether XPath refers to XPath.  (But got rid of the "Xpath"s, per someone else's earlier comment. </ALEX>

> In 3.7.
> 
> OLD:
> "First it will be used as the initial "push-update" if there is a need to
> synchronize the receiver at the start of a new subscription."
> 
> NEW:
> "First, it will be used as the initial "push-update", if there is a need to
> synchronize the receiver at the start of a new subscription."
> 

<ALEX> Sentence was already changed due to an earlier comment.  It now reads as follows:
"First, it MUST be used as the initial "push-update" if there is a need to synchronize the receiver at the start of a new subscription.  "
</ALEX>

> In 4.3.2.
> 
> OLD:
> "Where it is, the relevant "subscription-id" MUST be encoded as the first
> element within each "push-update" or "push-change-update"."
> 
> NEW:
> "If present, "subscription-id" MUST be encoded as the first element within
> each "push-update" or "push-change-update"."
> 

<ALEX> This has been changed to (also per previous comment) to 
"An "identifier" (that identifies the subscription) MUST be transported along with the subscribed contents.  
			This allows a receiver to differentiate which subscription resulted in a particular push. "
</ALEX>

> In 4.4.1.
> 
> OLD:
> "The specific parameters to be returned in as part of the RPC error response
> depend on the specific transport that is used to manage the subscription."
> 
> NEW:
> "The specific parameters to be returned as part of the RPC error response
> depend on the specific transport that is used to manage the subscription."

<ALEX> done, thank you </ALEX>
> 
> In 4.4.2.
> 
> The non-normative may is intentional here, correct? Also, rpc is also low caps
> here again - more consistency cannot hurt?
> "This rpc error response may contain hints encapsulated within the yang-data
> structure "modify-subscription-error-datastore".
> 

<ALEX> Changed rpc --> RPC.   Also, the text has been changed (may --> SHOULD), also per earlier comments. </ALEX>

> In 4.4.4.
> 
> OLD:
> "This RPC is only applicable only for on-change subscriptions previously
> established using an "establish-subscription" RPC."
> 
> NEW:
> "This RPC is only applicable for on-change subscriptions previously
> established using an "establish-subscription" RPC."
> 

<ALEX> Changed to " This RPC is supported only for on-change subscriptions previously established using an "establish-subscription" RPC.", also in response to earlier request </ALEX>

> In 4.4.5.
> 
> Is this intended to be a guidance section? It seems to me that the use of
> RFC6470 might be a good enough basis to warrant a SHOULD somewhere?
> 

<ALEX> the text has been updated, also per earlier comments by Martin.  
Thanks again for your valuable comments!
--- Alex
</ALEX>

> 
> Viele Grüße,
> 
> Henk
> 
> 
> 
> On 08/14/2018 07:28 PM, Kent Watsen wrote:
> > This message starts a Last Call on draft-ietf-netconf-yang-push-17:
> >
> >    https://tools.ietf.org/html/draft-ietf-netconf-yang-push-17
> >
> >
> > This marks the beginning of the last calls on the yang push suite of drafts.
> > Given the size and number of documents, the chairs decided to break the
> > reviews up into pieces so as to get focus on each in turn.  We are choosing
> > to go top-down, starting with yang-push and ending with the "notif" drafts.
> > We plan to submit the drafts for publication when they are ready as a
> > collective.  The goal is to do all this prior to IETF 103.
> >
> > We understand that, in reviewing yang-push, there is a need to consider
> the
> > subscribed-notifications draft.  We will not be surprised if, in the course
> > of things, both drafts are updated, even though the review is primarily on
> > the yang-push draft.
> >
> > While it's always nice to receive messages of support, at this time, the
> > question isn't so much if the working group supports the work, than if
> > the document is ready to progress.  The chairs need to see reviews that
> > indicate thorough end-to-end reading of the text.  Of course, if there
> > are any objections, these should be brought forward now as well.
> >
> > The current version (-17) of this draft was published on July 1st, just
> > before the IETF 102 meeting.  The datatracker page for the draft is here:
> > https://datatracker.ietf.org/doc/draft-ietf-netconf-yang-push.
> >
> >
> > Thanks,
> > Kent (and Mahesh)
> >
> >
> > _______________________________________________
> > Netconf mailing list
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/netconf
> >
> 
> _______________________________________________
> Netconf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/netconf