BIND 10 #1975: implement meta-or-container-of data source

[BIND 10 Development]

#1975: implement meta-or-container-of data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120619
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  9
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 The revised version mostly looks okay (I've made some more style
 fixes).  There is one last point to discuss, and a few open nits.

 '''To discuss'''

 > > Likewise, I'd like to make this condition as lightweight as possible:
 > > {{{#!cpp
 > >                     const uint8_t labels(
 > >
 result.zone_finder->getOrigin().getLabelCount());
 > >                     if (labels > candidate->matched_labels_) {
 > > }}}
 > > for performance sensitive setup, it's likely there's only one data
 > > source, and in many cases the search will result in a partial
 > > match [...]
 >
 > How does this help? Notice that the result contains the number of
 matched labels as well, so we should call it for the first DS too. Or, do
 you want to get rid of that part of result?

 Ah, right, I overlooked it.  Hmm, with correcting that, I was about to
 okay with it...but on thinking it further, I realized `getOrigin()`
 may not be always cheap when we adopt memory-efficiency data structure
 for in-memory cache.  The origin name may be encoded in some
 memory-efficient form and getOrigin() call may require constructing a
 Name object from it.  Maybe my concern is premature, but if we don't see a
 specific need for returning the label, I'm inclined to omit it from
 the result structure (after all, if the caller really needs it, it can
 then call getOrigin()->getLabelCount() itself), and optimize the
 find() code so it doesn't have to call getOrigin() for the first
 partial match case.

 '''Nits'''

 I'd leave these to you.  Just pointing them out.

 - file name: list.{cc,h} seems to be too generic and have a risk of
   causing naming conflicts in future.  I'd personally be more specific
   like client_list.{cc,h}

 - This doesn't parse well:
 {{{#!cpp
         /// Note that the pointer is valid only as long the ClientList
 which
         /// returned is alive and was not reconfigured. The ownership is
         /// preserved within the ClientList.
 }}}
   should be "as long as the ClientList which returned the pointer is
 alive"?

 - spacing policy between '{' and '}' isn't consistent:
 {{{#!cpp
         DataSourceInfo() :
             data_src_client_(NULL)
         {}
         DataSourceInfo(DataSourceClient* data_src_client,
                        const DataSourceClientContainerPtr& container) :
             data_src_client_(data_src_client),
             container_(container)
         { }
 }}}
   I'd personally not insert space here, and as far as I can see that
   style is adopted in larger parts of the code (while not documented
   in the guide).

 - about the braces in switch:

 > > - ConfigurableContainer::search: I don't understand why we need the
 > >   curly braces for each case.  Can't we do it like this?
 >
 > The `PARTIALMATCH` needs the curly braces, since it contains a variable.
 So I added it to the other cases too, for consistency. Is there a reason
 not to have them, when we have them for single-line if too?

 Does it really need braces?  The scope of the variable is the inner if
 block, so I don't think brace is required here.  In fact, it compiles
 perfectly fine for me without braces.  Besides, personally, I think if
 we need to have a relatively complicated things in a case that
 requires braces (as a result of defining local variables, etc), that
 means the code design should be reconsidered because it's quite likely
 to suggest the entire switch block is getting too big, decreasing
 readability.

 '''Other points'''

 Just following up with selected discussion points, not raising an
 issue for the code.

 > > - is this safe?
 > > {{{#!cpp
 > >         set<Name>::const_iterator it(zones.upper_bound(name));
 > >         --it;
 > > }}}
 > >   what if upper_bound returns begin() or end() iterator?
 >
 > Hmm. Decreasing end() in case the set is not empty is AFAIK valid
 operation. The begin could have been a problem. On the other hand, it is
 test code and it didn't happen, so it was no big deal. Anyway, fixed.

 As for end(), I found this
 http://stackoverflow.com/posts/5916501/revisions
 and it seems to support the use of -- for end() (unless the
 set is empty).

 As for begin(), it's clearly invalid, and, in general, I'd try to keep
 our code as clean as possible.  For tests there may be some cases
 where brevity is more important, but even in that case I'd like to
 leave some comments about the intent.

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