Re: [Netconf] WGLC on restconf-notif-08

["Reshad Rahman (rrahman)" <[e-mail]>]

Thanks for the review, inline.

On 2018-10-10, 7:45 AM, "Netconf on behalf of Martin Bjorklund" <[e-mail] on behalf of [e-mail]> wrote:

    Hi,
    
    I think that this document needs some fixes before it is ready.  Here
    are my comments.
    
    
    o  Title and Abstract
    
      This document is called:
    
         RESTCONF Transport for Event Notifications
    
      but netconf-notif called
    
         NETCONF Support for Event Notifications
    
      The titles should probably be aligned.
<RR> Ack.
    
      ... and note the document that they support is called
    
         Customized Subscriptions to a Publisher's Event Streams
    
      It is not obvious that "RESTCONF Transport for Event
      Notifications" relates to "Customized Subscriptions to a Publisher's
      Event Streams".
<RR> So you're suggesting "RESTCONF Support for Customized  Subscriptions to a Publisher's Event Streams" (and likewise for NETCONF)?
    
      Also, the abstracts should probably also be aligned.
<RR> The 1st paragraph is identical,  I'll remove the 2nd paragraph of abstract in RESTCONF-notif.    
    
    o  Section 3.1
    
      Last sentence:
    
        A subscriber can then attempt to re-establish.
    
      Please clarify what this means.  Specifically, the subscriber needs
      to re-send the "establish-subscription" and do a new GET on the new
      URI.  (right?)
<RR> Will clarify and right.    
    
    o  Section 3.2
    
       The reference RFC6520 is incorrect, it should be 8040.
<RR> Correct.    
    
    o  Section 3.3
    
      It seems this text starts a itemized list, but it probably
      should not be a list:
    
       o  In case of error responses to an "establish-subscription" or
<RR> Correct.    
    
    o  Section 3.4
    
      The text says:
    
       An HTTP GET is then sent on a separate logical connection (b) to the
       URI on the publisher.
    
      I think that is also ok if the GET is sent on (a) - in the case that
      you don't care about being able to modify the subscription.  This
      should be clarified.
 <RR> Yes that also works, will add some text.
   
      Also, "modify-subscription" may be sent on some other session.
<RR> As long as from same subscriber I assume. Do we care about NAT scenarios?   
    
    o  Section 4
    
       o  take any existing subscription "priority" and copy it into the
          HTTP2 stream priority, and
    
    
       What is the 'subscription "priority"'?  Can you add a reference.
<RR> Will do, dscp leaf in SN draft.    
       Can you also add a reference to HTTP2 stream priority?
 <RR> Will do, section 5.3 of rfc7540.   
    
    o  Section 4
    
       o  take any existing subscription "dependency" and map the HTTP2
          stream for the parent subscription into the HTTP2 stream
          dependency.
<RR> Will do, dependency leaf in SN draft and section 5.3.1 of rfc7540.       
      (same comments as above wrt. references)
    
      This text seems to imply that there in fact exists a HTTP2 stream
      for the "parent subscription".  I don't think is necessarily true.
<RR>  So just clarify that this applies only when there's an HTTP2 stream for the parent subscription?  
    
    o  Section 5
    
      In RESTCONF in general, JSON is not mandatory.  I don't think this
      document should change that.
<RR> I agree. 
    
    
    o  Section 5
    
      OLD:
    
       A publisher supporting [I-D.ietf-netconf-yang-push] MUST support the
       "operational" datastore as defined by [RFC8342].
    
      NEW:
    
       A publisher supporting [I-D.ietf-netconf-yang-push] MUST support the
       operational state datastore defined in [RFC8342].
    
      ... but why do we specify conformance requirement for YANG push in
      this document?
    
<RR> I think this is left over from earlier days, so will remove this. Looks like the whole section can be removed.    
    o  Section 6
    
      You should refer to Section 6.4 in RFC 8040 instead of RFC 5277.
<RR> Ack    
    
    o  Section 8
    
      Should the identity "restconf" be derived also from
      "sn:configurable-encoding"?
<RR> You are correct. And it was there in previous versions which had the http1.1/http2 transport identities.    
    
    o  Section 8
    
      Why is "uri" augmented to "subscription-modified"?   The URI can't
      change dynamically, right?
<RR> It shouldn't change so not needed. 
    
      Should the leaf "uri" be renamed to "location", to align with 8040?
<RR> I don't feel strongly about this one way or the other. Would like to hear from others.    
    
    o  A.1.1
    
      The example POST is:
    
       POST /restconf/operations/subscriptions:establish-subscription
    
      This is not correct, it should be
    
       POST /restconf/operations/ietf-subscribed-notifications:establish-subscription
    
      (same for all POSTs in the examples)
<RR> Ack.    
    
    o  A.1.1
    
      The example reply is:
    
    
       {
          "id": "22",
          "uri": "/subscriptions/22"
       }
    
      and then the GET is:
    
       GET /restconf/operations/subscriptions/22
    
    
      This isn't correct.  Perhaps change to:
    
          "uri": "https://example.com/restconf/subscriptions/22"
    
     and
    
       GET /restconf/subscriptions/22
<RR> Ack.    
    
    o  A.3
    
      The example of a subtree filter is not correct:
    
      OLD:
    
           "stream-subtree-filter": "/ietf-vrrp:vrrp-protocol-error-event/",
    
      NEW:
    
           "stream-subtree-filter": {
             "ietf-vrrp:vrrp-protocol-error-event": {}
            }
 <RR> Ack. Also s/XPATH/XPath in Figure 16.   
    
Regards,
Reshad.
    
    /martin
    
    _______________________________________________
    Netconf mailing list
    [e-mail]
    https://www.ietf.org/mailman/listinfo/netconf