BIND 10 #422: Implement memory Data Source Class

[BIND 10 Development]

#422: Implement memory Data Source Class
------------------------------+---------------------------------------------
      Reporter:  zzchen_pku   |        Owner:  zzchen_pku
          Type:  task         |       Status:  reviewing 
      Priority:  major        |    Milestone:            
     Component:  data source  |   Resolution:            
      Keywords:               |    Sensitive:  0         
Estimatedhours:  0.0          |        Hours:  0         
      Billable:  1            |   Totalhours:  0         
      Internal:  0            |  
------------------------------+---------------------------------------------
Changes (by jinmei):

  * owner:  UnAssigned => zzchen_pku


Comment:

 There doesn't seem to be a critical problem, but I have some
 design-level comments and comments on the documentation.

 '''zonetable_unittest'''
  - I believe we can simply use ConstZonePtr() (default constructed)
    instead of NULL + cast.  The static_cast hack used in the old code
    is to work around a peculiar compiler behavior with a bear value of
    NULL.  ConstZonePtr is a normal class type in terms of the type
    system so we shouldn't need a cast.

 '''memory_datasrc.h'''
  - this header file must include zonetable.h (mainly for ZonePtr).
  - the class description: MemoryDataSrc is not just "a set of
    authoritative zones"; it's a data source that uses in memory
    dedicated backend.  Likewise, at the level of data source
    abstraction, it's not appropriate to mention the internal data
    structures (or classes) like "(zone) table".  Users of this class
    doesn't have to (or shouldn't) care about whether it's a table or
    not; for the users a MemoryDataSrc just magically provides the
    requested answer (Note: this is for the general description of the
    class or method interfaces - it's okay or sometimes even necessary
    to talk about the internal details in developer's notes).  Please
    update the overall documentation at a higher abstraction level.
  - For the same reason, I believe we should use a more generic naming
    space for result codes than "ZoneTable::" (note that we'll
    eventually use these codes in other data sources, which may or may
    not contain a ZoneTable object).  My current suggestion is to
    define a new sub namespace datasrc::result and define enums in that
    space:
 {{{
 nanespace datasrc {
 ...
 nanespace result {
 enum Result {
         SUCCESS,  ///< The operation is successful.
 ...
 };
 }
 }}}
    (again, we should be at one level higher in abstraction and avoid
    using specific data structures or classes like ZoneTable)
    Once this is done, both ZoneTable and MemoryDataSrc use these
    enums.  We may eventually need to do the same thing for zones, but
    for now let's minimize the changes.
  - on reading the resulting code, I now think add()/remove()/find()
    should actually be named "addZone()/removeZone()/findZone()",
    respectively.
  - many doxygen comments are (essentially) a duplicate of those of
    zonetable.h.  This isn't good as any duplicated effort.  My
    suggestion is to revise zonetable.h so that the descriptions simply
    refer to the corresponding interfaces of memory_datasrc.h.  I'd
    also add a note that the ZoneTable class is primarily intended to
    be used as a backend for the MemoryDataSrc class, but is exposed as
    a separate class in case some application wants to use it directly
    (e.g. for a customized data source implementation).
  - the description of the default constructor doesn't match the
    implementation:
 {{{
     /// This constructor internally involves resource allocation, and if
     ...
 }}}
    (This indicate we shouldn't naively copy the description of similar
    class).  Also, we don't even have to explicitly declare and define
    it (and also the destructor) if we use the compiler generated
    implementations (but note the comment about pimpl below, which
    would request a non default constructor and destructor, and make
    the description valid again)
  - Some comment lines are too long.  I suggest lines be folded so that
    they won't be longer than 79 characters (as recommended in BIND 9's
    coding guidelines).
  - Please add blank lines between the code and (next) doxygen
    document for readability, e.g. from
 {{{
     /// See the description of specific methods for more details.
     typedef ZoneTable::Result Result;
     /// \brief A helper structure to represent the search result of
 }}}
    to:
 {{{
     /// See the description of specific methods for more details.
     typedef ZoneTable::Result Result;

     /// \brief A helper structure to represent the search result of
 }}}
  - I'd use the pimpl approach for this class, because its overhead is
    marginal for this class (MemoryDataSrc shouldn't be instantiated so
    often) and it's quite possible that we'll have more member
    variables as we implement more features for the data source.

 '''memory_datasrc_unittest'''
  - Most of the tests are a duplicate of those for zonetable_unittest
    and meaningless.  As long as we use ZoneTable as the backend in the
    straightforward way like we do now, I'd simply test a couple of
    trivial cases to confirm the interface (leaving a comment about why
    the tests are so simple)

-- 
Ticket URL: <http://bind10.isc.org/ticket/422#comment:3>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development