BIND 10 #1207: Enable the data source factory
BIND 10 Development
do-not-reply at isc.org
Tue May 15 01:25:16 UTC 2012
#1207: Enable the data source factory
-------------------------------------+-------------------------------------
Reporter: | Owner: jinmei
stephen | Status: reviewing
Type: task | Milestone:
Priority: | Sprint-20120515
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 |
-------------------------------------+-------------------------------------
Comment (by 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/
- 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.
- I've made a few editorial/style fixes
'''factory.h''
- not a big deal, but it doesn't seem to have to include the generic
exceptions.h.
- 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`.
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.
'''factory.cc''
- what does it include masterload.h?
'''auth_config.cc'''
- Do we need the cast here?
{{{#!cpp
(void)server_.getInMemoryClientContainer(rrclass_);
}}}
- 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.
- 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()`.
- 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.
- 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.
'''auth_srv.h'''
- s/InvalidOperation/InvalidParameter/?
{{{#!cpp
/// \exception InvalidOperation if the memory datasource has not been
set
/// (callers can check with \c hasMemoryDataSource())
}}}
- couldn't `FakeInMemoryClient` now be more generic, i.e, a direct
derived class of `DataSourceClient`?
- 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.
'''command.cc'''
- I'd like to explain in comments why we use static_pointer_cast (why
we can't use dynamic cast).
'''auth_srv_unittest'''
- Likewise, I'd like to explain why we need to clear the message
explicitly.
'''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);
}}}
btw, this cast can be a bit simpler:
{{{#!cpp
static_cast<InMemoryClient&>(dsrc->getInstance()).addZone(
}}}
'''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.
'''memory_datasrc_link.cc'''
- I'd add some comment to `InMemoryConfigError` about what it is.
- 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?)
{{{#!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?
- 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?)
- 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)
--
Ticket URL: <http://bind10.isc.org/ticket/1207#comment:12>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list