BIND 10 #2209: define and implement ConfigurableClientList::getCacheZoneUpdater()

[BIND 10 Development]

#2209: define and implement ConfigurableClientList::getCacheZoneUpdater()
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121106
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  background zone loading            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''others'''

 I made a few minor style(-ish) changes.

 '''client_list.h'''

 - `ReloadResult`: ZONE_RELOADED is not really appropriate for the
   return value of `getCachedZoneWriter()`.  We need to either rename
   it something more general or introduce a separate success code
   for `getCachedZoneWriter()` (note: the latter has a drawback that it
   will increase the risk of having a missing case in `switch-case` usage).

 - `ZoneWriterPtr`: it looks a bit awkward to be defined within
   `ConfigurableClientList` especially if it's public.  I'd suggest
   either hiding it as private or move it to memory/zone_writer.h.

 '''client_list.cc'''

 - revised reload(): I feel a bit nervous about using result.first as
   the return value.  While the code is actually correct, we do a lot
   of things (actual loading job) between getting the 'result' and
   returning it.  It also implicitly assumes if result.second is true
   (non NULL) the first should be ZONE_RELOADED (again, not incorrect,
   but makes me nervous too).  I'd rewrite it a bit, for example, as
   follows:
 {{{#!cpp
     const ZoneWriterPair result(getCachedZoneWriter(name));
     if (result.first != ZONE_RELOADED) {
         return (result.first);
     }

     assert(result.second);
     result.second->load();
     result.second->install();
     result.second->cleanup();

     return (ZONE_RELOADED);
 }}}
   as for whether we still need reload(), I don't have a strong
   opinion.  But if we don't see specific need for it, it's probably
   better to clean it up (maybe later) in the sense of YAGNI.  Any
   existing code increases maintenance costs.

 '''memory_client.cc'''

 - `InMemoryClient` constructor: it seems to be not exception safe
   about zone_table_segment_.  This may become a non issue if you
   migrate to the "formal" version of `ZoneTableSegment::create`, but
   we should check that again.

 '''memory_client.h'''

 - `getZoneTableSegment()`: I guess we should eventually let
   `ClientList` (or at least something outside of `InMemoryClient`)
   directly manage `ZoneTableSegment`, at which point `InMemoryClient`
   will probably take it as a parameter to the constructor.  But for
   now I can live with this workaround.

 '''client_list_unittest'''

 - what's "plaping"?  You mean "playing"?
 {{{#!cpp
 // plaping with that). Once we deprecate reload(), we should revert this
 }}}

 - not matter much, and may even become a non issue depending on what
   to do with reload(), but I found TYPED_TEST is often heavy in
   building (due to the use of template).  If we keep supporting both,
   maybe we should consider value-parameterized test (TEST_P).  It's
   particular so in this case because the use of different types seem
   to be a hack just so they can be used in TYPED_TEST.

 - this comment doesn't parse:
 {{{#!cpp
         // Can't use ASSERT_NE here, it would wan't to return(), which
         // it can't in non-void function.
 }}}
   "wan't" should be "want"?

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