BIND 10 #2907: add ConfigurableClientList::getZoneTableAccessor method

[BIND 10 Development]

#2907: add ConfigurableClientList::getZoneTableAccessor method
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  pselkirk
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130528
         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 vorner):

 * owner:  vorner => pselkirk


Comment:

 Hello

 There seem to be some mix of tabs and spaces here:
 {{{#!c++
 <tab>}

 <tab>// If caching is disabled for the named data source, this will
 <tab>// return an accessor to an effectivley empty table.
         return (boost::shared_ptr<const ZoneTableAccessor>
                 (new internal::ZoneTableAccessorCache(*config)));
     }
 }}}

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

 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?

 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:
 {{{#!c++
     void accessorIterate(boost::shared_ptr<const
 ZoneTableAccessor>accessor,
 }}}

 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.

 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. Also, if this
 `EXPECT_FALSE(it->isLast())` failed, the rest of the code would probably
 crash. If you rely on the check to pass in the rest of the code, use
 `ASSERT_` instead of `EXPECT_` (the first aborts the function, the second
 only logs failure, but continues).

 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.
 {{{#!c++
         "   \"cache-enable\": false,"
         "   \"cache-zones\": [\"example.org\"],"
 }}}

 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.

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