BIND 10 #1975: implement meta-or-container-of data source
BIND 10 Development
do-not-reply at isc.org
Tue Jun 12 12:43:39 UTC 2012
#1975: implement meta-or-container-of data source
-------------------------------------+-------------------------------------
Reporter: | Owner: vorner
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 vorner):
Hello
Replying to [comment:9 jinmei]:
> - (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.
Yes, that would indeed be nice. A clean-up ticket, then?
> - 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.
The problem might be I'm not really clear about the difference myself.
What is the difference between data source and data source client? Where
can I meat the first one, if we have classes for the second only?
I guess that if I myself am confused about the difference, it is projected
to the code and comments I write. I might try to improve it after I
understand.
> - 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.
I had two purposes in my mind when I thought about it.
* If we ever need a different matching strategy than best match, we can
have a different subclass of it.
* The query processing and auth server will need to be tested. This
includes returning various results from the Container class. For that, it
might be better to be able to put a mock class there instead of the real
one.
However, this support is not for testing the Container itself.
Do you think it is overkill and the classes could be squashed together?
> - 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.
Well, I didn't really discourage deriving something from it, but I don't
see any reason to really do it, so I didn't spend too much time in
protecting the internals from it. So I made a note that deriving the class
is not recommended and that the `data_sources_` variable should be
considered private in such case.
> '''container.h'''
> - 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).
You're right. While most of our code uses the shared pointers for
configuration, it doesn't seem to be necessary here.
> - 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.
I clarified it with the SearchResult, as it seems like more logical place
to me.
> - 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).
I added a note to it. I don't know if there's some better alternative, I
don't seem one that would not be too much work.
> - 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?)
The consts would not, unfortunately, work with vector. I could use (I
think) std::list, but it would be bad for performance, since lists are
slower than vectors, due to more complicated iteration through pointers
and worse cache effect (as the nodes are far away from each other).
I intended the DataSourceInfo to hold more things later on ‒ at least
configuration and the in-memory cache for it. I think the data structure
would be better initialized in several steps, as it would be more
complicated, so I didn't provide the constructor. But you're right, it
probably makes sense to provide a constructor for the things which are
there right now.
> - ConfigurableContainer::configure: is `TypeError` the only possible
> exception that can be thrown from the element module (excluding
> bad_alloc-like things)?
Yes, when I look at the data.{cc,h} module, the only thing it throws is
either `TypeError` or `JSONError`, the later one only when parsing JSON.
> - 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));
> }}}
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?
> - 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.
Leftover, there was a namespace once, but never more O:-).
> - 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.
> - 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.
> }}}
Ah, right. Relics of code refactored later on.
More comments will be addressed tomorrow.
--
Ticket URL: <http://bind10.isc.org/ticket/1975#comment:12>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list