BIND 10 #2907: add ConfigurableClientList::getZoneTableAccessor method

[BIND 10 Development]

#2907: add ConfigurableClientList::getZoneTableAccessor method
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130611
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by pselkirk):

 * owner:  pselkirk => vorner


Comment:

 Replying to [comment:12 vorner]:
 > There seem to be some mix of tabs and spaces here:

 I've futzed my .emacs, so this hopefully shouldn't happen again. (My `c
 -default-style` didn't seem to apply entirely to C++ for some reason.)

 > Isn't there some method to find a matching data source already? Instead
 of implementing yet another BOOST_FOREACH loop.

 I don't know. I modelled this on
 `ConfigurableClientList::getCachedZoneWriter` per Jinmei's suggestion.

 > Also, the loop seems to match differently than the rest of the class
 does. Especially, this skipping of cache-less data sources might lead to
 discovering a different data source than the rest of the code, which might
 be confusing and cause bugs. Is there a rationale behind it?

 If `datasrc_name` is empty, `ConfigurableClientList::getCachedZoneWriter`
 searches for the first data source with the matching zone name. So using
 an empty name is already somewhat non-deterministic as to which data
 source it will match. However, I don't object to removing that check, and
 returning the first data source unconditionally.

 > I don't know if there's an explicit mention of templated types in the
 style guide, but we usually write a space after the type name, even if the
 type name contains a greater-than sign:

 Okay, thanks, fixed.

 > Also, as shared_ptr is not a primitive type, it may be better to pass it
 as `const shared_ptr<...> &` (reference), so the counter is not
 incremented and decremented needlessly.

 Passing a reference to a pointer (even if it's not a true primitive
 pointer) makes my head hurt. In any case, I'll defer to Jinmei, who
 specified the interface.

 > The cycle iterating from 0 to `numZones` with `i` seems awkward, as `i`
 is never used inside. After little bit of thinking, I discovered the
 purpose, but I believe it would be more readable to iterate through the
 iterator, count the elements there and then check the counts match.

 Okay, that is the point of the check - to iterate through the table and
 verify that it a) has the expected number of zones, and b) it has the
 expected zone name. But I recoded.

 > This bit of configuration makes little sense. If `cache-enable` is
 false, `cache-zones` is not used at all. It is useless (and misleading) to
 include it then.

 All right. It's not actually illegal, and passes the parser, but I don't
 object to removing unnecessary code.

 > Also, while Jinmei's „yes“ to the first question is ambiguous, I believe
 it was for the second option, to add an abstract method to the base class.

 But then we should also add abstract methods for:
 * `configure`
 * `getConfiguration`
 * `getCachedZoneWriter`
 * `getDataSourceClient`
 * `getStatus`
 * `getDataSources`

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