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-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 vorner):

 * owner:  vorner => pselkirk


Comment:

 Hello

 Replying to [comment:13 pselkirk]:
 > > 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.

 My point wasn't that the data source selected would not be known. My point
 was that if I called `getCacheZoneWriter` with a zone name and empty data
 source name, I could get a different data source than when I call the
 `getZoneTableAccessor` with the same zone name.

 Anyway, it seems to be fixed now, so the reason does not matter.

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

 Then you think about it too much. The const reference does not need any
 thinking about it all. The only difference is there's no internal copy of
 the data, which makes it more light-weight. All reference magic is
 happening under the hood by C++ compiler and all effects of the fact that
 it is the same instance and not the other is mitigated by the const.

 The interfaces in ticket descriptions are more for illustration purposes
 than binding.

 And, actually, I thought we did have a note about passing of the shared
 pointers as const references wherever possible in the code guidelines. But
 I didn't find it there now. So I'm going to ask on the ML if we should
 update the guidelines.

 > > 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 is more readable now. However, the special-case for `numZones == 0`
 is not necessary and will follow of the basic loop naturally.

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

 These are directly related to the configuration, not to the client list
 itself.

 So, is the zone table accessor needed only when configuring it, or is it
 needed during general server handling? I must admit I'm hopelessly lost in
 the amount of layers and action classes (accessors, writers, etc), it is
 easier to me to imagine objects being actual „things“ than „actions“
 (maybe that's why I never really understood monads in haskell, beyond the
 obvious).

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