Last Call Review of draft-ietf-rtgwg-yang-rib-extend-03
review-ietf-rtgwg-yang-rib-extend-03-yangdoctors-lc-hopps-2020-06-12-00

* Review of draft-ietf-rtgwg-yang-rib-extend-03

** Questions:

- Should the routing-state branch also be augmented?

- Preferences are only be added to static routes it seems. Aren't preferences a
  generic RIB concept though that determines which route the RIB selects as
  active? Is there are reason to not add them as operational state for all routes?

- Why are repair-routes (which are really repair-paths i.e., a set of next-hops)
  defined globally and then referred to by ID with leaf-refs, while normal sets
  of next hops are not defined this same way (i.e., they are defined inline with
  the route)?

** Major

- Experience has shown that examples not only help users but also serve as
  validation of the module schema itself. I think the document should at minimum
  include an example which fully exercises the module (and is validated).
  ([RFC8407] "Module Usage Examples").

  My review will assumes the eventual existence of this validated example rather
  than trying to do by a bye-eye validation of various issues it would catch
  (e.g., that xpaths free of typos, etc)

** Minor

- In a few places more descriptions and reference are warranted . I think the
  level of expertise being assumed here may be too high (i.e., at the expert
  routing geek level vs someone with basic routing fundamentals knowledge), some
  comments following are specific to this point.

*** Module

- "What's a metric?" A bit more description could be added. As this module is
  adding both metrics and preferences I think it could be more specific about
  what they both are. The preference description for static routes is pretty
  good as it says what it is and how it's used. There's nothing similarly given
  (in the module) though for the metric leaf. Metrics are briefly mentioned in
  the document Introduction. This could be as simple as saying something in the
  module like "The metric value as defined by the source of the route". But I
  feel like maybe more needs to be said about whether the RIB itself ever uses
  these metrics (it doesn't right?). Its a bit confusing I think b/c of the
  presence of possibly unequal metrics in the set of nexthops for a given,
  possibly active, route.

- Appearing in multiple leafs: what is a "tag"s vs "application-tag"s. There's a
  description of "application-tag" in the module that uses "tag" to help define
  itself, but nothing for the "tag" its referring to. At least a reference or
  better, a few references should be given for these tag types.

  Routing experts with years of experience probably know what these are
  implicitly, but probably not others.

- "repair-route" should probably be named "repair-path". Unlike their sibling
  "routes" they only really exist inside an actual route (i.e., they have no
  prefix to stand on their own as a "route").

** Nits

*** Text

Section 3, last words of first paragraph:

  "monitor RIB." -> "monitor a RIB"?

*** Module

- "path-attribute" grouping, should probably rename this "repair-path-attribute"
  (or something similarly more specific since it's only adding one thing) it
  makes "uses repair-path-attribute" more obvious.