Last Call Review of draft-boydseda-ipfix-psamp-bulk-data-yang-model-02
review-boydseda-ipfix-psamp-bulk-data-yang-model-02-yangdoctors-lc-bjorklund-2019-12-16-00

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.