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

 '''design/general'''

 - I made a few editorial/style cleanups.
 - as for the build issue due to EXPECT_EQ, I tentatively fixed it
   using EXPECT_TRUE and operator==.  If you want please override it.
 - (note: I'm not requesting a change to this branch by this).  While
   it's understandable to introduce the trick with `DataSourcePair` to
   avoid handling `DataSourceClientContainer`, it seems to make the
   code fragile; we need to be careful about the integrity between the
   bare pointer and its container (see also the related note about
   `DataSourceInfo` below - maybe we could mitigate it by imposing some
   restrictions to the access of `DataSourceInfo`).  This seems to me
   to indicate we need more flexibility for `DataSourceClientContainer`
   as we discussed it in #1207 (see the second bullet for the factory.h
   comments of http://bind10.isc.org/ticket/1207#comment:12).
   If `DataSourceClientContainer` is completely abstract, we can more
   easily introduce a test version, without requiring tricky dy load
   stuff, and we could make its user like the `Container` (as defined
   in this branch) much simpler.  But even if this idea makes sense,
   that's certainly beyond the scope of this ticket.

 - I see general confusion between the concept of "data sources" and of
   "data source clients".  Throughout this branch, things tend to be
   called "data source" while they are actually used/defined in terms
   of their role/nature.  Maybe one underlying issue is that adding
   `Client` tends to make the class/variable names longer, and it
   probably acts as a barrier against giving them better names.  I
   don't know what's the best way to get a better balance, but I
   suggest revisiting the names that have just "data source" to see
   whether we can make it less confusing and yet not so verbose.

 - It's not clear to me what's the purpose of the base `Container`
   class.  In the test, you introduce a direct derived class of
   `ConfigurableContainer`, so essentially the pure base class is just
   a redundant layer.  While it might look cleaner in terms of the
   object oriented design, if it's basically a compromise for better
   testability (and the actual inheritance for the test is actually
   quite ugly anyway), I'm not sure if the purity is worth the
   overhead and complexity due to the additional layer.

 - On a related point, I'm concerned about the protected members of
   `ConfigurableContainer`, and, in particular, `data_sources_`.  Such
   a mutable bare member variable makes the class design very fragile;
   a naively implemented new derived class can break implicit/explicit
   assumptions in the base class, and can break applications that use
   the base class interface on top of the assumption.  If it's only for
   better tests and it's not expected to be derived in other way, I can
   probably live with it, but this comment seems to suggest that it's
   actually (possibly) assumed to be derived by other general-purpose
   classes:
 {{{#!cpp
     /// Also, derived classes might want to create the data sources
     /// in a different way.
 }}}
   If that's the intent, I'd like to make the class design more
   robust so a derived class cannot easily break the assumptions in the
   base class.  If it's actually not intended and
   `ConfigurableContainer` is only expected be derived for testing
   purposes, I'd like to clarify that in the comment, which would
   strongly discourage others to introduce a derived class of it.

 '''container.h'''

 - I'd like to clarify in the doc that a container is expected to
   contain data source clients for the same single RR class (is my
   understanding correct?).

 - ConfigurableContainer: is it supposed to be "inheritable" in general?

 - ConfigurableContainer::configure: it seems that the type of
   'configuration' can be a reference not a pointer (then we don't have
   to worry about the case it's NULL).

 - ConfigurableContainer::search: maybe 'find' is a better name for
   compatibility with STL containers (in terms of naming convention)?

 - ConfigurableContainer::search: I'd clarify in the doc the lifetime
   and ownership of the returned data source (client) (in the form of a
   pointer).  They are valid (only) as long as the container is alive
   (without being reconfigured), and the ownership isn't transferred to
   the caller (so the caller doesn't have to / should not delete it),
   right?  I think it should be explicitly clarified.

 - ConfigurableContainer::data_sources_: better alternative?  If not,
   at least emphasize more strongly that it shouldn't be used for other
   purposes (and maybe the class shouldn't even be inherited by others
   except for tests).

 - ConfigurableContainer::DataSourceInfo: isn't it better to introduce
   a constructor to initialize the struct than letting the caller
   assign them one-by-one?  Also, if it's supposed to be read-only
   after construction, I'd make members const.  Together, we can ensure
   the integrity of the struct as a whole. (hmm, maybe it doesn't work
   really well if we want to store it in the vector?)

 - ConfigurableContainer::dataSources(): per our naming convention, it
   should probably be named getDataSources

 '''container.cc''

 - - ConfigurableContainer::configure: (minor) is "recyclation" an
   English word?  My dictionaries don't seem to have it (not critical
   though, I think I understand what this means in this context anyway)

 - ConfigurableContainer::configure: what if config is a NULL pointer?

 - ConfigurableContainer::configure: is `TypeError` the only possible
   exception that can be thrown from the element module (excluding
   bad_alloc-like things)?


 - ConfigurableContainer::search: use of dynamic allocation.  Also
   about performance consideration in general.

 - ConfigurableContainer::search: I don't understand why we need the
   curly braces for each case.  Can't we do it like this?
 {{{#!cpp
             case result::SUCCESS:
                 // If we found an exact match, we have no hope to getting
                 // a better one. Stop right here.

                 // TODO: In case we have only the datasource and not the
 finder
                 // and the need_updater parameter is true, get the zone
 there.
                 return (SearchResult(info.data_src_, result.zone_finder,
                                      name.getLabelCount(), true));
 }}}

 - ConfigurableContainer::getDataSource: why '::' before
   `ConstElementPtr`?
 {{{#!cpp
 ConfigurableContainer::getDataSource(const string& type,
                                      const ::ConstElementPtr&
 configuration)
 }}}
   not incorrect, but unnecessary and looks quite awkward/confusing to
   me.  At least as far as I know we normally don't add it in such a
   case.

 '''container_unittest'''

 - I'd name it `TestDS` differently in terms of differentiating "data
   source" and "data source client'.  At the risk of sounding verbose,
   TestDataSourceClient (I personally don't like the abbrev of "DS"
   because it could be confused with the DS RR type) or if we want to
   make it short, maybe TestDSC.

 - is this safe?
 {{{#!cpp
         set<Name>::const_iterator it(zones.upper_bound(name));
         --it;
 }}}
   what if upper_bound returns begin() or end() iterator?

 - about ds_count: this hardcoding can be avoided this way:
 {{{#!cpp
 const char* ds_zones[][3] = {
 ...
 };

 const size_t ds_count = (sizeof (ds_zones) / sizeof (ds_zones[0]));
 }}}
   This will be more robust when you change the number of data source
   (client)s.

 - considering a bare pointer as boolean is against our style
   guideline:
 {{{#!cpp
         // Comparing with NULL does not work
         ASSERT_TRUE(ds);
 }}}
   Though awkward, our usual workaround in this case is:
 {{{#!cpp
         // Comparing with NULL does not work
         ASSERT_NE(static_cast<const TestDS*>(NULL), ds);
 }}}
   (or at least this one works, right?)
 {{{#!cpp
         // Comparing with NULL does not work
         ASSERT_TRUE(ds != NULL);
 }}}

 - multiConfiguration: what's `ds3`? `ds_[2]`?
 {{{#!cpp
                 // The ds3 is empty. We just check that it doesn't confuse
                 // us. Fall through to the case 0.
 }}}

 - multiConfiguration: what's '2' in 'as 2'? `ds_[1]`?
 {{{#!cpp
                 // It is the same as 2, but we take from the first one.
                 // The first one to match is the correct one.
 }}}


 - selfTest: I'd adding cases like these:
 {{{#!cpp
     EXPECT_EQ(result::NOTFOUND, ds_[0]->findZone(Name("aaa")).code);
 }}}
   (see the comment about upper_bound above - hmm, the sub.example.org
   case seems to indicate --end() works as expected.  but is it
   guaranteed?)

 - singleDSBestMatch: this comment is incorrect (seemingly a naive
   copy-paste):
 {{{#!cpp
     // When asking for a sub zone of a zone there, we get nothing
     // (we want exact match, this would be partial one)
     positiveResult(container_->search(Name("sub.example.org.")),
                    ds_[0], Name("example.org"), false, "Subdomain match");
 }}}

 - multiExactMatch: hardcoding of 4 can be avoided:
 {{{#!cpp
     for (size_t i(0); i < sizeof(test_names) / sizeof(test_names[0]); ++i)
 {
 }}}
   and should be more robust against future extensions.  same for
   multiBestMatch.

 - wrongConfig: technically, this test is not ideal
 {{{
         // Still untouched
         checkDS(0, "test_type", "{}");
 }}}
   because the whole new config is broken in the test.  Ideally, we
   first include a validly configured data source (client) followed by
   a broken one, and see the valid one doesn't happen in the config and
   the old one is intact.

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