This is my YANG doctor review of draft-boydseda-ipfix-psamp-bulk-data-yang-model-02. The review focuses on YANG-specifics only, since I am not familiar with the technology that is modelled. I have compared the modules in this document with the old "ietf-ipfix-psamp", and have focused by review on the differences between these models. Thank you for the well-written and easy to read YANG modules! o The list transport-session has key "name" with the following definition: leaf name { type name-type; description "The name of the transporter session."; } This is a new definition; the list in 6728 doesn't have a key. Since this is a config false list, it is not obvious what this name should be. I assume it is just an arbitrary string that the server makes up to uniquely identify the session. I suggest this is explained in the description of the leaf. o The list "template" in "transport-session" doesn't have any keys. I would assume that either just the "template-id", or perhaps also "observation-domain-id" and "set-id" could be used as key? (compare with the MIB). o The list "field" in "transport-session/template" doesn't have any keys. I would assume that the "ie-id" could be used as key? (compare with the MIB). o The old "destinationIPAddress" was a leaf-list of ip addresses, but is now replaced with a single leaf of type inet:host. Why isn't this a leaf-list? Also, this leaf is defined as: leaf destination-address { type inet:host; description "Destination IP address or hostname. A hostname may resolve to one or more IP addresses."; } } The description should specify what should happen in the case that the hostname resolves to more than one IP address. Will there be one session per IP address? Or will they be tried round-robin? Or something else? o There are a couple of nodes in "ietf-ipfix-packet-samplig" that use the XPath funtion "name" in "when" expressions, e.g.: leaf export-interval { when "name(..) = 'permanent-cache'" { You shoud use "local-name" rather than "name". "name" returns a QName, and the prefix part of that QName is implementation dependent. I have seen this trick in a couple of modules. I understand why it is used, but it really makes the groupings non-reusable. An alternative design would be to do: grouping flow-cache-base-parameters { leaf max-flows { ... } } grouping flow-permanent-cache-parameters { uses flow-cache-base-parameters; leaf export-interval { ... } } grouping flow-timeout-cache-parameters { uses flow-cache-base-parameters; leaf active-timout { ... } leaf idle-timout { ... } } o The leaf is-flow-key has this when expression: leaf is-flow-key { when "(name(../../..) != 'immediate-cache') and ((count(../ie-enterprise-number) = 0) or (../ie-enterprise-number != 29305))" { It should use 'local-name' as above. But "count(../ie-enterprise-number)" will always return 1, since "ie-enterprise-number" has a default value, so this XPath expression needs to be fixed. o The YANG modules have some references to RFC 5101, which is obsoleted by RFC 7011.