BIND 10 #422: Implement memory Data Source Class

[BIND 10 Development]

#422: Implement memory Data Source Class
------------------------------+---------------------------------------------
      Reporter:  zzchen_pku   |        Owner:  jinmei   
          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 zzchen_pku):

  * owner:  zzchen_pku => jinmei


Comment:

 Replying to [comment:3 jinmei]:
 > '''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.
 Done.
 > '''memory_datasrc.h'''
 >  - this header file must include zonetable.h (mainly for ZonePtr).
 Done.
 >  - 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.
 Updated.
 >  - 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.
 Done.
 >  - on reading the resulting code, I now think add()/remove()/find()
 >    should actually be named "addZone()/removeZone()/findZone()",
 >    respectively.
 Updated.
 >  - 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).
 Done.
 >  - 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)
 Done.
 >  - 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).
 Updated.
 >  - 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
 > }}}
 Updated.
 >  - 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.
 Updated to use the pimpl approach.
 > '''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)
 Updated, add more unittest.

 New change has been committed to r3810, thanks.

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