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      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:8 jinmei]:
 > - `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).

 Sorry, I was postponing the change during the work, until I forgot about
 it O:-).

 > - 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.

 It doesn't really reflect the flow of control as I see it (I think about
 it as „return the same thing as the getCachedZoneWriter did and use the
 writer if there's one“). But as this is a temporary state only probably, I
 don't mind, so I changed it.

 I think it should be cleaned up, but the auth server still uses it now. I
 think
 we'll remove it after we start using the background loading, since that
 one
 will naturally switch to not use the reload() method.

 > '''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.

 I guess both of these would be fixed once #2208 is merged into this, since
 this
 code won't be present. Which should be pretty soon. If this code doesn't
 disappear by then, I'll look into them.

 > - 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.

 Hmm. It seems that the size of change would not be really small and we'll
 probably remove the reload() soonish. Then it would be better to just
 remove
 the TYPED_TEST ones and unify the test fixture classes. I'd like to keep
 it
 this way for the short time, since the time spent on compiling the
 templates
 will definitely be smaller than the time I'd spend changing it.

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