based on draft-21 ietf-subscribed-notifications@2018-12-19 General Comments: -- the document is well-written and the feature-set is very comprehensive. There are a lot of YANG features (10) but they are defined to allow for a large set of publisher platforms. -- the extensibility mechanisms for filters, transport, and encoding should be valuable over time. There is a risk of proprietary solutions that do not interoperate but this base YANG module provides enough functionality. Reliance on future augmentations is new but should be OK -- advanced use of groupings, refine-stmt and augment-stmt within groupings makes the module difficult for humans to fully understand without additional tools, and without the normative text outside the YANG module. Issues: I1) 1.4. Relationship to RFC 5277 -- this section should mention that stop-time is not coupled to notification replay like RFC 5277. This parameter can be used on any stream, even if replay is not supported at all. I2) sec 2.1 para 5: If subscriber permissions change during the lifecycle of a subscription and event stream access is no longer permitted, then the subscription MUST be terminated. -- Why can't server suspend until NACM configured (i.e., MUST be terminated or suspended). Should be clear somewhere that suspend is for CPU and other resources, and NACM not considered to be a resource. I3) sec 2.1 para 6: Event records MUST NOT be delivered to a receiver in a different order than they were placed onto an event stream. -- does this apply to subscription-state? Think not, they are not events placed in event stream. Need to allow ended or suspended to be sent head-of-line whenever state changes -- Maybe another table or more text should be added listing the notifications that indicates (a) sent-for-configured (b) sent-for-dynamic (c) inserted at end of event stream, middle, or sent head-of-line replay-completed Y Y end subscription-completed Y N end subscription-modified Y Y middle [1] subscription-resumed Y Y head [2] subscription-started Y N head subscription-suspended Y Y head subscription-terminated Y Y head [3] -- [1] not clear where in the event stream subscription-modified is sent. Maybe implementation-dependent? e.g. does filter-change inserted at slot N means all events N+i are done with new filter? -- [2] not clear where in the event stream subscription-resumed is sent. There may be events ready to send. Clearly resumed is sent before any of these after transition to active state -- [3] not clear that subscription terminated right away then all events for all receivers are deleted and only subscription-terminated is sent to all receivers; sec 2.7.3 says this but not in module -- insert point may be specific to each receiver, not each subscription -- how long does the server wait for a receiver when sending subscription-terminated? I4) sec 2.4.6: RPC Failures -- concern about a subscription-specific error reporting system must make sure protocol error reporting system is used correctly -- The error-tag value needs to be identified for each 'reason' identity 2. "modify-subscription-stream-error-info": This MUST be returned with the leaf "reason" populated if an RPC error reason has not been placed elsewhere within the transport portion of a failed "modify-subscription" RPC response. This MUST be sent if hints -- all 3 paragraphs like this; unclear what "placed elsewhere" text means; not appropriate for MUST; Only 3 fields seem to be relevant (error-tag, error-app-tag, error-info). Protcol operations are expected to document server requirements for these 3 fields, if applicable. Only the error-tag is mandatory-to-use. -- the error assignments are extremely specific. e.g., it is not possible for to fail with an 'insufficient-resources' error; Do not agree that scoping each identity to specific RPC operations is a good idea. -- how are errors in these parameters reported for configured subscriptions when is the RPC that has the error? How are the yang-data structs used for edit-config or commit errors? I5) sec 2.5 Multiple configured subscriptions MUST be supportable over a single transport session. -- why is this a MUST instead of SHOULD? explain harm to interoperability if not supported I6) sec 2.5, para 3: On a receiver of a configured subscription, support for dynamic subscriptions is optional except where replaying missed event records is required. -- confusing because text in 1.3: Note that there is no mixing-and-matching of dynamic and configured operations on a single subscription. Specifically, a configured -- clarify the receiver may have multiple subscriptions here -- not clear what "except where replaying..." text means I7) leaf stream-xpath-filter: [multiple uses] The expression is evaluated in the following XPath context: o The set of namespace declarations is the set of prefix and namespace pairs for all YANG modules implemented by the server, where the prefix is the YANG module name and the namespace is as defined by the 'namespace' statement in the YANG module. -- This prefix processing is not done anywhere else in NETCONF or RESTCONF. IMO a bad precedent. Only the XML prefixes should be required for processing of XML encoding. YANG module prefixes are not required to be unique, unlike the prefix mappings in XML -- NMDA allows the same module to appear in multiple module-sets and different in each datastore. This text about "implemented by the server" does not work for NMDA I8) leaf /subscriptions/subscription/encoding { when 'not(../transport) or derived-from(../transport, "sn:configurable-encoding")'; type encoding; -- when-stmt is odd; there are no configurable-encodings specified -- there should be an example of a configurable encoding provided -- maybe add some text noting this is not the "encoding" leaf in establish-subscription. Text is confusing since not clear how a dynamic subscription would use this leaf inside subscription-policy grouping -- it is only clear in the YANG tree that modify-subscription does not allow encoding to be changed. Is this worth mentioning in the establish-subscription/input/encoding leaf? I9) leaf /streams/stream/description { type string; mandatory true; -- it is odd to see an admin-string be mandatory. should add explanation why this is mandatory (in config false even more odd) I10) leaf /subscriptions/subscription/configured-subscription-state if-feature "configured"; enum concluded { value 3; description "A subscription is inactive as it has hit a stop time, but not yet been removed from configuration."; } -- it is also possible for stop-time to be reached but not all events delivered to all receivers on the subscription. Is this a different state or part of 'concluded'? I11) extension subscription-state-notification { This statement is not for use outside of this YANG module."; -- this text should be removed. There is no value in limiting the scope of this extension. It prevents even this WG from creating a module that uses the extension again. I12) notification replay-completed { description "This notification is sent to indicate that all of the replay notifications have been sent. It must not be sent for any other reason."; -- 2nd sentence should be removed. It is implied that the notification is only sent for its defined purpose. No other notifications have this type of text. I13) notification subscription-started { sn:subscription-state-notification; if-feature "configured"; description "This notification indicates that a subscription has started and notifications are beginning to be sent. This notification shall only be sent to receivers of a subscription; it does not constitute a general-purpose notification."; -- 2nd sentence is confusing; all notifications are sent to receivers of a subscription. last part is redundant since the sn:subscription-state-notification extension is used I14) rc:yang-data modify-subscription-stream-error-info { leaf filter-failure-hint { type string; description "Information describing where and/or why a provided filter was unsupportable for a subscription."; } -- rpc-error already allows more precise error reporting It uses error-tag, error-path, error-string, and error-info extensions to identify which parameters/conditions caused the RPC to be rejected. This error reporting will continue to be used, Not sure this failure-hint has any standards value. Perhaps real-use example can be added I15) notification subscription-completed { sn:subscription-state-notification; if-feature "configured"; description "This notification is sent to indicate that a subscription has finished passing event records, as the 'stop-time' has been reached."; -- Could be more clear: A) replay in use and stop-time has past: * send replay-completed * send subscription-completed B) no replay and stop-time has past: * send subscription-completed I16) leaf replay-previous-event-time { -- It is not clear why this leaf is needed How does this relate to replay-log-creation-time and replay-log-aged-time? Nits: 2.3: This document provide for several QoS parameters -- s/provide/provides 2.4, para 1: -- term 'target' usage is confusing, inconsistent with sec 1.2 reference-stmt: "RFC XXXX:Customized Subscriptions to a Publisher's Event Streams"; -- Does not match document title feature configured: "This feature indicates that configuration of subscription is supported."; -- s/subscription/subscriptions/ refine "target/stream/replay-start-time" { description "Indicates the time that a replay using for the streaming of -- sentence seems mangled; needs repair