Hi, Here is my AD review for draft-ietf-netmod-nmda-diff-07. Apologies for the delay. Thank you for writing this document, I think that it is useful, and looks like it is in good shape. Main comments: 1. Should there be any text about how to find out what datastores are supported by a device? E.g., pointing them to either YANG library, or protocol specific mechanisms in the case of RESTCONF. 2. It might be helpful to add a comment about potential issues that could arise by comparing to , i.e., additional differences could be reported due to inactive configuration and template processing between and . 3. I would prefer if 'exclude=origin' was in the reverse sense and perhaps called 'report-origin' instead. With the reverse sense it seems to be safer if new datastores are defined, where otherwise the behaviour could end being under specified. 4. Should there be an option to filter on origin metadata? E.g., only include values that come from intended. Otherwise, things like IP addresses learned from DHCP may always turn up as differences. 5. I'm not that keen on the "Possible Future Extensions" section of an RFC. Personally, I would prefer that this section is deleted, but if you wish to retain it, then please can you move it to an appendix. I've also included some minor comments inline below, and some nits at the end: Abstract This document defines an RPC operation to compare management datastores that comply with the NMDA architecture. The abstract is perhaps somewhat terse. Perhaps: This document defines a YANG RPC operation to compare the contents of network management datastores that comply with the NMDA architecture and return the differences in the YANG-Patch format. 1. Introduction The revised Network Management Datastore Architecture (NMDA) [RFC8342] introduces a set of new datastores that each hold YANG- defined data [RFC7950] and represent a different "viewpoint" on the data that is maintained by a server. New YANG datastores that are introduced include , which contains validated configuration data that a client application intends to be in effect, and , which contains at least conceptually operational state data (such as statistics) as well as configuration data that is actually in effect. I would suggest deleting "at least conceptually", since the datastore does contain all operational state, but it may be implemented as a virtual construct that spans multiple nodes (e.g., linecards) and processes. NMDA introduces in effect a concept of "lifecycle" for management data, allowing to clearly distinguish between data that is part of a configuration that was supplied by a user, configuration data that has actually been successfully applied and that is part of the operational state, and overall operational state that includes both applied configuration data as well as status and statistics. "allowing to clearly distinguish" => distinguishing" "status and statistics" => "status information and statistics" As a result, data from the same management model can be reflected in multiple datastores. Clients need to specify the target datastore to be specific about which viewpoint of the data they want to access. This way, an application can differentiate whether they are (for example) interested in the configuration that has been applied and is actually in effect, or in the configuration that was supplied by a client and that is supposed to be in effect. Perhaps reword the last sentence to match the logical data flow in the server: For example, a client application can differentiate whether they are interested in the configuration supplied to a server and that is supposed to be in effect, or the configuration that has been applied and is actually in effect on the server. When configuration that is in effect is different from configuration that was applied, many issues can result. It becomes more difficult to operate the network properly due to limited visibility of actual status which makes it more difficult to analyze and understand what is going on in the network. Services may be negatively affected (for example, breaking a service instance resulting in service is not properly delivered to a customer) and network resources be misallocated. Perhaps change "actual status" to "actual operational status". I also suggest changing the last sentence to: Services may be negatively affected (e.g., degrading or breaking a customer service) or network resources may be misallocated. 3. Definitions: It should probably define that , , (and perhaps ) are used to indicate names of datastores. It should also explain that is used as the name of a YANG RPC. 4. Data Model Overview At the core of the solution is a new management operation, , that allows to compare two datastores for the same data. Suggest rewording this first sentence to: The core of the solution is a new management operation, , that compares the data tree contents of two datastores. o target: The target identifies the datastore to compare against the source. Suggest adding an example ", e.g., ." o filter-spec: This is a choice between different filter constructs to identify the portions of the datastore to be retrieved. It acts as a node selector that specifies which data nodes are within the scope of the comparison and which nodes are outside the scope. This allows a comparison operation to be applied only to a specific portion of the datastore that is of interest, such as a particular subtree. (The filter dow not contain expressions that would match values data nodes, as this is not required by most use cases and would complicate the scheme, from implementation to dealing with race conditions.) Perhaps "parts/part" rather than "portions/portion". Suggest rewording the last sentence to: Note, the filter does not allow expressions that match against data node values since that may incur implementation difficulties and is not required for normal use cases. o ... When the target datastore is , "origin" metadata is included as part of the patch. Including origin metadata can help in some cases explain the cause of a difference, for example when a data node is part of but the origin of the same data node in is reported as "system". I think that this test needs to refer back to the 'exclude-origin' or 'report-origin' options since the origin metadata isn't always included. 5. YANG Data Model The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT RECOMMENDED', 'MAY', and 'OPTIONAL' in this document are to be interpreted as described in BCP 14 (RFC 2119) (RFC 8174) when, and only when, they appear in all capitals, as shown here. I couldn't see that RFC 2119 language is actually used in the YANG module, so perhaps this can be deleted? rpc compare { description "NMDA compare operation."; Perhaps "NMDA datastore compare operation." anydata source-value { when "../operation = 'delete'" + "or ../operation = 'merge'" + "or ../operation = 'move'" + "or ../operation = 'replace'" + "or ../operation = 'remove'"; description "The anydata 'value' is only used for 'delete', 'move', 'merge', 'replace', and 'remove' operations."; I'm not convinced how useful the when statement really is in this case, since 'source-value' isn't marked as mandatory, a server is allowed to omit it when it doesn't apply anyway. 6. Example does not contain object "description" that is contained in . Another object, "enabled", has differences in values, being "true" in and "false" in . A third object, "name", is the same in both cases. The origin of the objects in is "learned", which may help explain the discrepancies. I think that we should probably refrain from calling them objects, perhaps "leaf instance" would be better? Rather than "differences in values", perhaps "different values". E.g., does not contain an instance for leaf "description" that is contained in . Another leaf, "enabled", has different values in the two datastores, being "true" in and "false" in . A third leaf, "name", has the same instance value in both datastores. The origin of the leaf instances in is "learned", which may help explain the discrepancies. //OPERATIONAL eth0 true There is an extra line, and dodgy indentation for "". Nits: "possibly for" => "possible for" "reference for the" => "the reference data tree for the" "is basis" => "is the basis" Thanks, Rob