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:9 vorner]:

 > > >  * 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 didn't mean the size of task now. It was clear from the ticket.
 >
 > But if someone comes to bind10 code, never seen it before, and tries to
 read
 > through it (or debug it or something), every layer that does nothing,
 only
 > joins things together, makes the reading harder. It seems generally
 better if
 > these wrapper/glue classes are not needed and we could call things
 directly.

 Okay, so this is about the API design.  First off, we can discuss
 design matters while we implement it, and IMO it's perfectly okay if
 it results in a pretty different design from the originally planned
 one.  That would indeed be one advantage of doing small tasks
 incrementally starting from a just roughly fixed design.

 Now, on this particular one.  Looking at the interface again, I can
 see it's possible, for example, that we just pass a functor for
 `addRRset` and a callbacks object to `MasterLoader` as separate
 parameters, if you meant something like that.  It may be especially so
 now that we define callbacks as a separate class.

 I guess the question is how we expect the concept of "loader context"
 will be complicated, including more states.  In our current usage,
 that's basically identical to the imaginary `addRRset` functor object,
 so it may make more sense to skip the additional layer of the class.
 On the other hand, if we anticipate the context can have more
 complicated states, it probably makes more sense to define a separate
 class (as we do now) than passing each of them as a parameter to
 `MasterLoader`.

 Right now I'm not sure which one is better.  If you want we can
 discuss it further, maybe at bind10-dev with others, and/or at the
 next team call.

 > > - 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?
 >
 > I'm not sure about it either, but it was in the proposed signature.
 Should I remove it?

 Hmm, I didn't realize it was in the very original proposal, and I
 don't remember why I included it there...in that case we should
 probably just remove it.  BIND 9's log message doesn't have this
 information either, so at least we are still compatible with BIND 9.

 > > - `addRrset` test: shouldn't we refill in expected_rrsets_ before the
 > >   second round of test?
 >
 > I don't think I understand what you mean.

 Apparently I misunderstood the original code.  Please ignore this
 comment.

 About the revised version of the branch:

 '''loader_callbacks.h'''

 - I guess we can simply use true-false check for boost::function
   objects:
 {{{#!cpp
         if (!error_ || !warning) {
             isc_throw(isc::InvalidParameter,
                       "Empty function passed as callback");
         }
 }}}

 '''loader_context'''

 - `LoaderContext::handleError` has a duplicate log argument for
   "line".  It's a trivial error so I fixed it myself.  If it's not
   your usual practice I suggest you check the actual log output if you
   modify any of them by "B10_LOGGER_DESTINATION=stdout make check"
   (and grep it if necessary).  The builtbot may also be able to catch
   cases like this (redundant arg), but there can be other errors that
   only humans can detect (such as passing a different type of arg).
 - this comment is now obsolete:
 {{{#!cpp
     /// They need to exist as a real variable. It also needs to be
 mutable,
     /// since the getCallbacks() is const method and it returns a non-
 const
     /// reference. It needs to be non-const, to be possible to call the
     /// the actual callbacks (the operator() might be non-const).
 }}}
   we can probably just remove it, but please check.

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