BIND 10 #374: Writable datasources: Basic support for writables in C++ datasrc library

[BIND 10 Development]

#374: Writable datasources: Basic support for writables in C++ datasrc library
------------------------------+---------------------------------------------
      Reporter:  jelte        |        Owner:  UnAssigned
          Type:  enhancement  |       Status:  reviewing 
      Priority:  major        |    Milestone:            
     Component:  data source  |   Resolution:            
      Keywords:               |    Sensitive:  0         
Estimatedhours:  0.0          |        Hours:  0         
      Billable:  1            |   Totalhours:  0         
      Internal:  0            |  
------------------------------+---------------------------------------------

Comment(by jinmei):

 continuing review...still ongoing.

 '''data_source.h'''
  - I made some trivial editorial changes to the branch (r3500)
  - you don't have to include dns/message.h.  it should just suffice to
    add a forward declaration of Message in the isc::dns name space.
  - it's mostly the same for rrset.h, if we change ConstRRsetPtr to const
 RRset& where possible as I suggested above.  The only tricky part is the
 return value of callbackHelper*(), but I guess it could be a bear pointer
 in this context

  - callbackHelperXXX(): I wonder whether it might make sense to do it
    in more C++-ish way.  For example, we might define the
    "RRsetEnumerator" class, whose operator() returns the "next"
    RRset(ptr).  We also define derived classes
    IteratorRRsetEnumerator, VectorRRsetEnumerator, etc, depending on
    the internal container.  Then doIXFR() would look like:
 {{{
 DataSrc::WriteResult
 DataSrc::doIXFR(DataSrcTransaction& transaction,
                 RRsetEnumerator& enumerator)
 {
     ...
     ConstRRsetPtr final_soa = enumerator();
     if (!final_soa) {
         return (W_ERROR);
     }
     ConstRRsetPtr first_soa = enumerator();
     if (!first_soa) {
         return (W_ERROR);
     }
     ...
 }
 }}}
    This is not only about a matter of style, but also about more
    practical advantages.  That is, this way we can make it type safer
    by not relying on passing around void* values and cast them to
    particular type of values.  Also, with this approach we can hide
    derived classes like "VectorRRsetEnumerator" in the test code (it
    seems to me this one is introduced for the convenience of tests; in
    practice we'd normally if not always use RRsetIterator).

    Alternatively we could make doIXFR(), etc templated so that it can
    take generic iterators.  It might be more C++-ish, although we
    couldn't do that in this current design because doIXFR() is a
    virtual function.

    In either case, for a vector based interface I'd use the begin and
    end iterators instead of an index.
  - AbstractDataSrc::WriteResult I'd make the comment for each enum
    value a doxygen description (using '//<')
  - DataSrcTransaction
   - I'm not sure if it makes sense to attribute "zone name/class" to
     this class, because a transaction in a data source may not always
     be specific to a single zone.  for example, we may want to add
     more than one zones to a data source as an atomic transaction.
     Also, even for the case where we add a single zone (which is not
     possible through the current public interface though), since the
     zone information is an initial attribute of DataSrcTransaction, it
     would look like
 {{{
     data_source.addZone(trans);
 }}}
     this is a bit awkward in terms of interface consistency because
     when we add an RRset we pass the RRset(ptr) to be added.

   - so, I wonder whether we might make this class is a smaller
     primitive: just holding a scoped transaction for a data source,
     similar to the Boost scoped_lock.  Then the code would look like
     as follows:
 {{{
     void doIXFROrSomething(data_source) {
         DataSrcTransaction trans(data_source);
         //
         // do whatever necessary within the transaction
         //
         trans.commit();
     }
 }}}
     Below, however, I'll continue the review assuming the current
 interface for now.
   - member variable names shouldn't begin with "_" (which is (maybe by
     convention) reserved for standard definitions, in my understanding)
   - it may be better to provide doxygen description for states?  and,
     according to the guideline, "states" should probably better be
     "States".
   - constructor: 'explicit' is not necessary for multi-parameter
     constructor.
   - destructor: since the behavior of the destructor may be critical
     (see the comment to its implementation), I think we should add
     some note in the doxygen style.
   - the getters should better be member const functions.
   - 'const' to the returned object type is redundant (getZoneName()
     and getZoneClass())
   - I'd define _zone_name and _zone_class as const.
   - I'm not sure why we pass a pointer of data source.  can it be
     NULL?
   - getZoneName() could return a reference to _zone_name, which would
     be a bit more efficient.
   - I'd feel nervous about the setters (setState() and setData()).
     Due to them _state and _data are effectively public member
     variables, and make the class quite fragile to careless (buggy)
     usage.  for example, a buggy implementation could set the state
     from "RUNNING" to "INIT" (or to "DONE" without commit or
     rollback), which would cause something very weird.  Likewise, an
     application may modify or even reset to NULL the internal data.
     Since the Sqlite3DataSrc implementation assumes it's always
     a valid pointer containing expected data, it would make the
     application crash.  We could blame the application saying it's
     "their bug", but it would be much better to prohibit such risky
     usage via the interface.
   - A simpler "scoped transaction" would be one possibility.  Another
     is to introduce a class hierarchy of transactions from a base
     abstract class to a specific derived class per data source.  With
     this approach, we define e.g. Sqlite3DataSrcTransaction derived
     from base DataSrcTransaction and have an instance of
     Sqlite3DataSrc create an associated transaction:
 {{{
     DataSrcTransaction* trans = data_source.createTransaction();
     // here, if data_source is of Sqlite3DataSrc, trans is of
     // Sqlite3DataSrcTransaction.

     data_source.addRRet(*trans, rrset); // or something like this
 }}}
     Then, we can hide the interface and implementation within the
 definition
     of Sqlite3DataSrcTransaction.
   - getDataSource() doesn't seem to be used.  since it's a mutable
   "handle", it would be better to not to disclose this information
   until we see the real need for it, that is, I suggest removing this
   method for now.
  - DataSrc class: the above design-level comment would also affect the
    modifications to DataSrc, but here I'll review it as is.
   - in general, I'd like to know whether/when the methods throw an
    exception.
   - addRRset(): '\return' is missing, in which W_SUCCESS should be.
    I'd be interested in what happens if the same <name,type> of rrset
 already
    exists.
   - delRRset(): \return is missing.  what if no such RRset exists?
   - delZone(): there should be a period at the end of \brief line.
    otherwise doxygen considers the next line a part of the brief
    description.  what if no such zone exists?

 (to be continued)

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