BIND 10 #1207: Enable the data source factory

[BIND 10 Development]

#1207: Enable the data source factory
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120529
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:12 jinmei]:
 > '''general points'''
 >
 > - It's known that tests using dynamic modules don't work with static
 >   link, at least not on some systems.  I've not checked that myself,
 >   but you may want to check it.  Look for the keyword 'USE_STATIC_LINK'
 >   in auth/tests/

 Hmm, i don't see anything fail on my system. I hope static linking does
 not disable dlopen completely, does it?
 (btw, i am revisiting this comment, and now wonder whether the last
 changes made actually fix this issue, or make it worse)

 > - ideally, it'd be better to move data source specific configuration
 >   tests to datasrc.  Then the auth implementation will be more data
 >   source independent.  But if it requires too many changes I'm okay
 >   with deferring it to a later task.

 Yes, that was the original intent. But I just removed the last InMemory-
 specific things, and perhaps it is quite nice now.

 > '''factory.h''
 >
 > - not a big deal, but it doesn't seem to have to include the generic
 >   exceptions.h.

 Removed.

 > - I think we need some clarification about what
 >   `DataSourceClientContainer` should be.  When I first saw it be
 >   changed to a base class, it seemed to be too ad hoc a hack, even for
 >   better testability.  But on further thought, it might not be that a
 >   bad idea - dynamic load setup could be troublesome in general (as
 >   we're now seeing there can be many weird portability issues with
 >   it), so we may want to allow data source backends to be used via
 >   normal link, while still providing transparent interface for
 >   applications.  If we go for it, however, I'd like to make the class
 >   design clearer, so that the base class is really abstract and
 >   move the current `DataSourceClientContainer` class to a separate
 >   derived class, say, `DyloadDataSourceClientContainer`.
 >

 Hmm. Supporting both might be even trickier. But certainly worth exploring
 (maybe it's not).

 I kind of considered it to indeed be an ad-hoc hack for testing which is
 slightly cleaner than having methods and members to the core class that
 only serve this purpose :) But yes, perhaps we should indeed go 'all the
 way' (even if combining real-linking and dynamic linking turns out to be
 too complicated, it would still be cleaner for the current ways it is
 used).

 >   Otherwise, I'd rather keep the container class as a leaf, and just
 >   add a separate constructor (noting that it's for testing purpose
 >   only), which would take `DataSourceClient*` and let the container
 >   object hold it in a trivial manner (we'd probably need some other
 >   tricks such as hiding the member variables).  That's because
 >   inheritance introduces a very strong coupling to yet-to-see derived
 >   classes, which may cause troubles in future.
 >
 >   For the purpose of this ticket, the first approach probably requires
 >   too much change, though.  So I'm okay with deferring it to a
 >   separate subtask.  It even applies to the second approach.  In these
 >   cases, I'd at least like to see a comment about the current intent
 >   and the revise plan about the class design.
 >

 Added comment.

 > '''factory.cc''
 >
 > - why does it include masterload.h?
 >

 oh I had been trying out something and didn't remove this, fixed.

 > '''auth_config.cc'''
 >
 > - Do we need the cast here?
 > {{{#!cpp
 >     (void)server_.getInMemoryClientContainer(rrclass_);
 > }}}
 >

 Not necessarily, but I tend to use that construct to show the return value
 is intentionally ignored. Granted, using an old-style cast :) (tbh, I
 think such use shows that we need a separate call to check for the
 rrclass, btw). If you dislike it much, I can drop the cast :)

 > - It seems we can actually even eliminate the need for
 >   `MemoryDatasourceConfig` completely.  Although it may look awkward to
 >   call something like `setInMemoryClient()` from the generic
 >   `DatasourcesConfig` class, this function itself has already become
 >   generic, and for longer term auth_config must become data-source type
 >   independent anyway, so it's one step in the right direction.
 >

 oh right, good point.

 Moved the special case into DatasourcesConfig. This also allowed for the
 removal of a few other special casing; createAuthConfigParser no longer
 needs the "datasources/memory" check, and no longer needs the bool
 internal argument, so the overloaded method itself isn't needed anymore.

 > - If possible (i.e, if it doesn't break other tests), I'd suggest
 >   moving the RR class check in `MemoryDatasourceConfig::build()`
 >   to the in-memory factory.  That way we can make auth_config even
 >   more type independent, and we can unify `getInMemoryClientContainer()`
 >   to `getInMemoryClient()`.
 >

 The factory itself shouldn't need or want class information, so maybe I
 misunderstand you here.

 I think the class check itself is not needed, actually, afaict it is
 currently only there because we can only set one in-memory datasource
 right now, and we want it to be IN (so actually, we could simply do one
 check upon first encountering the config item 'class'). But whether or not
 we even check that, this might become moot anyway if we generalize it (as
 the whole method that checks it right now would disappear).

 > - On top of the previous one, `getInMemoryClient()` probably doesn't
 >   have to throw when it's given an unsupported RR class; it can simply
 >   return NULL.  `LoadZoneCommand::validate()` will throw on it itself.
 >

 it already does; in the current split case where we have a separate
 getInMemoryClient() and getInMemoryClientContainer() it does not, but IMO
 the throw there provides better error reporting than failing later on an
 empty shared pointer. (but see previous point).

 > - hasInMemoryClient(): since the shared pointer has type conversion
 >   function to bool, it can be:
 > {{{#!cpp
 >     return (impl_->memory_client_container_);
 > }}}
 >   although it may be a matter of taste.
 >

 ack, changed.

 > '''auth_srv.h'''
 >
 > - s/InvalidOperation/InvalidParameter/?
 > {{{#!cpp
 >     /// \exception InvalidOperation if the memory datasource has not
 been set
 >     ///            (callers can check with \c hasMemoryDataSource())
 > }}}
 >

 changed

 > - couldn't `FakeInMemoryClient` now be more generic, i.e, a direct
 >   derived class of `DataSourceClient`?
 >

 ack, done

 > - The way `client_` is managed in `FakeContainer` is not exception
 >   safe.  For tests it might be less critical, but in general I'd like
 >   to keep all code as clean as possible.  One solution is to let the
 >   constructor take the parameters for `FakeInMemoryClient` and
 >   create `FakeInMemoryClient` inside it, stored in a scoped_ptr.
 >   btw, see also the discussion about the `DataSourceClientContainer`
 >   design in factory.h.
 >

 ok, done

 > '''command.cc'''
 >
 > - I'd like to explain in comments why we use static_pointer_cast (why
 >   we can't use dynamic cast).
 >

 added.

 > '''auth_srv_unittest'''
 >
 > - Likewise, I'd like to explain why we need to clear the message
 >   explicitly.
 >

 added.

 > '''command_unittest'''
 >
 > - loadZoneSQLite3: is it possible to have the server apply the initial
 >   configuration?  Then we'll be able to make the code more
 >   type-independent, and can avoid having kludge like this:
 > {{{#!cpp
 >     // The 'public' factory API does not allow for direct internal calls
 such
 >     // as addZone, so purely for this test we do a quick cast
 >     static_cast<InMemoryClient*>(&dsrc->getInstance())->addZone(
 >         ZoneFinderPtr(new InMemoryZoneFinder(RRClass::IN(),
 >                                              Name("example.org"))));
 >     server_.setInMemoryClient(RRClass::IN(), dsrc);
 > }}}

 Yes, but doing it naively would have changed the test. What is tested here
 is not whether the zone is loaded when specified in the configuration, but
 whether the zone_load command actually reloads it. The kludge is there is
 a way to get an empty zone in. By using the configuration, the test
 reverts to something which is tested later anyway (loading from config).

 However, now that you mention it, we can do this much better.

 I've changed the test to indeed use the configuration, the update the
 *underlying database* (through the DS API, woohoo!), and then check to see
 that upon reload, the new data is present.

 >   btw, this cast can be a bit simpler:
 > {{{#!cpp
 >     static_cast<InMemoryClient&>(dsrc->getInstance()).addZone(
 > }}}
 >

 OK, did change that.

 > '''client.h'''
 >
 > `getZoneCount()` may or may not be well defined in general for more
 > dynamic data sources like databased one (and in general I don't like
 > to too much rely on `NotImplemented` exceptions).  And, in any event,
 > we'll need to revisit things like this in a more generic context of
 > how we provide APIs to get access to the set of zones of a specific
 > data source.  For now, I'm okay with keeping it this way, but I'd
 > suggest leaving a comment that it's a tentative API and will likely be
 > revisited.
 >

 I agree about the general and generic case, though I disagree for
 databases, where, given our schema, it would be almost trivial to
 implement.

 But i added a note about being tentative.

 > '''memory_datasrc_link.cc'''
 >
 > - I'd add some comment to `InMemoryConfigError` about what it is.
 >

 ok

 > - What's the purpose of the commented-out code?  Can't we simply
 >   remove these two lines?  (btw is that change okay in the first place?)

 No, oversight, I should've removed them. Done

 I don't see why we wouldn't allow a memory data source that happens to
 have no zones configured.

 > {{{#!cpp
 >         if (!config->contains("zones")) {
 >             // Assume empty list of zones
 >             //addError(errors, "No 'zones' element in memory backend
 config");
 >             //result = false;
 >         } else if (!config->get("zones") ||
 > }}}
 >
 > - in the previous code, is it possible that get() returns NULL while
 >   contains doesn't?
 >

 Yes there is a semantical difference; contains returns true or false
 signaling whether any value with the given key exists. get() returns the
 value if it exists, but the value *itself* can also be null (technically,
 since zones is a non-optional value, this should never happen, but like
 the below points, the way the api is and is used, it's hard to get to that
 default, and i suspect we actually do misuse the flexibility in a number
 of tests).

 > - This comment doesn't make sense because it's replaced before the
 >   class check, and the class check now doesn't even have a comment:
 > {{{#!cpp
 >         // XXX: Like the RR class, we cannot retrieve the default value
 here,
 >         // so we assume an empty zone list in this case.
 > }}}
 >   (Is there any reason for the reordering and removing the comment?)
 >

 no, changed order again, and re-added comments. But I have changed them
 slightly :)

 > - createInstance(): personally, I'd manage `client` in an RAII manner
 >   to make the code more robust (by eliminating the need for explicit
 >   delete in each catch block), e.g.
 > {{{#!cpp
 >         auto_ptr<InMemoryClient> client(new
 isc::datasrc::InMemoryClient());
 >         applyConfig(client.get(), config);
 >         return (client.release());
 > }}}
 >   (for that matter, we could pass applyConfig() `*client` because we
 >   know it's not NULL)

 ack. updated.


 (I have not gone through these in complete chronological order, so if you
 happen to check them by commit, read the commit messages :))

 Oh and finally, with these changes, we *can* remove InMemory lib source
 dependency from the makefiles!
 So I did in the final commit.

 We better run this through buildbot before merging ;)

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