BIND 10 #2376: define LoaderContext class

[BIND 10 Development]

#2376: define LoaderContext class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121120
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:5 vorner]:

 > It should be ready for review. But I have two concerns about the class:
 >  * Since the operator() can be non-const and the getCallbacks is a const
 >    method, we need the ugly mutable keyword. Return a non-reference? Or
 make
 >    the method non-const?

 Hmm, I made my review comments before reading this, and I pointed out
 related/same point.

 Basically, if we want to allow the callback to modify the originating
 context (such as in `LoaderContext::handleError`), we simply cannot
 `getCallbacks()` a const member function; in the original proposal I
 rather thought the callbacks were stateless and can be const.  I see
 two options:

 - just give up making `getCallbacks()` const.  But in that case, I'd
   like to make the `Callbacks` structure safer (I discussed the issue
   in my own comments below).  Introducing a specific constructor,
   hide error/warn variables as private, and (e.g.) have its user call
   the callback via a wrapper method:
 {{{#!cpp
 struct LoaderCallbacks { // to this end this should probably be a class
 public:
     void error(source_name, line, byte, reason) {
         error_(source_name, line, byte, reason);
     }
 private:
     Callback error_;
 };
 }}}
 - force the callback user to keep the const semantics.  For example, I
   think it possible to change `LoaderContext::handleError` so it will
   be a static member function of `LoaderContext` that takes a mutable
   `LoaderContext`:
 {{{#!cpp
 class LoaderContext : public isc::dns::MasterLoaderContextBase {
 public:
     static void handleError(LoaderContext* context, source, line, byte,
 reason) {
         LOG_ERROR(logger,...);
         context->ok_ = false;
     }
 };
 }}}
   Technically, We are here, though, as the callback functor would
   actually holds a mutable reference to `this`, and `getCallbacks()`
   implicitly passes the mutable `this` even if it's defined as a
   const member function of `this`.

 >  * This is another glue class that does nothing :-(. Not that I'd know
 what to
 >    do with it at this stage, but it makes the code gradually unreadable.

 I didn't see any readability difficulty in the branch as someone who
 basically knows what we will do with this code in later stages.  I
 think it's a common case of the tradeoff about breaking a specific
 feature into several development tasks, and of the discussion about
 the reasonable "bitable" size of tasks.  I personally don't think this
 one is too small, but as we discussed in the call the other day, in
 future tasks we can discuss it before starting the sprint if someone
 feels a particular task is too small for the overhead.

 '''master_loader.h'''

 - maybe it's even better to define callbacks in a different header
   file (so when it's used in rdata it wouldn't look too awkward).
   but I don't come up with a specific good idea about where, and in
   any case the concept of 'filename' or 'line no.' indicates
   "loading", so probably we simply cannot eliminate the awkwardness
   anyway.
 - is it possible to make error and warning const?  In general, I'd
   like to avoid allowing a user of a class/struct to modify its
   internal members in an arbitrary way.
 - for the same reason, could the return value of getCallback() be
   `const LoaderCallbacks&`?  on a related note, it looks awkward that
   it's a const member function as in its natural implementation the
   corresponding callback should be part of the context in some way,
   at least conceptually.
 - `LoaderCallbacks`: slightly related to the previous point, if "All
   the callbacks must be set" also means they cannot be a null
   function, I'd introduce an explicit constructor and reject bogus
   input (and, of course, such checks on construction isn't useful much
   if the application can freely modify them).
 - I'm not sure if `source_byte` is of any use for error reporting
   purposes.  hmm, maybe to help identify the specific keyword in the
   `source_line` that causes the error?
 - you may want to document the parameter for
   `MasterLoaderContextBase::addRRset()`, although it may look obvious
   from the context and the general description.  Same for the return
   value of `getCallbacks()`.
 - `MasterLoaderContextBase` should have a virtual destructor.
 - `addRRset`: I wonder we might now want to make the parameter
   `ConstRRsetPtr`.  Excluding the old version of in-memory
   implementation, our current implementations shouldn't need to modify
   the given rrset.  On the other hand, since the master loader
   shouldn't need to be able to use the rrset after passing it to
   `addRRset`, it might be more convenient for the user of the loader
   if it can be mutable.  If we keep it mutable, I'd note that
   `MasterLoader` releases the ownership of the rrset on calling
   `addRRset` and won't modify it.  I'd also note that `MasterLoader`
   never gives a NULL pointer.  I'd further note `rrset` wouldn't be
   accompanied with RRSIG (RRSIG `RRsets` will be passed separately in
   a separate call to `addRRset`).

 '''loader_context.h'''

 - general: I'd document possible exceptions for each method, and say
   so if it's exception free.
 - constructor: I'd note `updater` must have been created in the
   "replace" mode (although the context class doesn't check it).
 - `callbacks_`: as discussed in master_loader.h, I guess we might
   revisit the constness of callbacks and of its related methods.

 '''loader_context.cc'''

 - we may want to include the zone name and class in the log messages.

 '''loader_context_test'''

 - not a big deal, but `LoaderContextTest` constructor doesn't have to
   be public (protected is enough)
 - `addRrset` test: shouldn't we refill in expected_rrsets_ before the
   second round of test?

 '''datasrc_message.mes'''
 - do we need quotations for line numbers?
 - in BIND 9, file name and line number are shown in a concise form:
   <filename>:<line_no> I guess such simplified form is better in this
   case in that log messages are generally better to be concise and in
   this case it should be pretty clear what it would mean.  But it may
   be a matter of taste, so I don't push it strongly.

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